1230176 - Weirdness at end of Fennec fling. Don't send duplicate DOM 'scroll' event for sub-pixel scrolling

Status: REOPENED

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

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Oh bostonglobe.com articles, they have a fixed navbar at the top of the page that is shown when you pan up. If you fling down, it is hidden, but then reappears when the fling completes (without any input) -- presumably because we actually scrolled up some, which is wrong.

Comment 1

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

... or maybe they're just detecting that the scroll stopped using setTimeouts and scroll event listeners?

Comment 2

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

Hm, I guess this didn't happen with JPZC so it is a regression from the C++ code. Interesting.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

I'm guessing they have a scroll listener and detect when you go up. We must be doing that a small amount at the end. Rounding error maybe?

Comment 4

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

I used WebIDE to add a scroll listeners and dump out document.documentElement.scrollTop. It looks like the scroll offset is always non-decreasing, but sometimes there are multiple events at the last scroll offset. e.g.:

740
741
742
743
744
744
745
745

And sometimes there are not:

1089
1090
1091
1092
1093
1093
1094

In the former case the toolbar comes down, in the latter case it does not. Methinks they are using a >= instead of a > somewhere. But we probably shouldn't be sending duplicate events to begin with.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yup, that definitely sounds plausible.

Comment 6

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

Actually, I'm pretty sure this has to be evangelism. We fire the scroll event if the scroll position in app units changes, even if it rounds to the same CSS pixels. We have to do that because the app unit scroll position is detectable by web content, via getBoundingClientRect() for example. If we didn't fire the scroll event there would be no notification to content if that changed.

See for example the two values of document.documentElement.getBoundingClientRect() below, both of which correspond to document.documentElement.scrollTop = 823:

DOMRect { x: 0, y: -822.5, width: 384, height: 7177.61669921875, top: -822.5, right: 384, bottom: 6355.11669921875, left: 0 }
DOMRect { x: 0, y: -823, width: 384, height: 7177.61669921875, top: -823, right: 384, bottom: 6354.61669921875, left: 0 }

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Interesting! Mike Taylor to the rescue? (comment #29)

Comment 8

[Karl Dubost💡 :karlcow]

We dealt with another issue about BostonGlobe. Just yesterday.
https://webcompat.com/issues/1957

This bar shows up only on articles not on the homepage, for example
http://www.bostonglobe.com/news/politics/2015/11/04/taxachusetts-lie-here-why/RSpMkAM9de7uh422nV0oIJ/story.html

The thing is that I'm not sure there is a bug here. 
The behavior is consistent with what Chrome on Android is doing. Or is there something that I'm not seeing?

Comment 9

[Mike Taylor [:miketaylr]]

> The behavior is consistent with what Chrome on Android is doing. Or is there something that I'm not seeing?

Probably yes. To reproduce:

1) Open bostonglobe.com and click on an article
2) (Gently?) scroll down the page with a flick of your finger. Try a few times if you don't see...

Expected:
You shouldn't ever see the menu bar.
Actual:
Sometimes the floating menu bar appears, or you'll see a tiny flickr that's 1 or 2px tall at the top of the viewport.

The menu bar should only appear when you scroll up, which is the behavior I see in Chrome Mobile (beta and release).

Comment 10

[Mike Taylor [:miketaylr]]

But let's find the code so we know how to suggest a fix.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

The weirdness I was seeing before is gone now, most likely because our fling stop threshold does not allow a sub-pixel scroll.

Comment 12

[Mike Taylor [:miketaylr]]

Snorp, you don't see stuff like this anymore? https://www.youtube.com/watch?v=TiAf4GBjt2M

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Mike Taylor [:miketaylr] from comment #12)
> Snorp, you don't see stuff like this anymore?
> https://www.youtube.com/watch?v=TiAf4GBjt2M

I didn't when I made that comment, but I just tried what you do in the video and I see that. Guess we still have some weirdness.

Comment 14

[Mike Taylor [:miketaylr]]

So they have a jQuery plugin called stickyScroll: https://gist.github.com/miketaylr/0318c2640e0b4578109f#file-globe-js-L665-L718

https://gist.github.com/miketaylr/0318c2640e0b4578109f#file-globe-js-L984-L992 is where they're showing the menu on "up scroll".

One suggestion could be for them to check for position: sticky support via `window.CSS.supports('position', 'sticky')` and behave accordingly. But that's a very hand-wavey suggestion. :/

Comment 15

[Karl Dubost💡 :karlcow]

