970141 - The value of deltaX and deltaY of WheelEvent should be in CSS pixels if its deltaMode is DOM_DELTA_PIXEL

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Evan Wallace [:evanw]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140129030602

Steps to reproduce:

1) Obtain a machine with a retina display and a trackpad (a Retina MacBook Pro, for example)
2) Open the web console
3) Type "document.body.addEventListener('wheel', function(e) { console.log(e.deltaX, e.deltaY); }, false);"
4) Move the mouse over the page and scroll very slowly



Actual results:

The scroll quantum is +2/-2 instead of +1/-1. This causes any app that uses custom scrolling events to scroll way too fast.



Expected results:

The scroll quantum should be +1/-1 as it is in Chrome and in Firefox on a non-retina display.

Comment 1

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

Hmm, we set actual device pixels to the delta values of WheelEvent.
http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/widget/cocoa/nsChildView.mm#l4974

However, I guess that ESM handles scroll by virtual device pixels:
http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/dom/events/nsEventStateManager.cpp#l2919

Then, the scroll speed may become too faster than other applications.

This is probably very bad things for web application developers since web application developers cannot create custom scrollable element which is HiDPI environment aware. So, DOMWheelEvent should return the delta values in CSS pixels like Chrome (I cannot confirm it, though). How do you think, smaug?

However, I'm not sure if we should fix the scroll speed of default action of WheelEvent since somebody may feel Firefox becomes slower due to the scroll speed change. I believe that a lot of users feel the speed of browser from its scroll speed. Therefore, fixing the default action's scroll speed may have bad impact for our marketing. However, I'm prefer the native behavior (i.e., same scroll speed with other applications), though. Therefore, I think that we should make the scroll speed slower down. How do you think, Steven?

If we will fix the scroll speed of the default action, we should just stop multiplying the scale in nsChildView.mm.

Comment 2

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #1)
> Hmm, we set actual device pixels to the delta values of WheelEvent.
> However, I guess that ESM handles scroll by virtual device pixels:

This seems correct to me. What do you mean by "virtual" device pixels?

> Then, the scroll speed may become too faster than other applications.

Is it? I don't think it is.

> So, DOMWheelEvent should return the delta values in
> CSS pixels

I agree with this. I think this is where the problem lies: the attributes of DOMWheelEvents are currently in device pixels, and web pages interpret them as CSS pixels.

Comment 3

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

