Bug 61413

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

61409, 61410

Attachments:

Description	Flags
Add a guard to document.activeElement	none
Patch	none
Patch	none

Hayato Ito

Reported 2011-05-24 19:29:20 PDT

We should return a right element when 'document.activeElement' is used in JavaScript if some elements in shadow content is currently focused. I think we should return a shadow host element in that case. We should make sure what will be returned.

Attachments
Add a guard to document.activeElement (5.27 KB, patch) 2011-05-27 06:03 PDT, Hayato Ito	no flags	Details Formatted Diff Diff
Patch (5.46 KB, patch) 2011-06-07 22:14 PDT, Hayato Ito	no flags	Details Formatted Diff Diff
Patch (10.54 KB, patch) 2011-06-08 03:57 PDT, Hayato Ito	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Hayato Ito

Comment 1 2011-05-27 06:03:06 PDT

Created attachment 95163 [details] Add a guard to document.activeElement

Dimitri Glazkov (Google)

Comment 2 2011-05-27 14:27:14 PDT

Please don't forget to cc peeps who should review! I didn't even realize this was here.

Dimitri Glazkov (Google)

Comment 3 2011-05-27 14:29:55 PDT

Comment on attachment 95163 [details] Add a guard to document.activeElement actually.... shouldn't we be doing this in HTMLDocument::focusedNode?

Hayato Ito

Comment 4 2011-05-29 22:32:41 PDT

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?

Hajime Morrita

Comment 5 2011-06-06 19:26:30 PDT

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.

Hayato Ito

Comment 6 2011-06-07 22:14:52 PDT

Created attachment 96380 [details] Patch

Hayato Ito

Comment 7 2011-06-07 22:15:20 PDT

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.

Hajime Morrita

Comment 8 2011-06-07 23:41:30 PDT

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?

Hayato Ito

Comment 9 2011-06-08 03:57:25 PDT

Created attachment 96403 [details] Patch

Hayato Ito

Comment 10 2011-06-08 03:58:00 PDT

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.

Hajime Morrita

Comment 11 2011-06-08 20:17:27 PDT

Comment on attachment 96403 [details] Patch Now it got good coverage! Let's land this.

WebKit Review Bot

Comment 12 2011-06-08 20:55:29 PDT

Comment on attachment 96403 [details] Patch Clearing flags on attachment: 96403 Committed r88418: <http://trac.webkit.org/changeset/88418>

WebKit Review Bot

Comment 13 2011-06-08 20:55:34 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.