Closed Bug 1316505 Opened 8 years ago Closed 8 years ago

Tabbar scrolls too fast with wheel supporting high resolution scroll on Windows

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bug 1316505 part.1 The "wheel" event handler of <scrollbox> should use |.scrollByPixels()| for respecting wheel event's scroll speed and scrolling smoother
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1316505 part.2 The "wheel" event of <scrollbox> should use |.scrollByPixels()| instead of |.scrollByPages()| for respecting high resolution delta value and making the scroll smoother
58 bytes, text/x-review-board-request
mstange
: review+
Details 
This is not a recent regression, sorry for the delay to report this.

I'm using MX Master of Logitech whose wheel supports high resolution scroll. When I try to scroll tabbar of Firefox, it's scrolled too fast and it's really unusable.

According to a testcase, it generates 40 wheel events per notch whose delta mode is DOM_DELTA_LINE and whose deltaY is 0.025.

According to the <scrollbox> code, wheel event handler sends the raw deltaY to scrollByIndex().
https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/toolkit/content/widgets/scrollbox.xml#556,563

Then, it treats the delta as integer:
https://dxr.mozilla.org/mozilla-central/rev/783356f1476eafd8e4d6fa5f3919cf6167e84f8d/toolkit/content/widgets/scrollbox.xml#318,337-338

