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

Status: RESOLVED FIXED

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

Description

[Jim Mathies [:jimm]]

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.

Comment 1

[Jim Mathies [:jimm]]

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.

Comment 2

[Brian R. Bondy [:bbondy]]

I think we'll need the gfx team's help for this.

Comment 3

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

Generally sounds like the displayport heuristics need tuning, but I can't be sure without reproducing and debugging.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 8

[Jim Mathies [:jimm]]

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.

Comment 9

[Jim Mathies [:jimm]]

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

Comment 10

[Jim Mathies [:jimm]]

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.

Comment 11

[Jim Mathies [:jimm]]

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.

Comment 12

[Jim Mathies [:jimm]]

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.

Comment 14

[Jim Mathies [:jimm]]

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.

Comment 15

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

(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.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 17

[Jim Mathies [:jimm]]

Attachment: basic wheel support (wip)

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.

Comment 18

[Jim Mathies [:jimm]]

Attachment: basic wheel support (wip)

Comment 20

[Jim Mathies [:jimm]]

Attachment: simple fix

Comment 21

[Jim Mathies [:jimm]]

Attachment: cleanup and comments

Comment 22

[Jim Mathies [:jimm]]

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%.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d439b0f78fe9
https://hg.mozilla.org/mozilla-central/rev/f9208ba78a22

Comment 24

[Kapranov]

On the home page when you scroll horizontally sometimes appear black stripes..

Comment 25

[Jim Mathies [:jimm]]

(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?

Comment 26

[Kapranov]

(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

Comment 27

[Jim Mathies [:jimm]]

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).

Comment 28

[Kapranov]

(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

Comment 29

[Martin lesinsky]

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. )