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

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Preferences
VERIFIED FIXED
5 months ago
4 days ago

People

(Reporter: mconley, Assigned: jaws)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 months ago
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.
(Reporter)

Updated

5 months ago
Summary: Make sure the search engine ignores scroll events if not scrollable → Make sure the search engine table ignores scroll events if not scrollable
(Reporter)

Updated

5 months ago
Depends on: 1335907
(Assignee)

Updated

5 months ago
Blocks: 1335907
No longer depends on: 1335907
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1352956
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
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

5 months ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

5 months ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1353056

Comment 5

5 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 7

5 months ago
mozreview-review
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.
(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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c2626db70138625de484bd099369483c88ac883

Comment 10

5 months ago
mozreview-review
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...
Attachment #8854048 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8854048 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8854258 - Flags: review?(enndeakin)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a98bc0685f0e
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
Attachment #8854258 - Flags: review?(enndeakin) → review+

Comment 15

5 months ago
Pushed by jwein@mozilla.com:
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fbafcb56369d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 17

5 months ago
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?
Flags: needinfo?(jaws)
(Assignee)

Updated

5 months ago
Depends on: 1355211
(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! :)
Flags: needinfo?(jaws)
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]
Thanks!
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified

Updated

4 months ago
See Also: → bug 1356398

Comment 21

4 days ago
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?
You need to log in before you can comment on or make changes to this bug.