Arkadiusz Michalski (Spirit) Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

This method is new in DOM spec:
http://dom.spec.whatwg.org/#dom-element-closest
https://www.w3.org/Bugs/Public/show_bug.cgi?id=16488

At now its not yet implemented anywhere but maybe Firefox will be first.

	Lynn Tran
	Comment 1 • 10 years ago

Hi, I'm interested in this bug. Where should I start with this?

	Mike Hoye [:mhoye]
	Comment 2 • 10 years ago

Hi, Lynn. Thanks for your help! Boris, can you point to a DXR link to get Lynn started?

	[ex-Mozilla] Agi Sferro | :agi Assignee
	Comment 3 • 10 years ago

So I was actually going to work on this (I've already emailed Lynn about it). I have a patch ready I just have to re-read one more time before posting it :)

	[ex-Mozilla] Agi Sferro | :agi Assignee
	Comment 5 • 10 years ago

Comment on attachment 8480349 [details] [diff] [review]
Implement closest()

bz what do you think?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 10 years ago

Ugh.  I'm sorry I didn't see comment 2; apparently being a mentor on a bug doesn't mean you get mail for it.  :(

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 10 years ago

Comment on attachment 8480349 [details] [diff] [review]
Implement closest()

You don't need the NS_IMETHOD bits in Element.h: this API is not declared in nsIDOMElement.idl (and shouldn't be).  That also means you don't need the nsresult-returning version of this method.

You also don't need to add anything to nsINode.  This API should live on Element only, just like Matches() does.

Also like :matches(), you want to call OwnerDoc()->FlushPendingLinkUpdates() in here before doing any matching.

The rest of this looks great.  r=me with the above simplifications

	[ex-Mozilla] Agi Sferro | :agi Assignee
	Comment 8 • 10 years ago

Thanks for the quick review bz! I did all the changes, asking to review again in case you want to see the patch before checkin.

Green Try on Win & Android: https://tbpl.mozilla.org/?tree=Try&rev=de43c16c0e12

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 10 years ago

Comment on attachment 8481984 [details] [diff] [review]
Implement Element.Closest() ; r=bz

One more nit, because the spec is in the process of changing:  You want to do this right after creating the TreeMatchContext:

   matchingContext.SetHasSpecifiedScope();
   matchingContext.AddScopeElement(this);

and maybe add a basic test for closest(":scope")?

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 10 years ago

On the other hand, now Anne and Tab are arguing about how :scope is supposed to work here.  So we may want to wait until they decide something...

	[ex-Mozilla] Agi Sferro | :agi Assignee
	Comment 11 • 10 years ago

OK, I'm following the discussion on www-dom as well.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 years ago

Giovanni, looks like Anne and Tab agreed on what comment 9 says, so if you get a chance to do that, it would be wonderful.

	[ex-Mozilla] Agi Sferro | :agi Assignee
	Comment 13 • 10 years ago

With pleasure! I added those two lines and three very basic tests. 

The test ":has(> :scope)" throws because we haven't implmeneted :has yet, I suppose? (I can't find the bug for it). I added that as a todo.

Asking for review in case you want to see it again before checkin, thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 10 years ago

Comment on attachment 8489043 [details] [diff] [review]
Implement Element.Closest() v2

> The test ":has(> :scope)" throws because we haven't implmeneted :has yet,

Yep.

r=me.

Do you have the bits to push this to inbound, or should I do that?

	[ex-Mozilla] Agi Sferro | :agi Assignee
	Comment 15 • 10 years ago

No, I don't have Level 3 yet. Thanks bz!

	Carsten Book [:Tomcat]
	Comment 16 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a567789cf4ae

	Carsten Book [:Tomcat]
	Comment 18 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a567789cf4ae

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 10 years ago

Comment on attachment 8489263 [details] [diff] [review]
Fix web-platform-tests metadata for addition of Element.closest

r=me

We should consider adding web-platform-tests for the actual behavior of :closest....

	James Graham [:jgraham]
	Comment 20 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cd3333dbb2d

	James Graham [:jgraham]
	Comment 21 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #19) 
> We should consider adding web-platform-tests for the actual behavior of
> :closest....

Indeed we should. So I ported the mochitests from this patch. Needs some review (but not much since the actual tests are already reviewed) [1]

[1] https://critic.hoppipolla.co.uk/r/2600

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8cd3333dbb2d

	Jean-Yves Perrier [:teoli]
	Comment 23 • 10 years ago

With the help of Ziyunfei (who fixed my errors in the example and translated it to Chinese), doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/Element.closest
https://developer.mozilla.org/en-US/Firefox/Releases/35

	James Graham [:jgraham]
	Comment 24 • 10 years ago

I landed the tests in web-platform-tests so we'll get them on the next update.

	Merih Akar
	Comment 25 • 10 years ago

I couldn't see relevant documentation about Element.closest() in https://developer.mozilla.org/en-US/Firefox/Releases/35. Am I missing something?

Also I think this feature deserves to be rel-noted, what do you think?

	Jean-Yves Perrier [:teoli]
	Comment 26 • 10 years ago

merihakar: you are right, the mention in Fx 35 for devs wasn't there; it is now. Thanks for noticing it, I don't know what happened there.