Closed Bug 1369072 Opened 7 years ago Closed 7 years ago

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

Categories

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

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: david+bugs, Assigned: masayuki)

References

Details

Attachments

(4 files)

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.
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?
Component: Theme → Event Handling
Flags: needinfo?(masayuki)
Product: Firefox → Core
Summary: Focus caught by non-displayed element causes keyboard shortcuts to fail → Focus caught by non-displayed element causes keyboard shortcuts to fail on youtube
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.
Flags: needinfo?(masayuki)
Priority: -- → P2
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
(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
(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.
(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.
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
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.
(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.
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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
Attachment #8904911 - Flags: review?(bugs) → review+
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.
Attachment #8904912 - Flags: review?(bugs) → review+
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.
Attachment #8904913 - Flags: review?(bugs) → review+
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
Attachment #8904914 - Flags: review?(bugs) → review-
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 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.
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.
(I'm starting to wonder if the changes in this bug are safe enough to land this late in FF57)
This is not a recent regression. So, putting off to 58 is okay.
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.
Attachment #8904914 - Flags: review?(bugs) → review+
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.
Pushed by masayuki@d-toybox.com:
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
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.
(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.
Depends on: 1418442
Depends on: 1482425
Depends on: 1499430
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.