1389368 - Twitter pages lose focus and do not respond to keyboard inputs

Status: NEW

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

Description

[Greg]

Attachment: Screenshot.png

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36

Steps to reproduce:

Go to a twitter page when you are not logged in to Twitter: https://twitter.com/micahflee.

Notice if a login box appears in the upper right corner that begins "Have an account?" (see attached screenshot for visual of login box). If login box is present, the cursor is blinking in the login box preventing the keyboard button "PgDn" from scrolling the twitter page.


Actual results:

I want to be able to scroll the page, so I press Esc to remove the login box. Now I press PgDn, but the page will still not scroll even though the login box is no longer present. That is, focus has not been given to the page.


Expected results:

Like happens on Google Chrome, pressing Esc should remove the login box and deliver focus to the twitter page so that pressing PgDn scrolls the page. On Firefox, pressing Esc makes the login box disappear but it does not seem to deliver focus to the page so that cursor keys and PgUp/PgDn keys will start working.

Comment 1

[Alice0775 White]

I can also reproduce the problem on Windows10 Firefox55.0.1, ESR52.3.0 and Edge40.15063.0.0, but not on Chrome60.0.3112.90.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Maybe Ben or Stone could take a look at this after they finish P1s on their plate ...

Comment 3

[Ben Tian (inactive)]

Pressing ESC intends to focus on body element but it's non-focusable, so focus remains on login box (input element).

[1] checks whether frame of body element is focusable and enters [2], where |aIsFocusable| is false for (tabIndex, disabled, HasAttr(tabindex)) as (-1, 0, 0).

[1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/base/nsFocusManager.cpp#1621
[2] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/html/nsGenericHTMLElement.cpp#2587

Comment 4

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #3)
> Pressing ESC intends to focus on body element but it's non-focusable, so
> focus remains on login box (input element).

Elaboration: Key events still go to body element (e.g., TAB) as [3] corrects event target, but key navigation (page up/down, up/down/left/right) is incorrect.

[3] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/base/PresShell.cpp#7773

Comment 5

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #4)
> Elaboration: Key events still go to body element (e.g., TAB) as [3] corrects
> event target, but key navigation (page up/down, up/down/left/right) is
> incorrect.

Focus of key navigation (APZCTreeManager.mFocusState [1]) depends on content focus target.
[1] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/gfx/layers/apz/src/FocusState.h#20

===

We should make body element focusable to correct key navigation. Two possible ways:
1) Add method |HTMLBodyElement::IsHTMLFocusable| instead of using inherited |nsGenericHTMLElement::IsHTMLFocusable|
2) Replace |doc->GetRootElement| with |doc->GetUnfocusedKeyEventTarget| in |nsFocusManager::CheckIfFocusable| [2]

[2] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/dom/base/nsFocusManager.cpp#1585

Comment 6

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #5)
> We should make body element focusable to correct key navigation. Two
> possible ways:
> 1) Add method |HTMLBodyElement::IsHTMLFocusable| instead of using inherited
> |nsGenericHTMLElement::IsHTMLFocusable|

Approach 1) affects body element only, while HTML document's GetUnfocusedKeyEventTarget may return body or frameset element.

> 2) Replace |doc->GetRootElement| with |doc->GetUnfocusedKeyEventTarget| in
> |nsFocusManager::CheckIfFocusable| [2]

Approach 2) would block root element of non-HTML document. An alternative is |doc->GetRootElement OR doc->GetUnfocusedKeyEventTarget|, so the former condition always allows root element and the later for body element of HTML document.

Comment 7

[Ben Tian (inactive)]

Making body element focusable fails reftest [1] for [2] finds existing focused node (i.e., body) and doesn't focus on element with attribute 'autofocus' [3].

[1] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/dom/html/reftests/autofocus/autofocus-after-body-focus.html
[2] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/dom/html/nsGenericHTMLElement.cpp#147
[3] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/dom/html/nsGenericHTMLElement.cpp#160

Comment 8

[Ben Tian (inactive)]

The input element is in <div> with visibility controlled by 'aria-hidden'. When user presses escape, the input element's primary frame would become null. So an alternative is to still blur focused content if its primary frame is null before returning in [1], while keeping body element non-focusable for autofocus test case in comment 7. Need further consideration on correct behavior.

[1] http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/dom/base/nsFocusManager.cpp#1207

Comment 9

[Ben Tian (inactive)]

Attachment: WIP patch

WIP patch with try. Need further refinement on condition to trigger.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d647fbe8dd52522cb627ae59fa4170237ae366a2

Comment 10

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): When new content to focus is not focusable, still clear focused element if it has no frame

Comment 11

[Ben Tian (inactive)]

Comment on attachment 8909690 [details] [diff] [review]
Patch 1 (v1): When new content to focus is not focusable, still clear focused element if it has no frame

Stone, can you feedback my patch per our prior offline discussion?

The patch clears frameless focused element when new content to focus is not focusable.

Comment 12

[Ming-Chou Shih [:stone]]

Comment on attachment 8909690 [details] [diff] [review]
Patch 1 (v1): When new content to focus is not focusable, still clear focused element if it has no frame

Sorry for replying late.

I'm thinking the possibility to clear focus when the element is hidden. With the patch, the blur event of the hidden element is fired when someone changes focus.

How do you think?

Comment 13

[Ming-Chou Shih [:stone]]

(In reply to Ming-Chou Shih [:stone] from comment #12)
> Comment on attachment 8909690 [details] [diff] [review]
> Patch 1 (v1): When new content to focus is not focusable, still clear
> focused element if it has no frame
> 
> Sorry for replying late.
> 
> I'm thinking the possibility to clear focus when the element is hidden. With
> the patch, the blur event of the hidden element is fired when someone
> changes focus.
> 
> How do you think?

Did some tests and found that
1. Chrome sets activeElement to null and fires blur when the content is removed in the focus event listener.
   Firefox sets activeElement to null but doesn't fire blur event.

2. Chrome sets activeElement to null and fires blur when the content is hidden in the focus event listener.
   Firefox doesn't set activeElement to null.

Comment 14

[Ming-Chou Shih [:stone]]

(In reply to Ming-Chou Shih [:stone] from comment #12)
> Comment on attachment 8909690 [details] [diff] [review]
> Patch 1 (v1): When new content to focus is not focusable, still clear
> focused element if it has no frame
> 
> Sorry for replying late.
> 
> I'm thinking the possibility to clear focus when the element is hidden.
Maybe we could check whether focus element is current element in nsGenericHTMLElement::AfterSetAttr.

http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/html/nsGenericHTMLElement.cpp#700