1217715 - [Win] Mouse wheel scroll speed should be accelerated by the system scroll speed override on any large scrollable elements not only on the root element of the root document

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[gordon]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.71 Safari/537.36

Steps to reproduce:

Create a page with an iframe that loads in overflowing content.  Scroll the iframe with the mousewheel


Actual results:

The iframe root scrolls very slowly.




Expected results:

The iframe root should scroll the same speed as the parent root element.

My understanding is that on root elements Firefox overrides the scroll step to be closer the Chrome.

mousewheel.system_scroll_override_on_root_content.enabled = true

However this doesn't appear to work for the root of any iframes in a page.  Surely this override should be applied to all root documents and not just the topmost frame?

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Can you please attach a minimal testcase that reproduces the issue so we're all for sure looking at the same thing?

Comment 2

[gordon]

Attachment: Testcase

This is a simple testcase to illustrate the problem.  The "blue" half of the page is content added directly to the "body" of the page.  The "red" half of the page on the left is the same content contained within an iframe.

When scrolling with the mousewheel, they scroll at different speeds.  In the iframe a single mousewheel "notch" or "pulse" scroll about 3 lines when scrolling on the "body" content, it moves about 6 lines.

The reason for this is that there is a mousewheel override attached to a pages root element.  If windows is set to the default of 3 lines per wheel "notch" then the override kicks in and makes the page move 6 lines, in order to be more consistent with other browsers, in particular Chrome which defaults to around 6 lines also.

However, this override does not kick in when you are scrolling iframes.  I understand that the decision was made to only provide the override on a documents root element, but I would argue that since an iframe in essence a complete document, that it too should receive the override when scrolling.

Both IE/Edge and Chrome scroll at the same speed for the iframe root and the parent root element.

Comment 3

[gordon]

Our app contains separate applications within an overall parent app.  These "inner" apps are sandboxed in to separate iframes in a manner very similar to the way in which Firefox OS manages it's applications.  Currently, in order to have a consistent experience with other browsers and between the parent/inner apps we have to override default scrolling behaviour within these iframes.

We have written a javascript implementation of Firefox's smoothscroll in order to make them match as much as possible but obviously, with APZ scrolling soon to be default, this situation is far from optimal.  We actually have to force the iframes to scrolling="no" in order to prevent APZ from kicking in or we get janky behaviour due to our javascript mousewheel scrolling.  Currently APZ + Javascript scrolling causes a significant janky effect.

Of course if we disable our javascript mousewheel scrolling then we gain all the wonderful benefits of APZ within our inner applications but scrolling suddenly becomes slow and completely out of sync with the parent application and all other browsers despiting being at a constant 60fps thanks to APZ.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Kats, is this something you'd be able to weigh in on?

Comment 5

[ovidiu boca[:Ovidiu]]

Hi, 
I can reproduce this problem on Win 10 Pro x64 with FF Nightly 44.0a1.(2015-10-26)

Steps to reproduce:

1 Go to Testcase from attachment 
2 [review] Scroll with the mouse the blue part and the the red part.

Expected results:

   Should scroll the same number of elements.

Actual results:

  In the red part when scroll once, it jumps 3 elements in one time and in the blue part when scroll once, it jumps 6 elements in one time.

Please see also bug 1217399.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Masayuki is probably a better person to comment on the override pref and if it should be applied to iframes as well.

If I'm understanding the situation properly though this should be an issue both with and without APZ in your app, if you disable your javascript scrolling. Is that correct?

Comment 7

[gordon]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)

> If I'm understanding the situation properly though this should be an issue
> both with and without APZ in your app, if you disable your javascript
> scrolling. Is that correct?

Indeed.  The difference in scrolling speed/distance and therefore this bug is the same regardless of APZ.  

It's just that the performance of our current hacky workaround for this behaviour in Firefox deteriorates with APZ.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Cool, thanks for clarifying. I myself have no objection to changing the behaviour of this pref but I have no idea why it was added and why it's only true on Windows. Leaving this for Masayuki to respond.

Comment 9

[gordon]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Cool, thanks for clarifying. I myself have no objection to changing the
> behaviour of this pref but I have no idea why it was added and why it's only
> true on Windows. Leaving this for Masayuki to respond.

