If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Typing a space in an input field causes page-down scrolling with apz.keyboard.enabled=true

VERIFIED FIXED in Firefox 56

Status

()

Core
Panning and Zooming
P3
normal
VERIFIED FIXED
3 months ago
14 days ago

People

(Reporter: kats, Assigned: rhunt)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Created attachment 8884421 [details]
Test page

Nightly on OS X. Turn on apz.keyboard.enabled, restart browser. Open the attached page and type "hello world" in the input field. Note that when you hit the spacebar it will scroll down instead of inserting a space.
(Reporter)

Updated

3 months ago
Attachment #8884421 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 1

3 months ago
I noticed this when I was trying to type a Bugzilla comment, it's annoying.

Something is going wrong in focus target creation, we must not be noticing a textarea is editable.
Assignee: nobody → rhunt
Whiteboard: [gfx-noted]
(Assignee)

Comment 2

2 months ago
Going to do a write up of what I know so far. I haven't gotten a great solution yet.

This problem is related to our check for key event listeners and is general.

When we create a focus target, we check for key event listeners that accept untrusted events and are not in the system group. These event listeners are considered to be web content and so they disable async key scrolling.

The problem with this, is that there are a lot of elements that use key event listeners that are not web content but should still prevent async key scrolling

1. <input>
2. <textarea> [1]
3. <video>
4. <audo> [2]
5. <button> [3]

Some of the event listeners come from XBL, others from libeditor. With any of these items you can reproduce bad things by putting the focus on them and then hitting space. With apz-key enabled the space will dispatch a scroll instead of activating the element.

I'm not sure of a good solution for this yet. There isn't a event listener flag that we can use to clear this up, and I'm not sure if adding a new one would be overkill for this problem.

A potential solution could be to check for _any_ key event listener on elements that have an XBL binding. All the problematic elements have XBL bindings so this would disable apz key scrolling when they have focus.

It's kind of cheating because while <input> and <textarea> have XBL bindings the event listeners come from elsewhere. So it's kind of a coincedence that this would work and could break.

Another problem with that approach is that <xul:browser> has an XBL binding and a system key listener. This key listener is used for toggling browse-with-caret and can be ignored, but would fall under that classification and disable apz key scrolling. This event listener could maybe be moved, or the passive flag could potentially be repurposed for this?

[1] http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/editor/libeditor/EditorEventListener.cpp#144
[2] http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/toolkit/content/widgets/videocontrols.xml#1321
[3] http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/toolkit/content/widgets/button.xml#146
Smaug might have some ideas about comment 2.
One possible option is to add a new hidden method "mozDisableApzKeyScroll()" on the Element interface, and require that any chrome code that's adding a key listener that interferes with scrolling call that function. That way we can set a flag on the element that indicates if it has focus we should not allow APZ keyboard scrolling on it. This is basically equivalent to adding a new event listener flag but possibly a little cleaner?
(Assignee)

Comment 5

2 months ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> One possible option is to add a new hidden method "mozDisableApzKeyScroll()"
> on the Element interface, and require that any chrome code that's adding a
> key listener that interferes with scrolling call that function. That way we
> can set a flag on the element that indicates if it has focus we should not
> allow APZ keyboard scrolling on it. This is basically equivalent to adding a
> new event listener flag but possibly a little cleaner?

This seems reasonable to me. There would be a bit of auditing required to make sure everything is marked, but my guess is it's doable.

Comment 6

2 months ago
Where would you store the information about mozDisableApzKeyScroll?
I'm not sure we have any spare bits on Element.
I don't think we need ELEMENT_HAS_SCROLLGRAB anymore now that B2G is dead. AFAICT there's no code that sets that flag on Element. We can get rid of that and use the bit for this instead. (We might want to keep the getter/setters at [1] and just empty their implementations to be no-ops, in case we decided to actually standardize that feature properly later).

[1] http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/dom/html/nsGenericHTMLElement.h#183,187
(Reporter)

Updated

2 months ago
Depends on: 1380375
(Assignee)

Comment 8

2 months ago
More thoughts on this...

I think a key event listener should disable key scrolling when any of the following things are true:

1. It listens for a key that could trigger a scroll
      - without this we might perform a scroll when the event listener should run
2. It may preventDefault the event
      - without this we might scroll when we shouldn't
