1369072 - Focus caught by non-displayed element causes keyboard shortcuts to fail on youtube

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[David A. Madore]

This bug's description probably isn't very good, but I couldn't manage a better description.  Here's a way to reproduce using the YouTube web site:

HOW TO REPRODUCE: Go to https://www.youtube.com/ and hover mouse cursor above the link to a video's creator (user/channel): this will make a hovercard appear; right-click on the link to the creator's name inside the hovercard; once the Firefox contextual menu appears, move the mouse cursor away from the hovercard so that it disappears from view (it is actually marked as "display: none" in the DOM), while the menu is still open; now click on "copy link location" inside the menu.  At this point, I think Firefox thinks the link has the focus, but it's in a non-displayed HTML element.  Problem: various global keyboard navigation shortcuts, such as control-L to access the location bar or control-R to reload, stop working.

(This has nothing to do with YouTube, of course, it's just that YouTube's interface provides a convenient way to describe/reproduce the bug.  It's quite minor, but it still annoys me, because this is something I often want to do: get the URL for a YouTube creator and edit it before I follow it, typically because I want to go to ".../user/foobar/videos" rather than ".../user/foobar?feature=hovercard", but, because of this bug, after I copy the link I need to restore the focus by clicking somewhere else.)

I'm not sure how to triage this but: trying Firefox → Theme, sorry if this is inappropriate.

Comment 1

[:Gijs (out for now; he/him)]

Youtube must be doing something special, because here's a minimal testcase that does something similar to what you describe:

http://jsbin.com/suhasafora/edit?html,js,output

if you right click the link (in the right-hand pane) and wait for it to disappear (I'm hiding the containing div), and click 'copy link location', and then try to use shortcuts, the shortcuts work fine.

But I can reproduce the issue you describe on youtube... Masayuki, do you know what's going on here?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the long delay.

Looks like that the "hovercard" is <iframe>. After context menu is closed, PresShell still thinks KeyboardEvent target is the hidden (unloaded?) document. However, redirecting keyboard event to the hidden/unloaded document is failed in PresShell::HandleEvent. Then, it's not dispatched to the DOM tree (including chrome's DOM tree).

Therefore, you cannot reproduce this bug in e10s mode since main process won't hit this issue but cannot scroll the patch with keyboard navigation. So, this should be a focus management bug.

# Although, I don't know how PresShell manages focused document.

Comment 3

[Ben Tian (inactive)]

One different behavior on Firefox and Chrome&Safari:
When focus changes from top-level page to iframe, top-level page can still receive key events on Chrome & Safari but not on Firefox.

example: https://jsbin.com/cesahenoti/edit?html,output
1) Keep scrolling page down with DOWN key
2) Focus changes to iframe in 3 seconds and background color becomes green
3) Scrolling stops on Firefox but keeps going on Chrome & Safari

Comment 4

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #3)
> One different behavior on Firefox and Chrome&Safari:
> When focus changes from top-level page to iframe, top-level page can still
> receive key events on Chrome & Safari but not on Firefox.

Elaboration: On Chrome & Safari, top-level page receives keyboard navigation ONLY when iframe is not scrollable. On Firefox iframe receives keyboard navigation no matter it's scrollable or not.

example: https://jsbin.com/xocexokumi/1/edit?html,output

Comment 5