I feel the same.  It seems odd to me to have any internally scrollable elements, let alone Iframes, scroll at a different speed to the root document.  Especially when you consider that no other browser, or any native application UI exhibits this behaviour.  I wonder if it was merely an oversight?  Or if there was indeed some kind of logic behind the decision.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The root reason is that Chrome makes user feel itself faster than other browsers with this trick. For the marketing reason, implementing similar cheat was suggested. However, somebody don't like their system settings are ignored, and also faster scroll on small scrollable area isn't useful. Therefore, the cheat is performed only on the root frame of the root document.

The reason why this is implemented only on Windows is, nobody was thinking that this is necessary for other platforms. On mac, their trackpad is enough useful without such cheats. On Linux, nobody suggested.

I know there could be "big" <iframe>s. I agree that we should enable the cheat on such big <iframe>s too. But the condition to enable it is very difficult. So, please suggest some good conditions which are safe, independent from magic number of pixels, etc.

Comment 11

[gordon]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> The root reason is that Chrome makes user feel itself faster than other
> browsers with this trick. For the marketing reason, implementing similar
> cheat was suggested. However, somebody don't like their system settings are
> ignored, and also faster scroll on small scrollable area isn't useful.
> Therefore, the cheat is performed only on the root frame of the root
> document.

I see.  I understand this reasoning, though I wonder if the logic behind it should be revisited now that Windows 10 has landed?  Scrolling small areas in modern universal apps is equally as fast as scrolling small ones.  Regardless, I agree that system settings other than default should definitely be honored. 

Although with that said, setting the step size to 6 in Windows 10 makes all native apps scroll far more quickly than Firefox.  I think mousewheel settings need to be re-compared with native Windows 10 apps.  Right now they don't feel in alignment, though that is a separate issue altogether.

> I know there could be "big" <iframe>s. I agree that we should enable the
> cheat on such big <iframe>s too. But the condition to enable it is very
> difficult. So, please suggest some good conditions which are safe,
> independent from magic number of pixels, etc.

In our case that is exactly the problem.  Our Iframe apps are nearly always the full height of the viewport making the difference extremely noticeable.

As for a condition could it not simply be:

1. If <iframe> is vertically scrollable and has height more than half the viewport height, enable override. (iframe.height > (viewport.height / 2) == override true.

OR

2. If <iframe> is horizontally scrollable and has width more than half the viewport width, enable override. (iframe.width > (viewport.width / 2) == override true.

Comment 12

[gordon]

Any more thoughts on this Masayuki?  We would love to be able to use native scrolling in Firefox as we do on all other browsers but the current decision to only implement the override on the root document is preventing us from doing so.

Comment 13

[gordon]

I really would love to hear more peoples thoughts on this.  Firefox's current behavior is very different from all other browsers and having to override default scroll behavior via javascript in order to keep it consistent is quite frustrating, especially as we lose all benefits of APZ by doing so.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

I think it's a good idea to make the iframe scroll speed consistent with the top-level content scroll speed. Here's a patch (untested) that should do it, for both APZ and non-APZ. I pushed it to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=be038c6668ac.

Gordon, once the builds in the try push are done, it would be great if you could check to see if they resolve the problem for you.

Comment 15

[gordon]

Thanks Kartikaya.  I've tested the win64 try build of attachment 8693606 [details] [diff] [review] and as expected, it works perfectly and completely rectifies our issue.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Thanks! The try push had some gtest failures which I fixed locally, and also seems to have run into some infra issues. I'll wait a while and redo the push to hopefully get real test results.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

I did another try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=80811dd0d61e&selectedJob=14268781 which is better. Shows a consistent M-2 failure but otherwise doesn't look bad. I'll see if I can fix that up.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

I just disabled the pref for that test, since it seems like the test doesn't have any special handling for it and I didn't want to add any. I found other tests that disable that pref as well so it seemed like a reasonable thing to do.

New try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=03030b2caa7b

Comment 19

[gordon]

Do you need me to test this latest one Kartikaya? If so i'll test it once it's finished building.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Nope, no need. Looks like that those patches introduced an unused variable warning anyway, so I cancelled the above try push and kicked off a new one:

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

/me crosses fingers

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Don't restrict the override to root content frames

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Rename pref

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8694736 [details] [diff] [review]
Part 1 - Don't restrict the override to root content frames

>@@ -412,22 +412,16 @@ WheelTransaction::OverrideSystemScrollSp
>   MOZ_ASSERT(aEvent->deltaMode == nsIDOMWheelEvent::DOM_DELTA_LINE);
> 
>   // If the event doesn't scroll to both X and Y, we don't need to do anything
>   // here.
>   if (!aEvent->deltaX && !aEvent->deltaY) {
>     return DeltaValues(aEvent);
>   }
> 
>-  // We shouldn't override the scrolling speed on non root scroll frame.
>-  if (sTargetFrame !=
>-        sTargetFrame->PresContext()->PresShell()->GetRootScrollFrame()) {
>-    return DeltaValues(aEvent);
>-  }

This makes *all* scrollable elements use overridden scroll speed. This is too bad for small scrollable elements like <select size="10">...</select>.

I think that you should compare the size of scrollable area with the root frame. But I'm not sure the good threshold. Over 3/4 of the root frame? And I think that you can apply only for x-axis or y-axis when the target is enough big only its width or height.

>diff --git a/dom/events/test/test_wheel_default_action.html b/dom/events/test/test_wheel_default_action.html
>--- a/dom/events/test/test_wheel_default_action.html
>+++ b/dom/events/test/test_wheel_default_action.html
>@@ -11,19 +11,27 @@
> <div id="content" style="display: none">
>   
> </div>
> <pre id="test">
> <script type="application/javascript">
> 
> SimpleTest.waitForExplicitFinish();
> SimpleTest.requestFlakyTimeout("untriaged");
>+SpecialPowers.pushPrefEnv({"set": [
>+    ["mousewheel.system_scroll_override_on_root_content.enabled", false]
>+]}, runTest);

So, this change really indicates your fault.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8694737 [details] [diff] [review]
Part 2 - Rename pref

I agree with renaming the pref. But I think there should be better name since you should restrict the target to enough big scrollable area.

Comment 25

[gordon]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #23)

> This makes *all* scrollable elements use overridden scroll speed. This is
> too bad for small scrollable elements like <select size="10">...</select>.
> 
> I think that you should compare the size of scrollable area with the root
> frame. But I'm not sure the good threshold. Over 3/4 of the root frame?

I understand your argument for keeping the slow scrolling on smaller elements, but we should be aware that every other browser out there does not do this.  Chrome/IE/Edge/Safari/Opera all apply the override on all elements, whatever the size.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to gordon from comment #25)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #23)
> 
> > This makes *all* scrollable elements use overridden scroll speed. This is
> > too bad for small scrollable elements like <select size="10">...</select>.
> > 
> > I think that you should compare the size of scrollable area with the root
> > frame. But I'm not sure the good threshold. Over 3/4 of the root frame?
> 
> I understand your argument for keeping the slow scrolling on smaller
> elements, but we should be aware that every other browser out there does not
> do this.  Chrome/IE/Edge/Safari/Opera all apply the override on all
> elements, whatever the size.

Even other browsers behave so, when we tried to change scroll speed in whole cases, some objections were reported. I still believe that we should keep current behavior for small elements which users may want to scroll with system default scroll speed.

Comment 27

[gordon]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #26)

> Even other browsers behave so, when we tried to change scroll speed in whole
> cases, some objections were reported. I still believe that we should keep
> current behavior for small elements which users may want to scroll with
> system default scroll speed.

Indeed.  I have no objection to small elements having a different scroll speed even despite the fact that it is inconsistent with other browsers.  As you say it can make sense in some instances.

I would however argue that your suggested 3/4 viewport threshold for the override feels a little low to me.  My instinct is that 1/2 of the root viewport would be a better cut-off point.  This would still allow small <select> elements to scroll slowly.

Regardless, as long as large <iframe>'s scroll at the same speed as the root document, then from our point of view, the issue would be resolved.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #23)
> This makes *all* scrollable elements use overridden scroll speed. This is
> too bad for small scrollable elements like <select size="10">...</select>.

Yes, I did that intentionally to be consistent. I don't want to implement a patch that compares the size of the scrollable area, because I think that adds unnecessary complexity, and the inconsistency will fall down in various cases. For example, you said that the overridden scroll speed is bad for small scrollable elements. Well, right now if you resize your browser window down to a small area, you will see the exact same problem on the root content. So by your argument, we should just disable the overridden scroll speed across the board. I'm fine with that approach too, we can just flip the pref to false on windows like it is everywhere else.

> >+SpecialPowers.pushPrefEnv({"set": [
> >+    ["mousewheel.system_scroll_override_on_root_content.enabled", false]
> >+]}, runTest);
> 
> So, this change really indicates your fault.

Well, it means the test doesn't handle this inconsistency resulting from this pref. Which is fine because that's not the point of the test, and that's why I figured it would be fine to disable the pref for the test. Even with the pref disabled, the test is still testing what it's supposed to.

Comment 29

[gordon]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #26)

> when we tried to change scroll speed in whole
> cases, some objections were reported.

I think you will find that this may now be outdated.  I would imagine the concerns arose when Win32 was the only Windows platform and thus, the scroll speed was consistent across Windows applications and the OS itself.  

Now we have a situation where Metro and Universal apps are taking over and in the case of Windows 10, all OS apps and elements are Universal.  All of these use a far higher scroll speed, whatever the size of the app.  When you add in the fact that Edge/Chrome/Safari/Opera do not behave in this way, it alienates Firefox further as it feels entirely different to both Widows and all other browsers.

Perhaps it would be worth reviewing in view of recent Windows OS developments?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't think so because same bugs haven't been filed. So, some users who need to scroll only in Firefox quicker than its default must have already modified the multiplier settings.

Anyway, I don't disagree with applying faster scroll in large scrollable area. So, somebody would write a patch, I'd review it.

Comment 31

[gordonrankin82]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #30)
> I don't think so because same bugs haven't been filed. So, some users who
> need to scroll only in Firefox quicker than its default must have already
> modified the multiplier settings.

If the need for slow scrolling on small elements was so great, why is Firefox the 'only' browser to exhibit that behaviour?  Surely the users of Edge/Chrome/Safari/Opera would have received the same feedback?  It feels as though Firefox is intentionally out of place here.  All we really want is the same behaviour across all browsers.

> Anyway, I don't disagree with applying faster scroll in large scrollable
> area. So, somebody would write a patch, I'd review it.

Fair enough, but it seems unlikely that someone is going to take this or they would have done so by now.  I guess we will just have to resign ourselves to using javascript based mousewheel handling forever, without ever getting the benefits of APZ.  In its current state Firefox is unusable with a mouse for us.

Thanks anyway Masayuki.

Comment 32

[Botond Ballo [:botond]]

(In reply to gordonrankin82 from comment #31)
> > Anyway, I don't disagree with applying faster scroll in large scrollable
> > area. So, somebody would write a patch, I'd review it.
> 
> Fair enough, but it seems unlikely that someone is going to take this or
> they would have done so by now.

It could be you.

Comment 33

[gordonrankin82]

(In reply to Botond Ballo [:botond] from comment #32)

> It could be you.

Believe me, If I were capable, I would do.  Unfortunately leading a team of 250 people requires a completely different skillset. My C/C++ knowledge is absolutely not where it should be.  My daily life revoles around managing teams, not writing code any longer.

Regardless, I understand your point.  But on the flip side, feel that bringing Firefox inline with every other browser out there should be frightfully easy.  In fact I would be so bold as to state that Mozillas failure to deal quickly with simple, expected functionality like this, is exactly the attitute that has contributed to Firefox' damatic loss in market share over the last year or so, despite it being bar far the best browers out there. (Yes I am a huge Mozilla fan, but not beyond criticism)

Simply put, people think 'WTF' when Firefox delivers an experience that is completely out of sync with their host OS.  This beautiful browser should meet it's users expectations. End of.

Comment 35

[indy.ruffles]

Is there any update on this?  We are finding this incredibly frustrating as large scroll-able areas are so prevalent throughout the web these days?

Many high ranking websites such as http://indy100.independent.co.uk/discover suffer from this problem.

Since kartikaya wrote a working patch that applied the mousewheel override on all elements, surely it should be trivial to write a patch that simply checks the viewport size before applying it?

Comment 36

[Kartikaya Gupta (email:[e-mail])]

It's up to Masayuki. I already stated my views in comment 28.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See comment 23, comment 26 and comment 30.

Comment 38

[Kartikaya Gupta (email:[e-mail])]

Is there any data we could conceivably collect via telemetry to settle this one way or another?

Comment 39

[gordonrankin82]

Another alternative other than checking the element size would be to simply apply the override to all elements, other than those intended to be small, such as <select> elements.  Perhaps with a simple blacklist.

That way Firefox will be consistent with all other browsers when using scrollable <div>'s but still slow down the mousewheel for small UI elements such as scrollable <select> menu's etc.

Comment 40

[flash.vkv]

Attachment: test-win-scroll.htm

Comment 41

[gordon]

It's worth noting that Google's Infinite scroller demo suffers from this issue in Firefox, due to it being contained in a full page height scrollable element.

https://developers.google.com/web/updates/2016/07/infinite-scroller

These use cases are becoming far more common in modern web-apps.

Comment 42

[Botond Ballo [:botond]]

(In reply to gordon from comment #41)
> These use cases are becoming far more common in modern web-apps.

Perhaps someone will become motivated to write a patch then.

I'll sweeten the pot and offer to mentor anyone who's willing to try. It sounds to me like either a strategy based on size as suggested in comment 23, or element type as suggested in comment 39, would be viable.

Comment 43

[Kartikaya Gupta (email:[e-mail])]

I'm still generally opposed to the idea of special-casing any elements, whether they are the root element or based on their size. However if somebody takes the time to write a patch I won't object to landing it and evaluating how it improves/degrades things.

Tagging bug as P5 since we'll accept patches but probably won't work on this directly.

Comment 44

[gordon]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #43)
> I'm still generally opposed to the idea of special-casing any elements,
> whether they are the root element or based on their size.

I agree, all elements should scroll at the same speed in my opinion.  As they do in all other browsers, and for every host OS scrollable UI component (Windows/OSX/Linux).  You're first patch addressed the issue perfectly as far as I am concerned.  But if special casing is the only way to make scrolling large elements bare-able, then I guess that will have to do.

Comment 45

[Kartikaya Gupta (email:[e-mail])]

smaug, do you have any opinion here? Here's a quick summary:

- On Windows, we treat the root scrollable content element as special, and apply different scroll multipliers to it. This results in inconsistent behaviour in a number of ways:
(1) it means subframes scroll differently from the root scrollable
(2) web apps that use full-page-height scrollable divs as their main scroller will scroll differently from "regular" pages that scroll the body/html element
(3) firefox is the only browser that has this behaviour

- My opinion (and that of some web authors above) is that we should remove the inconsistency and treat all the scrollable elements the same. The patches attached to this bug do that.

- Masayuki's opinion is that we should expand the special-casing behaviour to include more elements, based in part on how large they are. I'm not sure exactly what heuristic he wants to use this for this. There's some stuff in comment 10, comment 23, and a few others about this.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

The fewer special cases the better, IMO.

Though, as comment 23 hints, better to make sure things like <select> work still well.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

And with "better to make sure", I mean, need to test manually that things still work well, 
possibly with various OS level settings.

Comment 48

[Kartikaya Gupta (email:[e-mail])]

I'm having trouble making any sense of this bug now. I used mozregression to check clean profiles with e10s enabled and disabled on nightlies from 2015-10-23 (when this bug was filed) and yesterday's nightly. This is what I got:

mach mozregression --launch 2015-10-23 --pref 'browser.tabs.remote.autostart:false' 'browser.tabs.remote.autostart.1:false' 'browser.tabs.remote.autostart.2:false'
--> scrolls about 6 lines for both root and iframe

mach mozregression --launch 2016-08-15 --pref 'browser.tabs.remote.autostart:false' 'browser.tabs.remote.autostart.1:false' 'browser.tabs.remote.autostart.2:false'
--> scrolls about 6 lines for both root and iframe

mach mozregression --launch 2015-10-23 --pref 'browser.tabs.remote.force-enable:true'
--> scrolls 3 lines for iframe, 6 lines for root

mach mozregression --launch 2016-08-15 --pref 'browser.tabs.remote.force-enable:true'
--> scrolls 3 lines for iframe, 6 lines for root

mach mozregression --launch 2015-10-23 --pref 'browser.tabs.remote.force-enable:true' 'layers.async-pan-zoom.enabled:false'
--> scrolls 6 lines for both

mach mozregression --launch 2016-08-15 --pref 'browser.tabs.remote.force-enable:true' 'layers.async-pan-zoom.enabled:false'
--> scrolls 6 lines for both

Or in table form:

Build      | e10s off     | e10s+APZ on                  | e10s on, APZ off
-----------------------------------------------------------------------------
2015-10-23 | 6 lines each | 3 lines in iframe, 6 in root | 6 lines each
2016-08-15 | 6 lines each | 3 lines in iframe, 6 in root | 6 lines each

... which means this bug is about APZ-only behaviour, somehow. Which means we should just change the APZ behaviour to match the non-APZ behaviours and be done with it.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> ... which means this bug is about APZ-only behaviour, somehow.

I forgot the bug#, we have a bug to reported the APZ behavior.

Comment 50

[gordon]

Attachment: Testcase with scrollable DIV

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #48)

> ... which means this bug is about APZ-only behaviour, somehow. Which means
> we should just change the APZ behaviour to match the non-APZ behaviours and
> be done with it.

While I can confirm your findings, I have just modified the test case to switch out the iframe for a <div>.  And then the original behavior as described in the initial bug report returns.  Even with APZ disabled, I still only get the 3 lines per tick scrolling on the full height <div>.  So it would appear that something was fixed in the non-APZ path with regards to iframe roots only, meaning that this isn't 'only' an APZ bug.

To summarize, the bug effects:

- APZ path on all elements (iframe/div)
- Non-APZ path on all elements 'other' than iframes.

Perhaps the new iframe in the non-APZ path, creates a new root, triggering the override?

Kartikaya, can you confirm this behavior with the new tesstcase with/without APZ?

Comment 51

[Kartikaya Gupta (email:[e-mail])]

(In reply to gordon from comment #50)
> Kartikaya, can you confirm this behavior with the new tesstcase with/without
> APZ?

Yeah, I can confirm this behavior.

> Perhaps the new iframe in the non-APZ path, creates a new root, triggering
> the override?

It appears so, yeah. The non-APZ path uses the check at [1], which is true for the root scrollframe of any document (including the root scrollframe of iframes) whereas the APZ path uses the check at [2] which is only true for the root scrollframe of the root content document.

I also did some scrolling comparisons using the test page I made at [3], comparing FF to Edge and Chrome. Chrome scrolls the same amount per wheel scroll on everything, which means that on the small scrollable elements, it scrolls more than what is visible, and you skip completely past some elements. Edge seems to ramp up their scroll distance based on the height of the element. It also appears to be tuned for the larger sized elements, so those work well but then the smaller elements scroll by miniscule amounts which is annoying. FF (both APZ and non-APZ) has a mix of the two, where it tries to scroll a given amount, but then clamps it based on the height of the scrollable element so that we never scroll more than what is visible. IMO this is the best of both worlds. The bad thing about FF is, of course, the inconsistency between the root scroller and element scrollers, which I can remove by taking out the check at [2] in the APZ codepath.

[1] http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/dom/events/WheelHandlingHelper.cpp#416
[2] http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/gfx/layers/apz/src/AsyncPanZoomController.cpp#1507
[3] https://mozilla.staktrace.com/bug/1217715.html

Comment 52

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8694736 [details] [diff] [review]
Part 1 - Don't restrict the override to root content frames

Obsoleting these old patches. I'm going to make a new one that just changes the APZ code path. We already determined that the APZ codepath contains a regression compared to the non-APZ codepath (i.e. that iframes get treated as non-root whereas in non-APZ they get treated as root), so that alone justifies a fix here. My testing shows that treating all scrollers the same in the APZ codepath doesn't seem to make anything worse so for the sake of consistency I'm just going to go with that.

Comment 53

[gordon]

Just to clarify, does that mean that you're intending your patch to provide 6 lines per tick on 'all' scrollers (size permitting), making large/full-height elements/iframes scroll the same distance per tick as the root document currently does?

If so, fantastic.

Comment 54

[Kartikaya Gupta (email:[e-mail])]

Yes. But only for the APZ codepath.

Comment 55

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1217715 - Don't limit the system scroll override to the root content.

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

Comment 56

[Kartikaya Gupta (email:[e-mail])]

Try is misbehaving for windows jobs right now, but I ran this patch on my windows machine locally and it passed the tests that I touched in the previous patch, so I'm hoping everything will be green. I'll make sure to run tests on it properly before it lands.

Comment 57

[Botond Ballo [:botond]]

Comment on attachment 8782041 [details]
Bug 1217715 - Don't limit the system scroll override to the root content.

https://reviewboard.mozilla.org/r/72268/#review69916

To summarize, this patch makes us obey the system scroll override for all scrollable elements (with APZ enabled), but as Kats discovered in comment 51, we still clamp scrolling to ensure that a single wheel tick doesn't scroll by more than one "page" (i.e. by the height of the scrollable element) [1]. It seems to me that this clamping addresses the concerns about smaller scrollable elements.

[1] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/gfx/layers/apz/src/AsyncPanZoomController.cpp#1565

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

All scrollable elements include small elements like <select>, <textarea>, etc... So, some users might feel too fast with such elements.

However, if you change the APZ behavior, I'd like to suggest that you to remove the check in non-APZ path too. I don't understand the reason why we need to keep current behavior only in non-APZ path since you're changing the behavior of APZ aggressively.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

yes, apz and non-apz shouldn't work very differently.

Comment 60

[Kartikaya Gupta (email:[e-mail])]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #58)
> All scrollable elements include small elements like <select>, <textarea>,
> etc... So, some users might feel too fast with such elements.

I checked all those elements in my test page, and they all seem to behave fine.

> However, if you change the APZ behavior, I'd like to suggest that you to
> remove the check in non-APZ path too. I don't understand the reason why we
> need to keep current behavior only in non-APZ path since you're changing the
> behavior of APZ aggressively.

Ok, will do.

Comment 61

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8782041 [details]
Bug 1217715 - Don't limit the system scroll override to the root content.

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

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8782041 [details]
Bug 1217715 - Don't limit the system scroll override to the root content.

https://reviewboard.mozilla.org/r/72268/#review70572

Although, I still believe we should limit with size of scrollable elements, but let's try this for now.

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8759b5c7e171
Don't limit the system scroll override to the root content. r=botond,masayuki

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8759b5c7e171

Comment 65

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8782041 [details]
Bug 1217715 - Don't limit the system scroll override to the root content.

Approval Request Comment
[Feature/regressing bug #]: long-standing inconsistency issue. APZ added some extra inconsistency in 48 and up.
[User impact if declined]: On Windows, subframes (select elements, textareas, scrollable divs, etc.) scroll half as much as the root frame per mousewheel tick. This can be annoying to users and puzzling to web developers.
[Describe test coverage new/current, TreeHerder]: tested locally
[Risks and why]: users who prefer the old behaviour might complain. low risk of other types of regressions
[String/UUID change made/needed]: none

Comment 66

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hey Gordon, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 67

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8782041 [details]
Bug 1217715 - Don't limit the system scroll override to the root content.

Fixes a regression, Aurora50+

Comment 68

[gordon]

(In reply to Ritu Kothari (:ritu) from comment #66)
> Hey Gordon, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

I can verify that the issue is entirely fixed in the latest Nightly.  The difference is like night and day.  Mousewheel scrolling is actually usable in our app now.  It's going to be great to be able to recommend Firefox as our browser of choice, to our customers once again.

Great work guys. Thank you.

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4afc101231ca

Comment 70

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to gordon from comment #68)
> (In reply to Ritu Kothari (:ritu) from comment #66)
> > Hey Gordon, could you please verify this issue is fixed as expected on a
> > latest Nightly build? Thanks!
> 
> I can verify that the issue is entirely fixed in the latest Nightly.  The
> difference is like night and day.  Mousewheel scrolling is actually usable
> in our app now.  It's going to be great to be able to recommend Firefox as
> our browser of choice, to our customers once again.
> 
> Great work guys. Thank you.

Awesome! Thank you for a prompt verification. :)

Comment 71

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This could be one of the causes of bug 1316505.