3. It may change the focus or selection of the page
      - without this we might scroll when we need to do an entirely different action (like input text or click a button)

For web content we have no way of knowing whether any of that is true for a key event listener so we need to be conservative and disable apz key scrolling for this event.

Unfortunately, chrome uses key event listeners extensively so we need to apply the above test on each key listener.

In my opinion, for maintainability, it would be nice to be pessimistic about chrome key event listeners and assume they should block async key scrolling, unless marked otherwhise. With this we could also add a test to insure that no one adds a key listener that disables async key scrolling.

Here is a list of all of the sources of chrome key listeners that block async key scrolling:

1. XBLWindowKeyHandler
    - Installed in nsGlobalWindow and safe to ignore because keyboard apz works around it
2. <xul:keyset>
    - Used in chrome, browser.xul includes browser_sets.inc with a list of them
3. <xul:handler>
    - Used in various XBL bindings like browser.xml, tabbrowser.xml, tabbox.xml
4. Native code using AddEventListener
    - ChromeTooltipListener
    - nsMenuBarListener
    - nsFormFillController
5. Javascript using AddSystemEventListener
    - browser_content.js (for implementing the FindBar)
    - tabbox.xml
    - tabbrowser.xml

If these are all marked as safe, we could do a check on each event target for any key event listener not marked, and that would work.

For marking, it seems like a new listener flag and a new XBL attribute would suffice for all the ones listed above.

The alternative to being pessimistic, is assuming that every chrome key listener is safe unless marked otherwise. We could do this using kats suggestion. Smaug also suggested adding a new virtual method on Element for CanAPZKeyScroll, and then implementing it for all the problematic elements.

The disadvantage here is that it seems very easy to break with the addition of a new key event listener that forgets to be marked by an element as unsafe.
(Assignee)

Comment 9

2 months ago
I forgot to mention, the other alternative to all of this is to only enable async key scrolling for the root scrollable. That would work around this issue.