ok thanks Mike. Seen. I had not notice because I'm not concerned that such as small issue normally. ^_^ Sorry about that. So I guess the jumping at the bottom of the screen 1 of 5 free articles is also an issue.

Comment 16

[Karl Dubost💡 :karlcow]

StickyScroll
 Posted 5 years ago by Rick Harris	
http://www.orangecoat.com/stickyscroll

There is a github page for it
https://github.com/rickharris/stickyscroll

With
>  Not currently working or actively developed: A jQuery plugin for creating elements that 'stick' to the top of the window when scrolling down the page 

And https://github.com/rickharris/stickyscroll#warning
> Stickyscroll performs very badly in the current versions of Chrome and Firefox at the time of this writing, and I'm not sure I'm capable of fixing it without stripping it down to next to nothing. So, development is not very active right now.


Latest commit May 2011
https://github.com/rickharris/StickyScroll/commits/master

Latest issue Nov 2014 
https://github.com/rickharris/StickyScroll/issues

5 Pull requests from 2012-2014 in the queue
https://github.com/rickharris/StickyScroll/pulls

Not very encouraging.
At least Rick Harris is still alive
https://github.com/karma-runner/karma-testingbot-launcher/commit/b966a3a0b1278a4ce2ecbb248600b52050c3596f

Comment 17

[Mike Taylor [:miketaylr]]

I don't think that lib is actually the one they're using (I had searched as well) -- the source is different if you compare.

(A jQuery "plugin" is just a method on the jQuery prototype, which has an alias "fn", so $.fn.stickyScroll is them defining their own plugin). Based on the globe specific code in the plugin, I'm guessing it's something they made themselves: 

`if(globe.section === "todays-paper"){`

Comment 18

[Karl Dubost💡 :karlcow]

ok. understood. Let me try.
I pinged one of the devs 
https://webcompat.com/issues/1957#issuecomment-175951513

Comment 19

[Botond Ballo [:botond]]

(In reply to Karl Dubost :karlcow from comment #18)
> I pinged one of the devs 
> https://webcompat.com/issues/1957#issuecomment-175951513

The latest comment in that issue says the problem is now fixed.

Snorp, could you confirm that you don't see any more weirdness on the site?

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yup, seems to work now.

Comment 21

[Thomas Wisniewski]

Attachment: testcase.html

So both Firefox and Chrome will not fire two scroll events when you try to trigger it with script like this:

>  elem.scrollTop = 150;
>  elem.scrollTop = 150.5;

However if you zoom into a page to make it easy to do "subpixel" scrolls, it's easy to see the problem in Firefox for Android.

That's what my attached testcase does; it shows a zoomed-in page and displays the values of window.scrollY as you scroll around.

Chrome for Android doesn't show duplicates, but Firefox does. I can't get desktop Firefox or Chrome to exhibit the problem with my mouse, but perhaps I just can't zoom in enough to generate "subpixel" scrolls (and don't have a touchscreen monitor to test with either).

Comment 23

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

I believe the fix here would be to not do the call to PostScrollEvent [1] if "pt" and "curpos" resolve to the same CSS pixel coordinates. We already have an early-exit if they are the same app unit values [2] but for subpixel scrolls we still end up calling PostScrollEvent.

Should be a pretty straightforward fix, and the attached testcase can be trivially turned into a mochitest. The hard part would be debugging failures from any existing tests and/or ensuring it doesn't regress other web content.

(I don't plan on fixing this in the immediate future, but posting the above in case anybody else wants to take a crack at it)

[1] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/generic/nsGfxScrollFrame.cpp#2918
[2] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/layout/generic/nsGfxScrollFrame.cpp#2771

Comment 24

[Chris Peterson [:cpeterson]]

I am tagging some Fennec bugs related to Chrome scrolling parity.

Comment 25

[Chris Peterson [:cpeterson]]

[geckoview:klar:p3] because this is not a Klar+GeckoView blocker. We might want to revisit this bug for Fenix.

Comment 26

[Chris Peterson [:cpeterson]]

glamon (UX) says he was unable to reproduce this in Fennec 61 or at least it is not noticeable.

Comment 27

[Firefox Bug Husbandry Bot]

See bug 1547409. Moving webcompat whiteboard tags to keywords.

Comment 28

[[ex-Mozilla] Agi Sferro | :agi]

From my test the toolbar disappears on Chrome but not on Fenix. I wonder what's going on.

Comment 29

[Dennis Schubert [:denschub]]

We don't know if this is still an issue or not. If you see a broken site with this, please re-set the webcompat-priority flag.