(In reply to Markus Stange [:mstange] from comment #2)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #1)
> > Hmm, we set actual device pixels to the delta values of WheelEvent.
> > However, I guess that ESM handles scroll by virtual device pixels:
> 
> This seems correct to me. What do you mean by "virtual" device pixels?

I meant it's same as CSS pixel when the content isn't zoomed in/out.

Doesn't Cocoa hide actual device pixels?

E.g., how do we set the refPoint of mouse events?
http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/widget/cocoa/nsChildView.mm#l5117

It doesn't use nsCocoaUtils::GetBackingScaleFactor().

> > Then, the scroll speed may become too faster than other applications.
> 
> Is it? I don't think it is.

If nsPresContext's 1px isn't same as actual device pixel in HiDPI environment, isn't it correct? I assume that Cocoa's 1px doesn't represents actual device pixel's 1px with retina display.
# FYI: I don't have retina display environment, so, I cannot confirm actual behavior. 

> > So, DOMWheelEvent should return the delta values in
> > CSS pixels
> 
> I agree with this. I think this is where the problem lies: the attributes of
> DOMWheelEvents are currently in device pixels, and web pages interpret them
> as CSS pixels.

Oops, using CSS pixels is wrong if the content is zoomed in or out. I meant that they should represent in virtual device pixels.

Comment 4

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> (In reply to Markus Stange [:mstange] from comment #2)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #1)
> > > Hmm, we set actual device pixels to the delta values of WheelEvent.
> > > However, I guess that ESM handles scroll by virtual device pixels:
> > 
> > This seems correct to me. What do you mean by "virtual" device pixels?
> 
> I meant it's same as CSS pixel when the content isn't zoomed in/out.
> 
> Doesn't Cocoa hide actual device pixels?

No, it doesn't hide them. Or at least we revert any hiding in widget/cocoa so that Gecko always sees real device pixels. In HiDPI mode, when the content isn't zoomed, it's displayed at 30 app units per device pixel (and, as always, 60 app units per CSS pixel). nsDeviceContext::UnscaledAppUnitsPerDevPixel() plays a role here, I think.

> E.g., how do we set the refPoint of mouse events?
> http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/widget/cocoa/
> nsChildView.mm#l5117
> 
> It doesn't use nsCocoaUtils::GetBackingScaleFactor().

It uses nsChildView::CocoaPointsToDevPixels, see the definition in nsChildView.h:
return nsCocoaUtils::CocoaPointsToDevPixels(aPts, BackingScaleFactor());
                                                  ^^^^^^^^^^^^^^^^^^

> # FYI: I don't have retina display environment, so, I cannot confirm actual
> behavior. 

Oh, that makes it quite a bit harder.

> > > So, DOMWheelEvent should return the delta values in
> > > CSS pixels
> > 
> > I agree with this. I think this is where the problem lies: the attributes of
> > DOMWheelEvents are currently in device pixels, and web pages interpret them
> > as CSS pixels.
> 
> Oops, using CSS pixels is wrong if the content is zoomed in or out. I meant
> that they should represent in virtual device pixels.

Hmm. I think DOM APIs should only deal with CSS pixels. So if the page is zoomed in, and we want scrolling to keep the same device pixel scrolling speed as when the content is not zoomed, I think we should scale down the values we give to JS accordingly.

Comment 5

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

Thanks for a lot of information. Then, if we set delta values as same scale of refPoint, the issue of WheelEvent.detlaX/Y values is INVALID at least for now. They should be in same scale.

I'd like to know if the scroll speed is actually faster than other applications on retina display environment.

(In reply to Markus Stange [:mstange] from comment #4)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> > (In reply to Markus Stange [:mstange] from comment #2)
> > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #1)
> > > > So, DOMWheelEvent should return the delta values in
> > > > CSS pixels
> > > 
> > > I agree with this. I think this is where the problem lies: the attributes of
> > > DOMWheelEvents are currently in device pixels, and web pages interpret them
> > > as CSS pixels.
> > 
> > Oops, using CSS pixels is wrong if the content is zoomed in or out. I meant
> > that they should represent in virtual device pixels.
> 
> Hmm. I think DOM APIs should only deal with CSS pixels. So if the page is
> zoomed in, and we want scrolling to keep the same device pixel scrolling
> speed as when the content is not zoomed, I think we should scale down the
> values we give to JS accordingly.

Indeed... But it should be another bug.

Comment 6

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

Cancelling needsinfo? to Smaug per comment 4 and 5.

Comment 7

[Markus Stange [:mstange]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> Thanks for a lot of information. Then, if we set delta values as same scale
> of refPoint

Are you saying that refPoint currently is in device pixels? That would surprise me.

> I'd like to know if the scroll speed is actually faster than other
> applications on retina display environment.

No it's not. Gecko-internal scrolling with the default action is the same speed as other applications.

JS-implemented scrolling that interprets delta values as CSS pixels is too fast. And that's what this bug is about, no?

> (In reply to Markus Stange [:mstange] from comment #4)
> > Hmm. I think DOM APIs should only deal with CSS pixels. So if the page is
> > zoomed in, and we want scrolling to keep the same device pixel scrolling
> > speed as when the content is not zoomed, I think we should scale down the
> > values we give to JS accordingly.
> 
> Indeed... But it should be another bug.

I disagree, and I don't understand why you think it should.

Comment 8

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

(In reply to Markus Stange [:mstange] from comment #7)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> > I'd like to know if the scroll speed is actually faster than other
> > applications on retina display environment.
> 
> No it's not. Gecko-internal scrolling with the default action is the same
> speed as other applications.

That sounds good to me. It's very unclear point of the report.

> JS-implemented scrolling that interprets delta values as CSS pixels is too
> fast. And that's what this bug is about, no?

Then, the component is wrong :-)

> > (In reply to Markus Stange [:mstange] from comment #4)
> > > Hmm. I think DOM APIs should only deal with CSS pixels. So if the page is
> > > zoomed in, and we want scrolling to keep the same device pixel scrolling
> > > speed as when the content is not zoomed, I think we should scale down the
> > > values we give to JS accordingly.
> > 
> > Indeed... But it should be another bug.
> 
> I disagree, and I don't understand why you think it should.

Because it needs different work in ESM. Separating to another bug is better in such situation, I think.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #1)

> So, DOMWheelEvent should return the delta values in CSS pixels like
> Chrome (I cannot confirm it, though).

> However, I'm prefer the native behavior (i.e., same scroll speed
> with other applications), though. Therefore, I think that we should
> make the scroll speed slower down. How do you think, Steven?

I agree with this ... I think.  As does Markus ... I think.

In any case, I don't think "device pixels" should be "user visible"
(in the sense of being visible to some external content provider's
JavaScript code).  It's CSS pixels that should be visible in this way,
because they're hardware independent.

Comment 10

[Evan Wallace [:evanw]]

Sorry for the confusion. I may not have meant devicePixelRatio in the original bug title but it's probably easier to describe the desired behavior instead. To be consistent with the native OS X experience, scrolling with two fingers from top to bottom on the trackpad at a normal speed (one second top to bottom?) roughly corresponds to scrolling by a single window of content in a maximized window. I filed this bug because web apps that implement custom scrolling by mapping wheel event deltas to CSS pixel offsets will incorrectly scroll by two windows of content on a retina display in Firefox, unlike scrolling implemented natively in Firefox with "overflow: scroll". Obviously the single-window metric is impossible to precisely define with scroll acceleration in effect, which is why I used the scroll quantum in the original bug report instead.

Ideally the apparent device pixel scroll speed should still be the same regardless of current zoom level just like it is in other OS X apps. In Preview.app, for example, scrolling with the trackpad from top to bottom at a normal speed is still roughly a single window of content at every zoom level. But Chrome's implementation doesn't do this (it fails to scale the wheel deltas based on the zoom level) and perhaps consistency with other browsers is better than the ideal solution in the case of a zoomed page. Here's an example test page for testing scrolling speed with wheel event deltas: http://jsbin.com/gemedere.

Comment 11

[Masatoshi Kimura [:emk]]

Is this Mac specific?

Comment 12

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

(In reply to Masatoshi Kimura [:emk] from comment #11)
> Is this Mac specific?

Basically, yes. Because Windows and Linux don't use DOM_DELTA_PIXEL at dispatching WheelEvent. However, I'm not sure the gesture message case on Windows.

Comment 13

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

Attachment: testcase for scroll by default action

Comment 14

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

Attachment: testcase for scroll by pixel scroll event handling

Comment 15

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

Attachment: testcase for scroll by wheel event handling

Comment 16

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

Attachment: Patch

Since I cannot test this with Retina display, could you test this patch?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8a2e179d2b89/try-macosx64/firefox-30.0a1.en-US.mac.dmg

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8a2e179d2b89

This patch computes CSS pixels via app units at returing deltaX, deltaY and deltaZ.


FYI: This patch doesn't change the behavior of MozMousePixel event since the behavior with zoom has already been tested. We shouldn't change legacy event's behavior as far as possible.

If this patch works as I expected, some WheelEvent tests may fail on Retina display environment but I cannot know where I need to fix if tryserver machines are not in HiDPI mode.

Comment 17

[Evan Wallace [:evanw]]

Wow, our app feels so much better now! The scroll quantum in the new build is correctly 1 for a zoom level of 100%, but is different than both Chrome and Firefox for other zoom levels:

Scroll quantum for zoom levels of 50%, 100%, 150%, and 200% on a retina display:
- Chrome: 1, 1, 1, 1
- Unpatched Firefox: 2, 2, 2, 2
- Patched firefox: 2, 1, 0.6666666666666666, 0.5

Comment 18

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

(In reply to Evan Wallace from comment #17)
> Wow, our app feels so much better now! The scroll quantum in the new build
> is correctly 1 for a zoom level of 100%, but is different than both Chrome
> and Firefox for other zoom levels:
> 
> Scroll quantum for zoom levels of 50%, 100%, 150%, and 200% on a retina
> display:
> - Chrome: 1, 1, 1, 1
> - Unpatched Firefox: 2, 2, 2, 2
> - Patched firefox: 2, 1, 0.6666666666666666, 0.5

Thank you for the tests. The zoomed case behavior is expected behavior. What is the best behavior is difficult issue. If we assume that the wheel event is fired from like touch screen, the patched build's behavior must be better. Anyway, that's the default behavior's scroll amount in most cases.

If other applications of Mac like Word, Photoshop and something having zoom feature released by Apple changes the scroll speed by zoom level, please file another bug and cc me.

Comment 19

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

Comment on attachment 8387447 [details] [diff] [review]
Patch

You can see the test, I keep the current behavior of MozMousePixelScroll event's detail value at zoom cases because it was tested by initial implementation, IIRC. I.e., the detail value is always CSS pixels. I believe that we have no reasons to change such legacy event's behavior for compatibility with older build.

On the other hands, the standard event, WheelEvent's delta values should be CSS pixels but represents default action's scroll amount as far as possible. Our default action currently uses the pixels as device pixels. I.e., if content is zoomed in or out, the scroll amount becomes smaller or larger. I believe that this is better behavior. However, if we need to change this behavior for same UX as other native applications, we should change it in another bug.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/797110c218e3