Full async key scrolling could be put behind a preference until this is worked out.
(In reply to Ryan Hunt [:rhunt] from comment #9)
> I forgot to mention, the other alternative to all of this is to only enable
> async key scrolling for the root scrollable. That would work around this
> issue.

This seems fine to me, for now. That way we get it turned on in Nightly while we figure out what to do about subframe scrolling.
(Reporter)

Updated

2 months ago
Priority: -- → P3
(Assignee)

Comment 11

2 months ago
So a new idea.

When we do any keyboard scrolling the content that is used to locate a scroll frame is either:
(1) the focused element, or
(2) the focus node of the DOM selection, if there is no focused element

If we only support async keyboard scrolling with (2) then I don't think we have to worry about <input>,<textarea>, etc, because those are always in focus when they are used. (2) is used in most cases and it works with scrollable subframes.

(1) is used essentially only for interactive elements. The only time it may return a scrollable frame is if a user tabs to select a scrollable div.

So I wrote a patch to only use (2) for async keyboard scrolling. If there is a focused element on the page, we disable async keyboard scrolling. This seems to work quite well. I can async scroll in all the cases I could before, and also type in textarea's without a problem.

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

If the try run is green, I'll upload for review.
(Assignee)

Comment 12

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b00afb68d482e6aae752da7573690a369df17f60

I was pretty bewildered by try for a while, because of the row of orange in Windows10 x64 Debug. But it looks like those tests are timeout related and/or not run outside of try.

So I'm guessing this try run is green.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 months ago
mozreview-review
Comment on attachment 8887332 [details]
Bug 1379280 - Move FocusTarget include from nsIPresShell to PresShell.

https://reviewboard.mozilla.org/r/158160/#review163532
Attachment #8887332 - Flags: review?(bugs) → review+

Comment 16

2 months ago
mozreview-review
Comment on attachment 8887333 [details]
Bug 1379280 - Only do async keyboard scrolling for a selection, not a focused element.

https://reviewboard.mozilla.org/r/158162/#review163540

Those fixed, r+

::: dom/base/nsIDocument.h:945
(Diff revision 1)
>  
>    /**
> +   * Gets the event target to dispatch key events to if there is no focused
> +   * content in the document.
> +   */
> +  virtual nsIContent* GetUnfocusedKeyEventTarget();

Could you make nsIDocument::GetActiveElement() to use this. Less code duplication.

::: dom/html/nsHTMLDocument.cpp:892
(Diff revision 1)
> +nsHTMLDocument::GetUnfocusedKeyEventTarget()
> +{
> +  if (nsGenericHTMLElement* body = GetBody()) {
> +    return body;
> +  }
> +  return GetRootElement();

Perhaps call return nsDocument::GetUnfocusedKeyEventTarget() here.

::: gfx/layers/apz/src/FocusTarget.cpp:168
(Diff revision 1)
>  
>      mType = FocusTarget::eNone;
>      return;
>    }
>  
> +  nsCOMPtr<nsIContent> selectedContent = presShell->GetSelectedContentForScrolling();

This is confusing. We GetFocusedContentForScrolling and GetSelectedContentForScrolling. What is the difference of those?
If GetFocusedContentForScrolling returns non-null, we set mType to eNone and return.
And we never check the event listeners of GetSelectedContentForScrolling.

I think it would be better to rename GetFocusedContentForScrolling to something else. Perhaps GetFocusedContent, since that is what it is returning.
Attachment #8887333 - Flags: review?(bugs) → review+
(Assignee)

Comment 17

2 months ago
(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8887333 [details]
> Bug 1379280 - Only do async keyboard scrolling for a selection, not a
> focused element.
> 
> https://reviewboard.mozilla.org/r/158162/#review163540
> 
> Those fixed, r+
> 
> ::: dom/base/nsIDocument.h:945
> (Diff revision 1)
> >  
> >    /**
> > +   * Gets the event target to dispatch key events to if there is no focused
> > +   * content in the document.
> > +   */
> > +  virtual nsIContent* GetUnfocusedKeyEventTarget();
> 
> Could you make nsIDocument::GetActiveElement() to use this. Less code
> duplication.
> 

GetUnfocusedKeyEventTarget() in HTML documents will return the root element if the body is null, and which goes against this comment [1]. So I don't know if we can reuse this here.

> ::: dom/html/nsHTMLDocument.cpp:892
> (Diff revision 1)
> > +nsHTMLDocument::GetUnfocusedKeyEventTarget()
> > +{
> > +  if (nsGenericHTMLElement* body = GetBody()) {
> > +    return body;
> > +  }
> > +  return GetRootElement();
> 
> Perhaps call return nsDocument::GetUnfocusedKeyEventTarget() here.
> 

Done.

> ::: gfx/layers/apz/src/FocusTarget.cpp:168
> (Diff revision 1)
> >  
> >      mType = FocusTarget::eNone;
> >      return;
> >    }
> >  
> > +  nsCOMPtr<nsIContent> selectedContent = presShell->GetSelectedContentForScrolling();
> 
> This is confusing. We GetFocusedContentForScrolling and
> GetSelectedContentForScrolling. What is the difference of those?
> If GetFocusedContentForScrolling returns non-null, we set mType to eNone and
> return.
> And we never check the event listeners of GetSelectedContentForScrolling.
> 

We use the focused element only to see if a remote browser is focused and there are any key listeners.

Once we determine those are not the case, if there is a focused element we disable async key scrolling. This is to prevent the issues discussed above. We then need to find if we have a DOM selection to base key scrolling off of.

We never check the event listeners of the DOM selection because only event listeners of the focused content are run.

> I think it would be better to rename GetFocusedContentForScrolling to
> something else. Perhaps GetFocusedContent, since that is what it is
> returning.

I agree, I'll change that.
(Assignee)

Comment 18

2 months ago
Whoops, forgot to link [1]

[1] http://searchfox.org/mozilla-central/rev/88180977d79654af025158d8ebeb8c2aa11940eb/dom/base/nsDocument.cpp#3397

Comment 19

2 months ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0d1fdf2d20
Move FocusTarget include from nsIPresShell to PresShell. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/24dc21609404
Only do async keyboard scrolling for a selection, not a focused element. r=smaug

Comment 20

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b0d1fdf2d20
https://hg.mozilla.org/mozilla-central/rev/24dc21609404
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Verified as fixed using 57.0a1 2017-09-07 Nightly and 56.0beta 9 under Win 10 64-bit, Mac OS X 10.11 and Ubuntu 14.04 64-bit.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.