Bug 112992 – document.activeElement should not point elements with display:none

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: DOM

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 194418 [details]
Patch

[Hajime Morrita]

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".

[Kent Tamura]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 194447 [details]
WIP

[Alexey Proskuryakov]

> 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.

[Kent Tamura]

(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.

[Kent Tamura]

We should resolve Bug 32292 before this. Plugins would loose focus if we landed a patch like one I posted.

[Kent Tamura]

(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.

[Kent Tamura]

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.

[Kent Tamura]

*** Bug 116925 has been marked as a duplicate of this bug. ***

[Kent Tamura]

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