[Ben Tian (inactive)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> Sorry for the long delay.
> 
> Looks like that the "hovercard" is <iframe>. After context menu is closed,
> PresShell still thinks KeyboardEvent target is the hidden (unloaded?)
> document.

The <iframe> remains but moves to invisible position as clip:
https://drive.google.com/file/d/0B_re1yhsLbEDWE9HWnMyQUNFR3M/view

The iframe's (top, left) changes from (278px, 396px) to (-9990px, 10px) when mouse hovers out.

Comment 6

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #4)
> Elaboration: On Chrome & Safari, top-level page receives keyboard navigation
> ONLY when iframe is not scrollable. On Firefox iframe receives keyboard
> navigation no matter it's scrollable or not.
> 
> example: https://jsbin.com/xocexokumi/1/edit?html,output

Observation:
When scrolling to the bottom of iframe, Chrome & Safari send key navigation to top-level page instead, while Firefox doesn't.

Comment 7

[Ben Tian (inactive)]

New Youtube UI [1] doesn't show the hovercard now. Keep checking this bug with comment 3 & 4 examples.

[1] https://www.youtube.com/new

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49b0e9dbb41dd0e55ddc5207fd78a90b337193f5

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

As far as I investigated, there are 2 bugs here. One is when <iframe> has focus, arrow key (or other scroll related key) operations are consumed by the child document even if it doesn't have scrollable contents. This must be able to fix easily. The other is, if focused document is hidden by some reason, the document/window loses PresShell.  Therefore, PresShell stops dispatching coming keyboard events on the hidden document.  So, perhaps, keyboard events should be fired in its nearest visible ancestor document/window for avoiding users not to be able to continue navigating only with keyboard. So, the latter bug is a11y issue.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9b8ae1805dd3642af60861e1fdfd72813a9a8c2

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ba3ab39fadb8cedca8eae85a7ba60e749b56ab

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc78396981f72d3b6015650a7d849bff7419a14e

Comment 13

[Ben Tian (inactive)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> As far as I investigated, there are 2 bugs here. One is when <iframe> has
> focus, arrow key (or other scroll related key) operations are consumed by
> the child document even if it doesn't have scrollable contents.

Masayuki, should parent document scroll when iframe scrolls to the edges?
E.g., scrolling parent document up when ArrowUp keydown event is at vertically scrollable iframe whose contentDocument.documentElement.scrollTop is already 0.

Chrome and Safari do so while Edge doesn't.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ben Tian [:btian] from comment #13)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> > As far as I investigated, there are 2 bugs here. One is when <iframe> has
> > focus, arrow key (or other scroll related key) operations are consumed by
> > the child document even if it doesn't have scrollable contents.
> 
> Masayuki, should parent document scroll when iframe scrolls to the edges?
> E.g., scrolling parent document up when ArrowUp keydown event is at
> vertically scrollable iframe whose contentDocument.documentElement.scrollTop
> is already 0.
> 
> Chrome and Safari do so while Edge doesn't.

I don't think that it should depend on scrollTop and scrollLeft value of child document. I believe that it may make users confused. For example, when you scroll down parent document after iframe's child document is scrolled to its bottom, then, the iframe will be scrolled out from the viewport of the parent document. Finally, even when you use ArrowUp key, the "hidden" iframe is scrolled without any visual change of the parent document.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1369072 - part0: Add automated test for testing key event handlers to scroll parent document when an iframe element has or had focus

When an <iframe> element has focus and its sub document isn't scrollable, the
parent document should be scrolled instead.

Review commit: https://reviewboard.mozilla.org/r/176704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176704/

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1369072 - part1: PresShell should climb up scrollable frames when there is no selection, no focused content and root scrollable frame isn't scrollable

When there is no selection and no focused content, aContent of
nsIPresShell::GetScrollableFrameToScrollForContent() is nullptr.  Then, it'll
use root scrollable frame to scroll.  However, the root scrollable frame may
be not scrollable.  In such case, ancestor document's scrollable frame should
be scrolled instead.

Review commit: https://reviewboard.mozilla.org/r/176706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176706/

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1369072 - part2: PresShell::HandleEvent() should retarget KeyboardEvent if focused document is invisible

When focused document is invisible, PresShell::HandleEvent() won't dispatch
any events.  However, if it's KeyboardEvent, user cannot do anything only with
keyboard.  So, in this case, PresShell should retarget keyboard events to
first visible ancestor.

Review commit: https://reviewboard.mozilla.org/r/176708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176708/

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1369072 - part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window

nsSelectionCommandsBase is used with focused window because
nsXBLWindowKeyHandler for focused window uses it via nsXBLPrototypeHandler.
If the window is invisible, its PresShell is nullptr.  Then, commands which are
implemented by sub classes of nsSelectionCommandsBase are not available.
However, this too bad if user wants to do something with keyboard.

Therefore, nsSelectionCommandsBase::GetPresShellFromWindow() should work with
visible ancestor window's PresShell even if hidden window has focus.

Although, these commands are not available with document.execCommand, we should
check if the caller is not content for avoiding this hack to work unexpectedly.

Review commit: https://reviewboard.mozilla.org/r/176710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176710/

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8904911 [details]
Bug 1369072 - part0: Add automated test for testing key event handlers to scroll parent document when an iframe element has or had focus

https://reviewboard.mozilla.org/r/176704/#review181724

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8904912 [details]
Bug 1369072 - part1: PresShell should climb up scrollable frames when there is no selection, no focused content and root scrollable frame isn't scrollable

https://reviewboard.mozilla.org/r/176706/#review181730

This stuff is scary to change.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8904913 [details]
Bug 1369072 - part2: PresShell::HandleEvent() should retarget KeyboardEvent if focused document is invisible

https://reviewboard.mozilla.org/r/176708/#review181736

Super scary. I expect some regressions from this.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8904914 [details]
Bug 1369072 - part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window

https://reviewboard.mozilla.org/r/176710/#review181740

We should try to avoid adding more IsCallerChrome calls, I think.
Could we make document.execCommand take CallerType and then that information would be used here. Internal callers would default to system type caller.

::: dom/base/nsGlobalWindowCommands.cpp:230
(Diff revision 1)
> +
> +  // If the window is invisible, we should use its ancestor selection
> +  // controller.  However, don't do that if the caller is content script
> +  // because it might cause some security issues.  For safety, we should
> +  // check it here.
> +  if (NS_WARN_IF(!nsContentUtils::IsCallerChrome())) {

What guarantees this is called with JS on stack?
IsCallerChrome ends up calling this http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/dom/base/nsContentUtils.cpp#3242

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8904913 [details]
Bug 1369072 - part2: PresShell::HandleEvent() should retarget KeyboardEvent if focused document is invisible

https://reviewboard.mozilla.org/r/176708/#review181736

Yeah, but I believe that current behavior is obviously wrong since discarding keyboard event if focused document is invisible, any keyboard operation is not available. This is critical issue for users who use keyboard navigation mainly.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8904914 [details]
Bug 1369072 - part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window

https://reviewboard.mozilla.org/r/176710/#review181740

Hmm, I have no idea how to pass such information to command implementations without big changes. On the other hand, we need an only thing if it comes from nsXBLWindowKeyHandler.

> What guarantees this is called with JS on stack?
> IsCallerChrome ends up calling this http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/dom/base/nsContentUtils.cpp#3242

Hmm, I keep investigating what is the best for now.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc8819d1e25ae4287ab2c4ee7fdfe38eca1ab0bd

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8904911 [details]
Bug 1369072 - part0: Add automated test for testing key event handlers to scroll parent document when an iframe element has or had focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176704/diff/1-2/

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8904912 [details]
Bug 1369072 - part1: PresShell should climb up scrollable frames when there is no selection, no focused content and root scrollable frame isn't scrollable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176706/diff/1-2/

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8904913 [details]
Bug 1369072 - part2: PresShell::HandleEvent() should retarget KeyboardEvent if focused document is invisible

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176708/diff/1-2/

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8904914 [details]
Bug 1369072 - part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176710/diff/1-2/

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

According to my investigation, there is one nsXBLWindowKeyHandler in a top level window. It listens to all keyboard events for any child DOM windows and looks for an nsXBLProtoTypeHandler from the key combination.  Then, nsXBLProtoTypeHandler gets a controller with focused window via nsWindowRoot. That's the reason why controller for invisible window is retrieved. The new patch tries to fix the behavior of nsWindowRoot.  However, nsWindowRoot depends on nsFocusManager::GetFocusedDescendant().  Therefore, new patch needs to change nsFocusManager::GetFocusedDescendant() and a lot of its callers.

FYI: As far as I tested, editors in hidden window does NOT start handling keyboard events even with the patches. So, I think that there is no problem with current our behavior.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(I'm starting to wonder if the changes in this bug are safe enough to land this late in FF57)

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is not a recent regression. So, putting off to 58 is okay.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8904914 [details]
Bug 1369072 - part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window

https://reviewboard.mozilla.org/r/176710/#review182706

Maybe this isn't so scary after all, but better to watch for regressions and probably back out if there are any and then try to reland early in FF58

::: dom/base/nsFocusManager.cpp:327
(Diff revision 2)
> +    }
> +
> +    if (aSearchRange == eIncludeAllDescendants) {
> +      continue;
> +    }
> +

I think it would help with code readability if you asserted here that aSearchRange  is eIncludeVisibleDescendants

::: dom/base/nsFocusManager.cpp:335
(Diff revision 2)
> +    nsIDocShell* docShell = window->GetDocShell();
> +    if (!docShell) {
> +      break;
> +    }
> +    nsIPresShell* presShell = docShell->GetPresShell();
> +    if (!presShell) {

So this is really about whether the docshell has presshell. Have you thought how we should handle cases like
<iframe style="visibility:hidden">
I think we create the presshell inside iframe in that case.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8904914 [details]
Bug 1369072 - part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window

https://reviewboard.mozilla.org/r/176710/#review182706

> I think it would help with code readability if you asserted here that aSearchRange  is eIncludeVisibleDescendants

Okay.

> So this is really about whether the docshell has presshell. Have you thought how we should handle cases like
> <iframe style="visibility:hidden">
> I think we create the presshell inside iframe in that case.

Hmm, yes, this doesn't support visibility:hidden; case. On Google Chrome, I can scroll parent document when the focused sub window is hidden with visibility:hidden;. I think that I should file a follow up bug.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8904914 [details]
Bug 1369072 - part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176710/diff/2-3/

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e507cfef60d
part0: Add automated test for testing key event handlers to scroll parent document when an iframe element has or had focus r=smaug
https://hg.mozilla.org/integration/autoland/rev/19f8a5d3da39
part1: PresShell should climb up scrollable frames when there is no selection, no focused content and root scrollable frame isn't scrollable r=smaug
https://hg.mozilla.org/integration/autoland/rev/1c3ac6cb53ea
part2: PresShell::HandleEvent() should retarget KeyboardEvent if focused document is invisible r=smaug
https://hg.mozilla.org/integration/autoland/rev/ef1641e40903
part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window r=smaug

Comment 37

[Neil Deakin]

Since this issue of elements not losing focus when hidden has existed forever (this is bug 570835), I think we should instead strive to fix this the right way rather than applying regression-prone workarounds. It probably would be fairly simple to handle the case of an iframe being hidden at least (by calling nsFocusManager::WindowHidden at the right point) without having to fix all of bug 570835.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Neil Deakin from comment #37)
> Since this issue of elements not losing focus when hidden has existed
> forever (this is bug 570835), I think we should instead strive to fix this
> the right way rather than applying regression-prone workarounds. It probably
> would be fairly simple to handle the case of an iframe being hidden at least
> (by calling nsFocusManager::WindowHidden at the right point) without having
> to fix all of bug 570835.

Hmm, however, when focused element is being hidden, the hidden element still have focus even on Chrome ("blur" event won't be fired in the focused element in <iframe> nor the <iframe> itself. So, if we fix bug 570835, we may meet some web-compat issues with Chrome.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e507cfef60d
https://hg.mozilla.org/mozilla-central/rev/19f8a5d3da39
https://hg.mozilla.org/mozilla-central/rev/1c3ac6cb53ea
https://hg.mozilla.org/mozilla-central/rev/ef1641e40903