Closed Bug 907100 Opened 6 years ago Closed 6 years ago

Content doesn't update properly when scrolling with mouse wheel or auto-scroll

Categories

(Firefox for Metro Graveyard :: Pan and Zoom, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: jimm, Assigned: jimm)

References

()

Details

Attachments

(3 files, 1 obsolete file)

STR:

1) open a scrollable page
2) pan down a section of the page in a short quick motion

result: the top of the page that is scrolled into view will briefly be rendered as a black rectangle.

tested on a Surface Pro.
That was missing some steps - 

1) open a scrollable page
2) pan down the page a bit
3) flick the page back to the top in a short quick motion

This doesn't reproduce 100% of the time.
Summary: Top of page renders black when scrolling down → Top of page renders black when scrolling up
I think we'll need the gfx team's help for this.
Generally sounds like the displayport heuristics need tuning, but I can't be sure without reproducing and debugging.
Depends on: 907179
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Generally sounds like the displayport heuristics need tuning, but I can't be
> sure without reproducing and debugging.

I was just in there looking at those settings, seems reasonable. Also this is on a high dpi system, so some of the values are off. Filed bug 907243.
Duplicate of this bug: 914650
Blocks: 915723
No longer blocks: metro-apzc
Duplicate of this bug: 903429
Assignee: nobody → jmathies
Duplicate of this bug: 920442
We have a couple different problems here. First there's a problem with redraw associated with touch input, which is hard to reproduce but if you really fling content around you'll likely see it. 

The second problem involves scrolling using input methods the apz doesn't handle. The two types of input I've found that have this issue are scroll wheel and auto scroll. In both cases, content is scrolled via dom events delivered to the scrollable frame. This in turn comes back to the apz through UpdateScrollOffset as if it were a scrollBy via content script. In this case, it looks like the scroll offset gets updated but a redraw isn't triggered in time, or the display port is calculated incorrectly leaving areas that aren't painted.

Oddly enoungh this second issue is only visible on the top of the page for simple text pages. With pages that have lots of overlaying divs, it shows up in various places (www.marketwatch.com is a good example).

Independent of the touch scrolling issue, I'm wondering if we are we handling alternate scroll input (scroll wheel and auto-scroll) correctly here or if we should find a way to route this input through the apz as we do with touch.
Kind of interesting how all of this happens currently. There's a lot of overhead that could be optimized out over time. Basically:

1) widget fires dom wheel events
2) dom event get processed by content triggers a scroll event on the browser
3) browser content script picks up the scroll event in ContentScroll [1]
4) ContentScroll sends an async scroll event back to chrome [2]
5) chrome fires an observer down to widget comunicating that a scroll took place in content
6) widget tells the apz about this via UpdateScrollOffset

At which point a repaint is sort of hit or miss. The missing part here is that the display port hasn't been updated on browser. That has to be done up on front end code and is usually triggered by an apz call to RequestContentRepaint, which through widget sends an observer up to chrome, which forward an async event over to content, which calls winutils setDisplayPortForElement.

