Open Bug 1230176 Opened 9 years ago Updated 2 years ago

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

Categories

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

Unspecified
Android
defect

Tracking

()

REOPENED
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- ?

People

(Reporter: snorp, Unassigned)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [apz-evangelism] [sitewait] [gfx-noted] [geckoview:klar:p3])

Attachments

(1 file)

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.
... or maybe they're just detecting that the scroll stopped using setTimeouts and scroll event listeners?
Hm, I guess this didn't happen with JPZC so it is a regression from the C++ code. Interesting.
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?
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.
Yup, that definitely sounds plausible.
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 }
Interesting! Mike Taylor to the rescue? (comment #29)
Flags: needinfo?(miket)
Flags: needinfo?(miket)
Whiteboard: [apz-evangelism]
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?
Flags: needinfo?(miket)
Whiteboard: [apz-evangelism] → [apz-evangelism] [needsdiagnosis]
> 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).
Flags: needinfo?(miket)
But let's find the code so we know how to suggest a fix.
The weirdness I was seeing before is gone now, most likely because our fling stop threshold does not allow a sub-pixel scroll.
Snorp, you don't see stuff like this anymore? https://www.youtube.com/watch?v=TiAf4GBjt2M
Flags: needinfo?(snorp)
(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.
Flags: needinfo?(snorp)
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. :/
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.
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
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"){`
ok. understood. Let me try.
I pinged one of the devs 
https://webcompat.com/issues/1957#issuecomment-175951513
Whiteboard: [apz-evangelism] [needsdiagnosis] → [apz-evangelism] [sitewait]
(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?
Flags: needinfo?(snorp)
Yup, seems to work now.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(snorp)
Resolution: --- → WORKSFORME
Attached file 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).
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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
tracking-fennec: --- → ?
I am tagging some Fennec bugs related to Chrome scrolling parity.
Component: Toolbar → Panning and Zooming
Keywords: parity-chrome
OS: Unspecified → Android
Product: Firefox for Android → Core
Summary: Weirdness at end of fling → Weirdness at end of Fennec fling. Don't send duplicate DOM 'scroll' event for sub-pixel scrolling
Whiteboard: [apz-evangelism] [sitewait] → [apz-evangelism] [sitewait] [geckoview:klar]
Priority: -- → P3
Whiteboard: [apz-evangelism] [sitewait] [geckoview:klar] → [apz-evangelism] [sitewait] [geckoview:klar][gfx-noted]
Depends on: 1448439
[geckoview:klar:p3] because this is not a Klar+GeckoView blocker. We might want to revisit this bug for Fenix.
Whiteboard: [apz-evangelism] [sitewait] [geckoview:klar][gfx-noted] → [apz-evangelism] [sitewait] [gfx-noted] [geckoview:klar:p3]
glamon (UX) says he was unable to reproduce this in Fennec 61 or at least it is not noticeable.
No longer depends on: 1448439
Keywords: parity-chrome

See bug 1547409. Moving webcompat whiteboard tags to keywords.

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

tracking-fennec: ? → ---
See Also: → 1682203
Webcompat Priority: --- → ?

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.

Webcompat Priority: ? → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: