Closed
Bug 1055533
Opened 9 years ago
Closed 9 years ago
Implement Element.closest()
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: crimsteam, Assigned: agi, Mentored)
Details
(Keywords: dev-doc-complete, Whiteboard: [lang=c++])
Attachments
(2 files, 2 obsolete files)
8.07 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.71 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
||
Updated•9 years ago
|
Mentor: bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [lang=c++]
Comment 2•9 years ago
|
||
Hi, Lynn. Thanks for your help! Boris, can you point to a DXR link to get Lynn started?
Assignee | ||
Comment 3•9 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 :)
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8480349 [details] [diff] [review] Implement closest() bz what do you think?
Attachment #8480349 -
Attachment description: WIP → Implement closest()
Attachment #8480349 -
Flags: review?(bzbarsky)
![]() |
||
Comment 6•9 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. :(
![]() |
||
Comment 7•9 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
Attachment #8480349 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 8•9 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
Assignee: nobody → agi.novanta
Attachment #8480349 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8481984 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•9 years ago
|
Keywords: dev-doc-needed
![]() |
||
Comment 9•9 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
Attachment #8481984 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 10•9 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...
Assignee | ||
Comment 11•9 years ago
|
||
OK, I'm following the discussion on www-dom as well.
![]() |
||
Comment 12•9 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.
Assignee | ||
Comment 13•9 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!
Attachment #8481984 -
Attachment is obsolete: true
Attachment #8489043 -
Flags: review?(bzbarsky)
![]() |
||
Comment 14•9 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?
Attachment #8489043 -
Flags: review?(bzbarsky) → review+
Flags: needinfo?(agi.novanta)
Assignee | ||
Comment 15•9 years ago
|
||
No, I don't have Level 3 yet. Thanks bz!
Flags: needinfo?(agi.novanta)
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a567789cf4ae
Keywords: checkin-needed
Comment 17•9 years ago
|
||
Updated•9 years ago
|
Attachment #8489263 -
Flags: review?(bzbarsky)
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a567789cf4ae
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
![]() |
||
Comment 19•9 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....
Attachment #8489263 -
Flags: review?(bzbarsky) → review+
Comment 21•9 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
Comment 23•9 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
Keywords: dev-doc-needed → dev-doc-complete
Comment 24•9 years ago
|
||
I landed the tests in web-platform-tests so we'll get them on the next update.
Comment 25•9 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?
Flags: needinfo?(jypenator)
Comment 26•9 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.
Flags: needinfo?(jypenator)
You need to log in
before you can comment on or make changes to this bug.
Description
•