whew.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.js#550
[2] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/apzc.js#138
I think we could call RequestContentRepaint in UpdateScrollOffset, however this will ultimately cause content to call scrollBy on the browser, and that will shut down smooth scrolling, which we don't want. I think we might be able to shorten this up a bit by updating the display port when the first scroll is received by content. That way we can avoid RequestContentRepaint.
An alternative here which I'm still kicking around would be to add support for scroll wheel scrolling in apz. Auto-scroll is more tricky, we'd probably have to disable toolkit support for it in <browser> and re-implement it in the layers gesture recognizer code.
Summary: Top of page renders black when scrolling up → Content doesn't update properly when scrolling with mouse wheel or auto-scroll
I think I'll take a shot at getting the apz hooked up to nsIDOMWheelEvents. Auto-scroll we can disable until we find the time to support it.
No longer depends on: 907179
Blocks: 920687
Duplicate of this bug: 915690
Blocks: 921020
No longer blocks: 920687
No longer blocks: 915723
We have a great deal of legacy logic built into our handling a scroll wheel events. Since initially we'll be using this in metrofx I'm not too concerned about most of this, but in time we'll have to make some decisions on what we support when we add apzc support to the desktop browser.
Blocks: 920687
(In reply to Jim Mathies [:jimm] from comment #11)
> An alternative here which I'm still kicking around would be to add support
> for scroll wheel scrolling in apz. Auto-scroll is more tricky, we'd probably
> have to disable toolkit support for it in <browser> and re-implement it in
> the layers gesture recognizer code.

I would like to route all scroll-triggering input through APZC. This includes scroll wheel. To implement this, I imagine adding additional gesture types to widget/InputData.h (e.g. MOUSEWHEEL_INPUT) and sending instances of those to the APZC when you receive the corresponding input in MetroWidget. APZC would need to add support for handling these kinds of events and doing the appropriate thing. We can also implement auto-scroll in APZC; we did something similar in Fennec for gamepad input that we will need to port anyway so I don't think it will be a lot of additional work beyond that.

(In reply to Jim Mathies [:jimm] from comment #14)
> We have a great deal of legacy logic built into our handling a scroll wheel
> events. Since initially we'll be using this in metrofx I'm not too concerned
> about most of this, but in time we'll have to make some decisions on what we
> support when we add apzc support to the desktop browser.

Do you have any examples as to what kind of legacy logic we're dealing with here? But yes we should defer these decisions to when we switch the desktop browser over to APZC.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> (In reply to Jim Mathies [:jimm] from comment #11)
> > An alternative here which I'm still kicking around would be to add support
> > for scroll wheel scrolling in apz. Auto-scroll is more tricky, we'd probably
> > have to disable toolkit support for it in <browser> and re-implement it in
> > the layers gesture recognizer code.
> 
> I would like to route all scroll-triggering input through APZC. This
> includes scroll wheel. To implement this, I imagine adding additional
> gesture types to widget/InputData.h (e.g. MOUSEWHEEL_INPUT) and sending
> instances of those to the APZC when you receive the corresponding input in
> MetroWidget. APZC would need to add support for handling these kinds of
> events and doing the appropriate thing. We can also implement auto-scroll in
> APZC; we did something similar in Fennec for gamepad input that we will need
> to port anyway so I don't think it will be a lot of additional work beyond
> that.
> 

I'm working on this now - routing WidgetWheelEvents to the apz. Some of the scroll messages triggered by keyboard shortcuts will likely ride along with this since we convert them to wheel events in widget code.

> (In reply to Jim Mathies [:jimm] from comment #14)
> > We have a great deal of legacy logic built into our handling a scroll wheel
> > events. Since initially we'll be using this in metrofx I'm not too concerned
> > about most of this, but in time we'll have to make some decisions on what we
> > support when we add apzc support to the desktop browser.
> 
> Do you have any examples as to what kind of legacy logic we're dealing with
> here? But yes we should defer these decisions to when we switch the desktop
> browser over to APZC.

Lots and lots of customization through prefs. I'm trying to make reasonable trade off decisions here since metro isn't widely used yet. We can do follow up work for desktop based on feedback from users. 

mousewheel.transaction.*
mousewheel.system_scroll*
general.smoothScroll.*
mousewheel.acceleration.*
mousewheel.min_line_scroll_amount
mousewheel.*.delta_multiplier_*
mousewheel.default.*
mousewheel.with*

https://wiki.mozilla.org/index.php?title=Gecko:Mouse_Wheel_Scrolling
No longer blocks: 921020
Blocks: 915723
No longer blocks: 920687
Attached patch basic wheel support (wip) (obsolete) — Splinter Review
This is a rollup of the work I've been doing to get scroll wheel piped directly into the apz. A few notes on this - 

Any wheel event can be cancelled by content, so to detect wheel handlers I've added a peek helper to dom window. Unfortunately this puts us in a situation where wheel events on pages with listeners need to be delivered directly to content instead of the apz. Scrolling in this case must take the long route through front end code before it can be delivered back to the apz via a scroll offset update. 

This sucks because we end up with two different methods of processing wheel deltas. The dom path supports our old school smooth scroll and all of the customization prefs that go along with our legacy scroll wheel support. The apz route on the other hand supports a newer form of smooth scroll implemented by the apz. The two do not match behaviors perfectly, even if we were to get all of the customization nailed down in the apz. I think we can get the two working similarly, but that is going to take a lot of tweaking, or possibly updating how the dom scroll works such that it delivers events more directly to the apz without actually doing any content scrolling. Alternatively we could just live with discrepancies. I'm not sure our users will accept that.

The other issue I've running into with these patches is that our frame rates for dom scroll / repaint are actually better than with the apz/compositing. This might be a side effect of the way I've implemented this, not sure. But I do get better smooth scroll when going through the dom.

I've spent enough time on this for now. Since we still occasionally go through dom, we still have to solve the paint problems that we have with the apz and dom scrolling (which this bug originally was filed about) so I am going to take a break from this work, and concentrate on getting the paint problems worked out first. I'll come back to this when I have more time.
Attachment #821257 - Attachment is obsolete: true
Duplicate of this bug: 920687
Attached patch simple fixSplinter Review
Attachment #821771 - Flags: review?(mbrubeck)
Attachment #821773 - Flags: review?(mbrubeck)
Attachment #821773 - Flags: review?(mbrubeck) → review+
Attachment #821771 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/integration/fx-team/rev/f9208ba78a22
https://hg.mozilla.org/integration/fx-team/rev/d439b0f78fe9

Unfortunately testing in a release build I still see a little black at the edges from wheel scroll, so I'm not convinced this is fixed 100%.
Whiteboard: [leave-open]
Assignee: jmathies → nobody
On the home page when you scroll horizontally sometimes appear black stripes..
(In reply to Kapranov from comment #24)
> On the home page when you scroll horizontally sometimes appear black
> stripes..

scrolling via mouse, keybaord, or touch?
(In reply to Jim Mathies [:jimm] from comment #25)
> (In reply to Kapranov from comment #24)
> > On the home page when you scroll horizontally sometimes appear black
> > stripes..
> 
> scrolling via mouse, keybaord, or touch?

Mouse
So I haven't determined if the remaining issue is keyboard/mouse (long trip) related, or if what's left is related to touch scroll paint problems (bug 915811).
(In reply to Jim Mathies [:jimm] from comment #27)
> So I haven't determined if the remaining issue is keyboard/mouse (long trip)
> related, or if what's left is related to touch scroll paint problems (bug
> 915811).

Yes, it seems this is a bug 915811. On Web sites, all is excellent
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
Assignee: nobody → jmathies
Target Milestone: --- → Firefox 27
No longer blocks: 915723
Blocks: metro-apzc
reproducible on dell xps 12 win8 pro

- firefox-29.0a1.en-US.win32
- page - https://www.origin.com/fi-fi/store/buy/fifa-14/pc-download/base-game/standard-edition-ANW.html

( with quick scroll down dislpey turn into black. )
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.