Closed Bug 1008172 Opened 8 years ago Closed 7 years ago

Scrolling up and down on pages with scrollbars in about:preferences will change subgroups (the Advanced subpanes)

Categories

(Firefox :: Preferences, defect)

31 Branch
All
Linux
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.2

People

(Reporter: whimboo, Assigned: jaws, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 1 obsolete file)

It's an annoying behavior for scrolling. When you are on the e.g. advanced page and you scroll up and down with the mouse, while having its pointer located near the top sub page area, the sub group will change accidentally. So the setting you wanted to reach are not available anymore, and you have to reselect the wanted sub page.

We should fine a way to stop this behavior. Do we really have to handle the mouse event here for scrolling?
Henrik, on what platform are you finding this behavior? I can't reproduce on latest nightly (32.0a1 (2014-05-27)) on OSX.
I'm on Linux (Ubuntu) and it's still visible. I have an external mouse (MS optical mouse), and using the scroll wheel. Looks like that this is necessary to reproduce the problem.
Ok, I can reproduce what Henrik says on Linux, but not on OSX nor Windows. Marking this as Linux Only.
OS: All → Linux
Flags: firefox-backlog+
Do we need to support the "scroll" event on the menu? I think we could preventDefault on it.
Points: --- → 2
Mentor: jaws
Whiteboard: [good first bug][lang=js]
Richard, can you try adding the following line to preferences.js,

>  categories.addEventListener("scroll", event => event.preventDefault());

below the `categories.addEventListener("select", event => gotoPref(event.target.value));` line?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Flags: needinfo?(richard.marti)
Hmm, this doesn't work. I also tried

categories.addEventListener("DOMMouseScroll", event => event.preventDefault());

But also this doesn't work.

The special tab scrolling on Linux is IMHO coming from here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tabbox.xml#581
Flags: needinfo?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #6)
> Hmm, this doesn't work. I also tried
> 
> categories.addEventListener("DOMMouseScroll", event =>
> event.preventDefault());
> 
> But also this doesn't work.
> 
> The special tab scrolling on Linux is IMHO coming from here:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tabbox.
> xml#581

Can you try adding a third argument of `true` to the addEventListener call, so that we receive the event during the capturing phase? For example,
> categories.addEventListener("DOMMouseScroll", event => event.preventDefault(), true);
Flags: needinfo?(richard.marti)
This still doesn't work. Then I tried, because the tabs aren't in categories, to apply  to #tabsElement, but still negative:

  let tabs = document.getElementById("tabsElement");
  tabs.addEventListener("DOMMouseScroll", event => event.preventDefault(), true);

Applying it to #mainPrefPane or #advancedPrefs stopped the mouse scrolling when the mouse is over them. Maybe it would work, when it's applied to the tabs instead the tabsElement.

What can I use instead of getElementById() to get the tabs?
Flags: needinfo?(richard.marti)
Summary: Scrolling up and down on pages with scrollbars in about:preferences will change subgroups → Scrolling up and down on pages with scrollbars in about:preferences will change subgroups (the Advanced subpanes)
Attached patch Patch (obsolete) — Splinter Review
Richard, this fixes it for me. What do you think about this patch?
Attachment #8535163 - Flags: feedback?(richard.marti)
Comment on attachment 8535163 [details] [diff] [review]
Patch

For me it works too.

Only one question, shouldn't the ifdef be above the comment? Like it is now the other platforms would have a comment for not existent code.
Attachment #8535163 - Flags: feedback?(richard.marti) → feedback+
Attached patch Patch v1.1Splinter Review
Yeah, good catch.
Assignee: richard.marti → jaws
Attachment #8535163 - Attachment is obsolete: true
Attachment #8535184 - Flags: review?(gijskruitbosch+bugs)
Iteration: --- → 37.2
Flags: qe-verify?
Comment on attachment 8535184 [details] [diff] [review]
Patch v1.1

Review of attachment 8535184 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/advanced.js
@@ +83,5 @@
> +#ifdef MOZ_WIDGET_GTK
> +    // GTK tabbox' allow the scroll wheel to change the selected tab,
> +    // but we don't want this behavior for the in-content preferences.
> +    let tabsElement = document.getElementById("tabsElement");
> +    tabsElement.addEventListener("DOMMouseScroll", event => {

I wonder if we need to remove this at some point to avoid leaks. We didn't do that when we converted from attributes to event listeners (see above), but those by and large don't use closures of this kind, which makes me a lot less nervous. Do we have tests that will hit this code and scream if we're leaking?
Attachment #8535184 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #12)
> closures of this kind

What variable is being closed over here?
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/26ec6656f074
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > closures of this kind
> 
> What variable is being closed over here?

the arrow function implicitly holds a ref to |this| (in that it's mostly the same as (function() {}).bind(this) ) which creates a circular reference pattern between the document and its nodes on the one hand, and the event listener on the other.

Seeing as this landed and nothing went orange, presumably the cycle collector and/or the document's destruction when the tab is closed doesn't leak. :-)
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.