657401 - Page flickers once when scroll to top by dragging thumb of scrollbar. (disabled Hardware acceleration)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/6e4fb61ef475
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110516 Firefox/6.0a1 ID:20110516030622

If I disabled Hardware acceleration.
Page flickers once when scroll to top by dragging thumb of scrollbar.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile + Disable Hardware acceleration
2. Open URL
3. Scroll up about 200px (in order to hide header/menu part)
4. Scroll to top by dragging thumb of scrollbar

Actual Results:
 Page flickers once.

Expected Results:
 Page should not flicker.

Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/a28f4e72593a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110429 Firefox/6.0a1 ID:20110429151036
Fails:
http://hg.mozilla.org/mozilla-central/rev/8a650b9e55db
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110429 Firefox/6.0a1 ID:20110429152220
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a28f4e72593a&tochange=8a650b9e55db
Triggered by:
8a650b9e55db	Zack Weinberg — Bug 651498: call gfxPlatform::Init when necessary and not earlier. r=joedrew,bsmedberg

Comment 1

[Kai Liu]

Between bug 651017 (2011-04-19) and bug 651498 (2011-04-29), HWA could not be disabled because some gfx prefs were not being read and applied, so the build right before bug 651498 actually had HWA enabled.

Comment 2

[Alice0775 White]

(In reply to comment #1)
> Between bug 651017 (2011-04-19) and bug 651498 (2011-04-29), HWA could not
> be disabled because some gfx prefs were not being read and applied, so the
> build right before bug 651498 actually had HWA enabled.

OK
Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/0e8c23e50c6c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110418 Firefox/6.0a1 ID:20110418195729
Fails:
http://hg.mozilla.org/mozilla-central/rev/fc1ed658bf4b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110418 Firefox/6.0a1 ID:20110418201130
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0e8c23e50c6c&tochange=fc1ed658bf4b
Triggered by:
Bug 641426 - Unify point/size/rect types

This also happens on Ubuntu10.04(it is slightly difficult to reproduce, it need repeat step 3 and step4)
http://hg.mozilla.org/mozilla-central/rev/6e4fb61ef475
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110516 Firefox/6.0a1 ID:20110516030622

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Reduced testcase

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is, after we've fixed bug 647560, that ChildrenPartitionVisibleRegion assumes that BasicLayers don't paint outside the bounds of their visible region, but this is not true in general. In this testcase, there's an L-shaped ThebesLayer that paints its whole area. But the same problem could conceivably arise for ImageLayers and other layers that don't clip to their visible region. So we need to make BasicLayers clip to their visible regions in some cases, at least when they have no ancestor doing double-buffering for them.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

I don't think we can test this double-buffering code currently.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch applies on top of the patches for bug 637852 and bug 647560.

Comment 7

[Timothy Nikkel (:tnikkel)]

I'm curious about how bug 641426 caused this.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know.

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 532930 [details] [diff] [review]
fix

-   * in response to this method call. aContext will already have been
+   * in response to this method call. aContext will already have been1593

Seems unintended.

+  PRBool ForceClipToVisibleRegion() { return mClipToVisibleRegion; }

I think this should be called GetClipToVisibleRegion or something. ForceClipToVisibleRegion makes it sound like it is doing something, ie forcing us to clip to the visible region.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Done.

Comment 11

[Asa Dotzler [:asa]]

roc, can you (or one of the reviewers here) help release drivers make the approval-aurora? call with a description of the benefit of taking this change and what kind of risk is here?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The change would need to be rebased so it doesn't depend on bug 647560, which is not trivial.

Personally I think this regression is very minor. AFAIK we have only Alice's report, and no duplicates. I'm not really worried if it ships in FF6. Alice, what do you think?

Comment 13

[Alice0775 White]

This is a minor issue.I agree with comment #12.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 532930 [details] [diff] [review]
fix

agreeing with alice and roc. not for aurora.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d63d5764397c

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d63d5764397c

Comment 17

[al_9x]

Please take a look at Bug 684032 - continuous pronounced flickering when loading and scrolling in 7.0 (thunderbird and firefox)

The effects that I am observing, though intermittent, are far more pronounced and unpleasant than what's described here (a single flicker), but it's likely that this fix addresses them.  It doesn't make sense to release 7.0 with such a significant visual regression, it's going to generate many negative impressions.

Because I can repro reliably, on demand, only in TB, I would need a TB build with this fix, to confirm it does in fact take care of the problem.

Comment 18

[al_9x]

Please comment on Bug 684032.  Would it not make sense to address those symptoms for 7.0 releases?  You should be able to repro easily with TB, or watch the attached video.

Comment 19

[al_9x]

I confirmed that this patch resolves the symptoms described in Bug 684032, which are more severe than in this one, so this fix should be in 7.0.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 532930 [details] [diff] [review]
fix

Renominating based on new information about the scope of the problem...

If we don't take this for Firefox, is this something Thunderbird should consider taking on their relbranch?  :(

Comment 22

[LegNeato]

Comment on attachment 532930 [details] [diff] [review]
fix

Clearing the aurora request flag as it looks like this landed on central before the aurora8 uplift (please request again if we're wrong).

Denying for beta due to the rebasing / additional dependencies needed. If TB wants this (as it looks like a bigger issue) they can take it on a relbranch.

Please request approval again with justification if you disagree.

Comment 23

[al_9x]

(In reply to Christian Legnitto [:LegNeato] from comment #22)
>
> Denying for beta due to the rebasing / additional dependencies needed. If TB
> wants this (as it looks like a bigger issue) they can take it on a relbranch.
> 

Why is it a bigger issue for TB?  How often does one open web pages in TB (vs. Fx)?  Did you watch the screencap video?  The same problem happens intermittently in Fx, it makes it seem alpha quality.  Such regressions should not be acceptable, considering that staying with 6.0 is not an option.

Comment 24

[al_9x]

The following symptom is possibly related and still present in 8.0:

1. do a Google maps search
2. as you mouse over the results on the left, the corresponding pushpin in the map expands.  The pushpin flickers as it expands.  
3. Panning the map slightly, gets rid of the flicker, as you mouse-over again, the pushpin expands smoothly.

Comment 25

[Timothy Nikkel (:tnikkel)]

You should file a new bug and test in a nightly for that issue.

Comment 26

[al_9x]

(In reply to Timothy Nikkel (:tn) from comment #25)
> You should file a new bug and test in a nightly for that issue.

Bug 695891 - Google maps pushpins flicker (not always) as they expand on results mouse over