Bug 112992 - document.activeElement should not point elements with display:none
Summary: document.activeElement should not point elements with display:none
Status: RESOLVED DUPLICATE of bug 29241
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords: BlinkMergeCandidate
: 116925 (view as bug list)
Depends on: 32292
Blocks: 119713
  Show dependency treegraph
 
Reported: 2013-03-21 19:28 PDT by Kent Tamura
Modified: 2013-09-22 06:19 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.11 KB, patch)
2013-03-21 19:38 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (845.54 KB, application/zip)
2013-03-21 21:29 PDT, WebKit Review Bot
no flags Details
WIP (5.71 KB, patch)
2013-03-21 23:06 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2013-03-21 19:28:49 PDT
Steps to reproduce the bug:

<div id=div1 tabindex=1></div>
<script>
div1.focus();
div1.style.display = 'none';
// Check document.activeElement
</script>

The original bug report is http://crbug.com/178146. This bug is serious because implicit form submission by Enter key can proceed with invisible text inputs.
Comment 1 Kent Tamura 2013-03-21 19:38:09 PDT
Created attachment 194418 [details]
Patch
Comment 2 Hajime Morrita 2013-03-21 20:23:37 PDT
Comment on attachment 194418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194418&action=review

> Source/WebCore/dom/Document.cpp:1904
> +

Should we do this here or do the check on Document::activeElement()?
My concerns are:
- Whether this is a "catch all" location.
- If we want to restore focus after setting display property back to something other than "none".
Comment 3 Kent Tamura 2013-03-21 20:42:12 PDT
Comment on attachment 194418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194418&action=review

>> Source/WebCore/dom/Document.cpp:1904
>> +
> 
> Should we do this here or do the check on Document::activeElement()?
> My concerns are:
> - Whether this is a "catch all" location.
> - If we want to restore focus after setting display property back to something other than "none".

> - If we want to restore focus after setting display property back to something other than "none".

good point.  I confirmed IE, Firefox, and Opera kept focus state when we set display:none then display:block.  This patch is not compatible.
Comment 4 WebKit Review Bot 2013-03-21 21:29:02 PDT
Comment on attachment 194418 [details]
Patch

Attachment 194418 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17193608

New failing tests:
fast/events/tab-imagemap.html
accessibility/canvas-fallback-content-2.html
fast/events/mouse-focus-imagemap.html
fast/events/imagemap-norender-crash.html
accessibility/canvas-fallback-content.html
platform/chromium/virtual/gpu/fast/canvas/fallback-content.html
fast/events/tab-focus-link-in-canvas.html
fast/canvas/fallback-content.html
Comment 5 WebKit Review Bot 2013-03-21 21:29:06 PDT
Created attachment 194438 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 6 Kent Tamura 2013-03-21 22:07:53 PDT
(In reply to comment #4)
> New failing tests:
> fast/events/tab-imagemap.html
> accessibility/canvas-fallback-content-2.html
> fast/events/mouse-focus-imagemap.html
> fast/events/imagemap-norender-crash.html
> accessibility/canvas-fallback-content.html
> platform/chromium/virtual/gpu/fast/canvas/fallback-content.html
> fast/events/tab-focus-link-in-canvas.html
> fast/canvas/fallback-content.html

Hmm, we must check isFocusable. Some elements are focusable even if they don't have renderers.
Comment 7 Kent Tamura 2013-03-21 23:06:14 PDT
Created attachment 194447 [details]
WIP
Comment 8 Alexey Proskuryakov 2013-03-22 11:25:24 PDT
> This bug is serious because implicit form submission by Enter key can proceed with invisible text inputs.

Does it work in other browsers?

Sometimes sites focus an invisible element intentionally, so that they could more deeply customize the UI. http://tryruby.org used to employ this trick to draw a custom caret, and judging from the looks, it might still be. I don't remember if it was display:none or visibility:hidden, however the rationale you posted applies to both equally.
Comment 9 Kent Tamura 2013-03-24 16:22:12 PDT
(In reply to comment #8)
> > This bug is serious because implicit form submission by Enter key can proceed with invisible text inputs.
> 
> Does it work in other browsers?

IE and Firefox don't have this bug, and the HTML standard clearly says we should loose focus when a focused element becomes non-focusable.
Comment 10 Kent Tamura 2013-03-24 18:16:01 PDT
We should resolve Bug 32292 before this. Plugins would loose focus if we landed a patch like one I posted.
Comment 11 Kent Tamura 2013-03-24 22:19:56 PDT
(In reply to comment #3)
> > - If we want to restore focus after setting display property back to something other than "none".
> 
> good point.  I confirmed IE, Firefox, and Opera kept focus state when we set display:none then display:block.  This patch is not compatible.

Ah, my test code was wrong!  They don't restore focus state when the element becomes focusable again. We don't need to record the last focused node.
Comment 12 Kent Tamura 2013-05-22 15:01:01 PDT
We fixed this in Blink by http://src.chromium.org/viewvc/blink?view=revision&revision=150870 .
However we feel the fix is not cool and wondering if there is a better way.
Comment 13 Kent Tamura 2013-05-29 01:00:16 PDT
*** Bug 116925 has been marked as a duplicate of this bug. ***
Comment 14 Kent Tamura 2013-09-22 06:19:07 PDT

*** This bug has been marked as a duplicate of bug 29241 ***