1348828 - Make sure the search engine table ignores scroll events if not scrollable

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

In bug 1335907, the search engine table is being put into the General preferences pane, which is the default. If the user scrolls the page (since it's now quite a big longer, as we've consolidated a number of preference panes), and the mouse cursor ends up over the search engine table, the table will absorb the scroll events and the page will stop scrolling.

STR:

1) Once bug 1335907 has landed, go to about:preferences
2) Ensure you're on the General pane
3) Move your mouse towards the middle of the page, and begin scrolling down with the scroll wheel

ER:

When the page scrolls up and the mouse cursor intersects the search engine table, if the search engine table is itself not scrollable, we should continue scrolling the page.

AR:

The search engine table, regardless of whether or not it's scrollable, will absorb the scroll events and the page will stop scrolling.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Gijs, what do you think we can do here? Adding a capturing "scroll" event listener to the xul:tree (#engineList) and using event.preventDefault() or event.stopPropagation() doesn't work (as you may expect). I wonder if we need to fix something deeper in the xul:tree implementation.

We don't have this problem with HTML overflow:scroll items when there is no actual overflow, for example try the following test case:
data:text/html,<div style="overflow:scroll; height: 200px; width: 200px;"><p>h<p>h<p>h<p>h</div><p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h<p>h

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1348828 - Only prevent default behavior of scroll event if the tree is scrollable.

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

Comment 5

[Tomer Cohen :tomer]

As I've mentioned on bug 1353056, I suggest eliminating the listbox, so there won't be multiple scrollbars on the same page and instead have the list dynamically expand as needed. (i.e., as with a simple HTML list items)

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8854048 [details]
Bug 1348828 - Only prevent default behavior of scroll event if the tree is scrollable.

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

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8854048 [details]
Bug 1348828 - Only prevent default behavior of scroll event if the tree is scrollable.

https://reviewboard.mozilla.org/r/126038/#review128622

::: toolkit/content/widgets/tree.xml:720
(Diff revision 2)
>        <handler event="touchend">
>          <![CDATA[
>            this._touchY = -1;
>          ]]>
>        </handler>
> -      <handler event="MozMousePixelScroll" preventdefault="true"/>
> +      <handler event="DOMMouseScroll">

I see this was all added as a way to prevent test failures because the page was scrolling when only the inner part of the tree was expected to scroll.

If hidevscroll is true then the table has no scrolling, and the test shouldn't have the goal of scrolling the page.

I'll push this patch to tryserver to make sure that this won't fail any tests.

I tested this patch on my desktop with a mouse and my laptop with two finger touch scrolling and the patch as-is fixes the bug on both machines. This surprised me since I didn't need to change the MozSwipeGesture or the touchmove events.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tomer Cohen :tomer from comment #5)
> As I've mentioned on bug 1353056, I suggest eliminating the listbox, so
> there won't be multiple scrollbars on the same page and instead have the
> list dynamically expand as needed. (i.e., as with a simple HTML list items)

We could do that, though it would be a much larger project and right now we don't have the resources×priority to create the same markup/design with a different approach.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 10

[:Gijs (he/him)]

Comment on attachment 8854048 [details]
Bug 1348828 - Only prevent default behavior of scroll event if the tree is scrollable.

https://reviewboard.mozilla.org/r/126038/#review128660

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3c2626db70138625de484bd099369483c88ac883

mochitest-chrome looks unhappy on Linux...

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1348828 - Only prevent default behavior of scroll event if the tree is scrollable.

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

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8854258 [details]
Bug 1348828 - Only prevent default behavior of scroll event if the tree is scrollable.

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

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 14

[Neil Deakin]

Comment on attachment 8854258 [details]
Bug 1348828 - Only prevent default behavior of scroll event if the tree is scrollable.

https://reviewboard.mozilla.org/r/126194/#review129500

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fbafcb56369d
Only prevent default behavior of scroll event if the tree is scrollable. r=enndeakin+6102

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbafcb56369d

Comment 17

[Itiel]

I can confirm this is fixed in latest Nightly, but something seems off.
If the search engine list is scrollable, scrolling in it scrolls *also* the whole settings page.

Is this intended?

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to ItielMaN from comment #17)
> I can confirm this is fixed in latest Nightly, but something seems off.
> If the search engine list is scrollable, scrolling in it scrolls *also* the
> whole settings page.
> 
> Is this intended?

No this was not intended. I filed bug 1355211 to fix this. Thank you for noticing this and saying something! :)

Comment 19

[Saheda Reza [:Antora]]

I have reproduced this bug on Firefox nightly according to (2017-03-20)

Fixing bug is verified on Latest Nightly-- Build ID: (20170411030208), User Agent: Mozilla/5.0 (Windows NT 10.0; rv:55.0) Gecko/20100101 Firefox/55.0

Tested OS-- Windows10 32bit

[bugday-20170405]

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Thanks!

Comment 21

[Itiel]

This issue is reproducible again on latest Nightly. I can't scroll down (and more importantly, up) once the cursor is on the engine list. Can someone confirm?