Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Description • 10 years ago

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?

	Hernán Rodriguez Colmeiro (:peregrino)
	Comment 1 • 10 years ago

Henrik, on what platform are you finding this behavior? I can't reproduce on latest nightly (32.0a1 (2014-05-27)) on OSX.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 2 • 10 years ago

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.

	Hernán Rodriguez Colmeiro (:peregrino)
	Comment 3 • 10 years ago

Ok, I can reproduce what Henrik says on Linux, but not on OSX nor Windows. Marking this as Linux Only.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 4 • 10 years ago

Do we need to support the "scroll" event on the menu? I think we could preventDefault on it.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 5 • 10 years ago

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?

	Richard Marti (:Paenglab)
	Comment 6 • 10 years ago

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

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 7 • 10 years ago

(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);

	Richard Marti (:Paenglab)
	Comment 8 • 10 years ago

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?

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 9 • 10 years ago

Richard, this fixes it for me. What do you think about this patch?

	Richard Marti (:Paenglab)
	Comment 10 • 10 years ago

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.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 11 • 10 years ago

Yeah, good catch.

	:Gijs (he/him)
	Comment 12 • 10 years ago

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?

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 13 • 10 years ago

(In reply to :Gijs Kruitbosch from comment #12)
> closures of this kind

What variable is being closed over here?

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 14 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/26ec6656f074

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/26ec6656f074

	:Gijs (he/him)
	Comment 16 • 10 years ago

(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. :-)