I think that wheel event handler should accumulate deltaX/Y values and put off until accumulated delta becomes 3 or -3 (perhaps, it's default amount of one notch of legacy mouse wheel on Windows).
Or we could fall back to pixel scrolling (based on some pixel "line height") for deltas smaller than one.
See Also: → 1263975
If we will expose lineOrPageDeltaX/Y to chrome script, this must be able to be fixed easy.
Comment on attachment 8809846 [details]
Bug 1316505 part.1 Expose mozilla::WheelEvent::mLineOrPageDelta(X|Y) to chrome script via DOM WheelEvent

https://reviewboard.mozilla.org/r/92344/#review92382

I don't understand this. If we need to add this kind of feature to WheelEvent, we would need to add it to the spec too, no? Since if chrome code needs this, why wouldn't web pages too.

::: dom/webidl/WheelEvent.webidl:26
(Diff revision 1)
>    readonly attribute double        deltaY;
>    readonly attribute double        deltaZ;
>    readonly attribute unsigned long deltaMode;
>  
> +  // lineOrPageDelta attributes are useful when wheel event handlers want to
> +  // handles wheel events as low resoultuion events.  If the wheel device

resolution
Attachment #8809846 - Flags: review?(bugs) → review-
Comment on attachment 8809847 [details]
Bug 1316505 part.2 wheel event handler of XUL <scrollbox> should use lineOrPageDelta(X|Y) when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE

https://reviewboard.mozilla.org/r/92346/#review92384

::: toolkit/content/widgets/scrollbox.xml:579
(Diff revision 1)
>            let isVertical = Math.abs(event.deltaY) > Math.abs(event.deltaX);
> -          let delta = isVertical ? event.deltaY : event.deltaX;
> +          let delta;
> +          if (event.deltaMode == event.DOM_DELTA_PIXEL) {
> +            delta = isVertical ? event.deltaY : event.deltaX;
> +          } else {
> +            // Use low resolution delta value for line or page scroll because

So shouldn't this code just store the delta value somewhere and if < 1, add new deltas there and once > 1, then do some scrolling.
Comment on attachment 8809846 [details]
Bug 1316505 part.1 Expose mozilla::WheelEvent::mLineOrPageDelta(X|Y) to chrome script via DOM WheelEvent

https://reviewboard.mozilla.org/r/92344/#review92382

Hmm, the concept of this approach is, legacy DOMMouseScroll event handlers can be replaced with wheel event handler easy and low risk. So, for fixing quickly in 52, I'd like to use this. (So, although, this is hacky approach. The ideal fix is reimplementing the two methods of <scrollbox>, but it's risky for now.)

Anyway, do you think that these concept attributes are useful for web apps? I don't think so because:

* Most web apps are not maintained continuously, so, I guess that most developers won't try to replace legacy event handlers with new wheel event handler.
* When web apps need to implement something with wheel event handler, I guess that they will support high resolution scroll support.

If web developers want low resolution wheel events, like these attributes must be useful, but I have no idea when they want such rough delta value.

Now, I think the time is over for 52, I'll try to rewrite the patch with Markus's approach. But if you think I should suggest rough delta value to UI Events, let me know, I'll do that.
Comment on attachment 8809847 [details]
Bug 1316505 part.2 wheel event handler of XUL <scrollbox> should use lineOrPageDelta(X|Y) when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE

https://reviewboard.mozilla.org/r/92346/#review92518

::: toolkit/content/widgets/scrollbox.xml:579
(Diff revision 1)
>            let isVertical = Math.abs(event.deltaY) > Math.abs(event.deltaX);
> -          let delta = isVertical ? event.deltaY : event.deltaX;
> +          let delta;
> +          if (event.deltaMode == event.DOM_DELTA_PIXEL) {
> +            delta = isVertical ? event.deltaY : event.deltaX;
> +          } else {
> +            // Use low resolution delta value for line or page scroll because

Yes, but that would be a re-implementation of EventStateManager::DeltaAccumulator, it's not so simple because it discards accumulated delta values with timeout or different direction wheel event.

So, if we'd take like this approach, every legacy mouse scroll event handlers need to reimplement such code. I don't think it makes sense. Therefore, I tried to expose the low resolution delta values.
I'm basically saying that we should use web-APIs in browser chrome as much as possible, and if those aren't enough, it possibly hints the API is missing something which should be exposed also the web.

If web pages need to reimplement something like EventStateManager::DeltaAccumulator often, perhaps we really should expose some new API.

Another possibility here is, I think, is to change scrollByIndex() and scrollByPage() to use doubles.

But if you think it is lots of work to make the needed changes in JS, I think I'm ok to add the new [ChromeOnly] attributes. Feels a bit bad, but if it makes browser chrome behave better, I can live with it.
Okay, I filed spec issue: https://github.com/w3c/uievents/issues/114

However, I'll fix this without the new spec because I must have enough time to stay here for now.
Attachment #8809846 - Attachment is obsolete: true
Attachment #8809847 - Attachment is obsolete: true
I rewrote the patches with the approach which Markus said at comment 2.

I used font height as line height of the unit of 1 line scroll in <scrollbox>. However, I feel it may be slow for tabbar of tabbrowser.

I think that if somebody will report about the slowness, browser should override it or allowing to override lineScrollAmount value with attribute value. E.g., <scrollbox linescrollamount="32">, etc, although, I'm not sure how that work with HiDPI environment.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Comment on attachment 8810166 [details]
Bug 1316505 part.1 The "wheel" event handler of <scrollbox> should use |.scrollByPixels()| for respecting wheel event's scroll speed and scrolling smoother

I don't think I understand any of the scrolling code or what the implications of this patch are. Maybe Markus would understand this better.

Note that scrollbox.boxObject.scrollByLines will scroll by lines using what the scrollframe thinks a line is without having to calculate it yourself.
Attachment #8810166 - Flags: review?(enndeakin) → review?(mstange)
Attachment #8810167 - Flags: review?(enndeakin) → review?(mstange)
(In reply to Neil Deakin from comment #20)
> Note that scrollbox.boxObject.scrollByLines will scroll by lines using what
> the scrollframe thinks a line is without having to calculate it yourself.

Ah, thanks. However, scrollByLines takes long. Therefore, it isn't available for high resolution scroll like this case... (Should we change it here??)
Comment on attachment 8810166 [details]
Bug 1316505 part.1 The "wheel" event handler of <scrollbox> should use |.scrollByPixels()| for respecting wheel event's scroll speed and scrolling smoother

https://reviewboard.mozilla.org/r/92564/#review95346

So this has two effects: It reduces the scroll speed in the tab bar for regular "notch"-based wheel scrolling, and it means that we don't try to align the tab edge with the scroll port. Both of these changes sound ok to me, but I guess we'll see how users react to them.

::: toolkit/content/widgets/scrollbox.xml:161
(Diff revision 1)
> +          // XXX Is this possible case?  For the last resote, let's return
> +          //     default font-size of web contents.

dbaron says that "half the web probably depends on" the value always being in pixels. nsComputedDOMStyle::DoGetFontSize() uses SetAppUnits which I think will always result in a "px" value.
Attachment #8810166 - Flags: review?(mstange) → review+
Comment on attachment 8810167 [details]
Bug 1316505 part.2 The "wheel" event of <scrollbox> should use |.scrollByPixels()| instead of |.scrollByPages()| for respecting high resolution delta value and making the scroll smoother

https://reviewboard.mozilla.org/r/92566/#review95354

IIRC I had to implement scrollByPage just for wheel handling; you can probably remove the scrollByPage method now.
The reason I didn't use scrollByPixels was that I wanted to align the scrolled element with the scroll port. But doing that might actually not be important at all.
Attachment #8810167 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #23)
> IIRC I had to implement scrollByPage just for wheel handling; you can
> probably remove the scrollByPage method now.
> The reason I didn't use scrollByPixels was that I wanted to align the
> scrolled element with the scroll port. But doing that might actually not be
> important at all.

Hmm, unfortunately, some tab related add-ons use this for scrolling tabbar. So, I'll keep it at least for now.

(In reply to Markus Stange [:mstange] from comment #22)
> So this has two effects: It reduces the scroll speed in the tab bar for
> regular "notch"-based wheel scrolling, and it means that we don't try to
> align the tab edge with the scroll port. Both of these changes sound ok to
> me, but I guess we'll see how users react to them.

Exactly and yes, we should check regression reports.

> ::: toolkit/content/widgets/scrollbox.xml:161
> (Diff revision 1)
> > +          // XXX Is this possible case?  For the last resote, let's return
> > +          //     default font-size of web contents.
> 
> dbaron says that "half the web probably depends on" the value always being
> in pixels. nsComputedDOMStyle::DoGetFontSize() uses SetAppUnits which I
> think will always result in a "px" value.

Yeah, but I guess that it's not guaranteed by the spec. I'm afraid that Gecko changes the behavior for improving compatibility with another browser in the future.


So, I'll land them without any change. If you think I should change something, let me know later. I'll file a bug and write new patches.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7748e7cc3d9c
part.1 The "wheel" event handler of <scrollbox> should use |.scrollByPixels()| for respecting wheel event's scroll speed and scrolling smoother r=mstange
https://hg.mozilla.org/integration/autoland/rev/f4f6a59b5310
part.2 The "wheel" event of <scrollbox> should use |.scrollByPixels()| instead of |.scrollByPages()| for respecting high resolution delta value and making the scroll smoother r=mstange
https://hg.mozilla.org/mozilla-central/rev/7748e7cc3d9c
https://hg.mozilla.org/mozilla-central/rev/f4f6a59b5310
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I think this patch made tab bar scrolling weird on Linux. No more smooth scrolling and the behaviour is different.
Depends on: 1320467
Oh right, I forgot about the fact that it stops the tab bar from doing smooth scrolling! That is a problem. I think we do need to expose lineOrPageDelta.
Or, as an alternative, we could always scroll smoothly (but still in amounts of pixels) when the event's delta mode is lines. But then we'll have to tweak the smooth scroll animation to be a little more aggressive so that it doesn't feel too sluggish with a high resolution scroll mouse.
I guess that better solution is, using smooth scroll only when the scroll amount is enough big (e.g., over 1 line height).
Component: Tabbed Browser → XUL Widgets
Product: Firefox → Toolkit
Target Milestone: Firefox 53 → ---
Target Milestone: --- → mozilla53
It looks like this caused a regression: bug 1327271
Depends on: 1327271
I guess we want this to ride the train with 53.
status-firefox52: affected → wontfix
Depends on: 1349828
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: