| Summary: | Make document.activeElement work with shadow content. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hayato Ito <hayato> | ||||||||
| Component: | DOM | Assignee: | Hayato Ito <hayato> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | dglazkov, dominicc, morrita, rolandsteiner, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 61409, 61410 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Hayato Ito
2011-05-24 19:29:20 PDT
Created attachment 95163 [details]
Add a guard to document.activeElement
Please don't forget to cc peeps who should review! I didn't even realize this was here. Comment on attachment 95163 [details]
Add a guard to document.activeElement
actually.... shouldn't we be doing this in HTMLDocument::focusedNode?
Thank you for the review. Yes, it's nice that document.activeElement should be the same to the return value of HTMLDocument::focusedNode(). Its on my rader, but there remains things to be solved to achieve that. I explained the current status and my rough idea in bug 61532 (https://bugs.webkit.org/show_bug.cgi?id=61532#c1). That might expain why we cannot simply modify Document::focusedNode() in this case. I'd appreciate if you could see that and give me your thoughts. (In reply to comment #3) > (From update of attachment 95163 [details]) > actually.... shouldn't we be doing this in HTMLDocument::focusedNode? Comment on attachment 95163 [details] Add a guard to document.activeElement View in context: https://bugs.webkit.org/attachment.cgi?id=95163&action=review > Source/WebCore/html/HTMLDocument.cpp:142 > + if (!node) { Could you split out this loop logic into a static function or Frame method? nested if-if-for-if looks complicated enough. > Source/WebCore/html/HTMLDocument.cpp:152 > + ASSERT(!node || node->document() == this); I'd like to return early here when node is null. Created attachment 96380 [details]
Patch
Comment on attachment 95163 [details] Add a guard to document.activeElement View in context: https://bugs.webkit.org/attachment.cgi?id=95163&action=review >> Source/WebCore/html/HTMLDocument.cpp:142 >> + if (!node) { > > Could you split out this loop logic into a static function or Frame method? nested if-if-for-if looks complicated enough. Sure. I splited it out to a static function. >> Source/WebCore/html/HTMLDocument.cpp:152 >> + ASSERT(!node || node->document() == this); > > I'd like to return early here when node is null. Done. Comment on attachment 96380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96380&action=review Code looks good! Only thing left is paying a duty... > LayoutTests/fast/dom/shadow/activeelement-should-be-shadowhost-expected.txt:8 > + I'd like to have more test cases that - returns <body> - when the focus is inside an iframe - when the focus is inside a nested iframe - call activeElement() of detached document - call activeElement() of iframed-document - with several focus variations. These should be done by original authors. But I bet it isn't... > Source/WebCore/html/HTMLDocument.cpp:159 > + return toElement(node); Can |node| be null? Created attachment 96403 [details]
Patch
Comment on attachment 96380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96380&action=review >> LayoutTests/fast/dom/shadow/activeelement-should-be-shadowhost-expected.txt:8 >> + > > I'd like to have more test cases that > - returns <body> > - when the focus is inside an iframe > - when the focus is inside a nested iframe > - call activeElement() of detached document > - call activeElement() of iframed-document > - with several focus variations. > These should be done by original authors. But I bet it isn't... The most cases are already covered in fast/dom/HTMLDocument/active-element-frames.html. I agree that we need more coverage which involves shadow, so I rewrote the test, adding the following cases: - shadow in iframe - shadow in shadow - iframe in shadow >> Source/WebCore/html/HTMLDocument.cpp:159 >> + return toElement(node); > > Can |node| be null? Yeah, that cannot be null. Before |node| becomes null, we surely reach a node whose treescope is same to this document. I removed the null check and added assertion instead. Comment on attachment 96403 [details]
Patch
Now it got good coverage! Let's land this.
Comment on attachment 96403 [details] Patch Clearing flags on attachment: 96403 Committed r88418: <http://trac.webkit.org/changeset/88418> All reviewed patches have been landed. Closing bug. |