588403 - Auto-scroll can cause rendering artifacts

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matheus Kerschbaum]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100817 Minefield/4.0b5pre
Build Identifier: 

With patches from Bug 130078, auto-scroll (middle click scrolling) "grabs" whatever surface it was clicked on and renders it somewhere else. http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox is a good place to test this.

Reproducible: Always

Steps to Reproduce:
1.Middle click on a green build
2.Scroll to somewhere else
Actual Results:  
The green part around middle-click is rendered wherever you scroll to.

Expected Results:  
After scrolling off the green build, middle-click should have removed the green part around it.

I'm not using the build from Bug 130078 atm, but I can grab it if a screenshot is needed here.

Comment 1

[Timothy Nikkel (:tnikkel)]

I first started seeing this after retained layers landed.

Comment 2

[Timothy Nikkel (:tnikkel)]

I don't see this anymore. Can you try this build http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2050d6216b77/ ? 

Probably fixed by bug 588407. Transparent surface over transparent surface maybe doesn't get along so well.

Comment 3

[Matheus Kerschbaum]

(In reply to comment #2)
> I don't see this anymore. Can you try this build
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2050d6216b77/
> ? 
> 
> Probably fixed by bug 588407. Transparent surface over transparent surface
> maybe doesn't get along so well.

Confirming fixed in latest build.

Comment 4

[Timothy Nikkel (:tnikkel)]

Should be fixed by bug 588407.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm seeing this with the latest tryserver builds posted today.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-cf64eea45e07/

Comment 6

[Timothy Nikkel (:tnikkel)]

Weird. It definitely was fixed, but now is back.

Comment 7

[Timothy Nikkel (:tnikkel)]

The link I posted to the try server build (2050d6216b77) was incorrect, that build did not have 130078. So this was never fixed.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

It isn't just the autoscroll icon that causes this. If you have a noautohide, noautofocus panel element overlaid from an icon in the statusbar, scrolling with that panel open causes the same kind of tearing around the panel. (eg, Echofon's primary panel for showing tweets: If you scroll a webpage while it's open, the space behind the panel (and a bit around it ((just to the right?)) shows the pre-scroll page content.

Comment 9

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

I don't see any problem with the autoscroll icon on Mac.

Comment 10

[Timothy Nikkel (:tnikkel)]

Latest try server build still has the problem.

Comment 11

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

I see this on Windows 7.

Comment 13

[Alice0775 White]

See Bug 591585
It happens not only on Aero but also non Aero style of Windows7.
It also happens on WindowsXP.
It does not happen on Linux.

Comment 14

[[not reading bugmail]]

I thought it was just transparent before now its like half transparent on the left side and half picking up a square the size of the autoscroll widget.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: screenshot

Opening the bookmarks sidebar also does similar things. Not sure if these would all have the same fix.

Comment 16

[Matheus Kerschbaum]

(In reply to comment #15)
> Created attachment 470196 [details]
> screenshot
> 
> Opening the bookmarks sidebar also does similar things. Not sure if these would
> all have the same fix.

That's pretty bad. I'll nominate this as blocking then.

Comment 17

[pal-moz]

(In reply to comment #15)
> Created attachment 470196 [details]
> screenshot
> 
> Opening the bookmarks sidebar also does similar things. Not sure if these would
> all have the same fix.

cannot reproduce here.

Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100828 Firefox/4.0b5pre ID:20100828040640

Comment 18

[[not reading bugmail]]

(In reply to comment #17)
> (In reply to comment #15)
> > Created attachment 470196 [details] [details]
> > screenshot
> > 
> > Opening the bookmarks sidebar also does similar things. Not sure if these would
> > all have the same fix.
> 
> cannot reproduce here.
> 
> Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100828 Firefox/4.0b5pre
> ID:20100828040640

I can reproduce under regular mode and d2d the sidebar issue.  It happens on bugzilla pages for example.

Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100828 Firefox/4.0b5pre ID:20100828110247

Comment 19

[Timothy Nikkel (:tnikkel)]

I can't reproduce the sidebar issue, and from the surface it doesn't seem like it's related. Could we get a bug filed for the sidebar issue too?

Comment 20

[pal-moz]

Attachment: screenshot

sidebar issue happened, but different rendering.

[STR]
Open sidebar

Comment 21

[Timothy Nikkel (:tnikkel)]

Filed bug 591834 for the sidebar issue. Is there any reason to think they are the same issue?

Comment 22

[[not reading bugmail]]

It looks like the sidebar issue would be looked at in bug 591671, no?

Comment 23

[Timothy Nikkel (:tnikkel)]

Bug 591671 has different steps to reproduce. I can't say if they are both the same issue without investigation. Better to have it on file and resolve it as fixed by another bug then not tracking it at all.

Comment 24

[tx.perry.j]

I can confirm that I'm having a similar issue on both WinXP 32bit and Win7 64bit

Comment 25

[tx.perry.j]

Here's a 20second video showing the exact issue I've been having.
http://tinyogg.com/files/UtNnB.ogv

Comment 28

[Timothy Nikkel (:tnikkel)]

Hmm, even we I always return opaque white from PresShell::ComputeBackStopColor the autoscroll is still translucent, and the weird square below the autoscroll icon still exists. The glass at the top of the window gets drawn as white though, so it's definitely taking effect.

Comment 29

[Timothy Nikkel (:tnikkel)]

The layer tree's for both the autoscroll's widget and the main widget look okay though.

Comment 31

[Filip Zakrzewski]

Maybe there is something weird with personas implementation? Not with autoscroll's widget and the main widget

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #31)
> Maybe there is something weird with personas implementation? Not with
> autoscroll's widget and the main widget

It affects me regardless of whether I use a persona.

Comment 34

[Filip Zakrzewski]

Attachment: I think there is some problems with glass window or personas implementation of glass window

I think there is some problems with glass window or personas implementation of glass window

Better to check this, why it is unstable

Comment 35

[Timofei Shatrov]

After I updated to FF4 beta5 I'm having this bug on both my computers with Windows XP.

Comment 36

[Oskar Ivanić (:icecold)]

It happens to me with latest nightly build. (20100908)
Btw. I'm talking about middle click.

OS is Windows 7.

Comment 37

[Timothy Nikkel (:tnikkel)]

Comment 28 was wrong. I wasn't making it opaque properly.

When I make the autoscroll opaque the glitch is still there.

The position of the glitch rect relative to the cursor changes depending on if you have the menubar disabled or not.

Comment 38

[Timothy Nikkel (:tnikkel)]

nsWindow::GetBounds, nsWindow::mBounds, and the view/ seem to be a little confused for popup windows and I think this is causing the bug somehow.

Comment 40

[Timothy Nikkel (:tnikkel)]

nsView::CalcWidgetBounds expects popups to be in screen coordinates, not parent widget coordinates. As does ::SetWindowPos. But nsWindow::GetBounds tries to do something else. I'm still investigating.

Comment 42

[[e-mail]]

I still have this Problem on
Mozilla/5.0 (Windows NT 6.0; rv:2.0b5) Gecko/20100101 Firefox/4.0b5
But i hadnt had the bug in the 4th (and previous) beta of firefox 4

Comment 43

[Timothy Nikkel (:tnikkel)]

So the confusion with popup widgets being in screen coords vs relative to parent widget was a red herring. It just made the glitch offset from the autoscroll icon instead of directly under it. I'll fix that too.

The real problem is in nsViewManager::UpdateWidgetArea. We subtract the area of each child widget from the area we have to update on the parent widget. But if the child widget is transparent that is wrong, we still need to update that area.

We didn't see this bug without 130078 because the autoscroll was a child of the chrome widget but it only appears over the content area, so we didn't subtract its area from the area to update on the content widget.

It also wasn't visible until retained layers landed because we would blit most of the window when scrolling.

Comment 44

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Make nsWindow::GetBounds return in screen coordinates for popups (checked in)

Comment 45

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. In nsViewManager::UpdateWidgetArea needs to adjust for popup widgets.

Comment 46

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Don't subtract the area of transparent widgets from what we need to draw on the parent widget.

This actually fixes the bug.

Comment 47

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

Comment on attachment 474246 [details] [diff] [review]
Part 2. In nsViewManager::UpdateWidgetArea needs to adjust for popup widgets.

Why do we even want to recurse into popup widgets here? It seems to me we do not. Invalidates inside popups will invalidate the popup's view and then the popup's widget directly.

Comment 48

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

In fact, if we just skip popups there, that would take care of part 3 as well. We know that we don't create transparent child widgets.

Comment 49

[Timothy Nikkel (:tnikkel)]

Ok, so we can skip recursing on popup widgets. Removing the area to update from the parent widget when it is covered by a popup widget would still save some painting. Is it worth it?

Comment 50

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

I don't think so, and I don't even think it's correct on any platform where the window manager caches window contents.

Comment 51

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Ignore popup child widgets when updating.

Comment 52

[Jim Mathies [:jimm]]

(In reply to comment #44)
> Created attachment 474244 [details] [diff] [review]
> Part 1. Make nsWindow::GetBounds return in screen coordinates for popups.

Couldn't we make this the default behavior for all calls to GetBounds now that we've ditched child widgets?

Comment 53

[Timothy Nikkel (:tnikkel)]

There are still plugin widgets at least.

Comment 54

[Mats Palmgren (inactive)]

Comment on attachment 474332 [details] [diff] [review]
Part 2. Ignore popup child widgets when updating.

This shouldn't be necessary.  You shouldn't find eWindowType_popup
widgets on any child list.  Which platform did you see that on?

Comment 55

[Timothy Nikkel (:tnikkel)]

On Windows. I didn't actually look at the childlist pointers to verify, so I can double check if needed.

Comment 56

[Timothy Nikkel (:tnikkel)]

I know the parent pointer was set to our top level widget, and I don't see anything popup specific about how nsBaseWidget::AddChild is called.

Comment 57

[Mats Palmgren (inactive)]

nsWindow::mParent is set for all widgets on Windows, but it's not actually
used for anything (yet), it's just to hold a strong ref to wallpaper some
crash bugs.  GetParent() on Windows uses the native window ancestors to
lookup the nsIWidget parent, except for top-level widgets like
eWindowType_popup where we just return null directly.

Comment 58

[Mats Palmgren (inactive)]

(the plan is that GetParent() should use mParent eventually (bug 569583))

Comment 59

[Timothy Nikkel (:tnikkel)]

What I meant by the parent part was that the code in nsBaseWidget::BaseCreate at http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/nsBaseWidget.cpp#222 would then presumably add the popup widget as a child like any other.

Comment 60

[Mats Palmgren (inactive)]

It seems we have two kinds of eWindowType_popup widgets;
a top-level kind where GetParent()==nsnull and don't live on any child list
and a child kind that is added to its GetParent()'s child list.
I wasn't aware of that.  Ok, carry on...

Comment 61

[Timothy Nikkel (:tnikkel)]

On Windows nsWindow::GetParent() is very different from mParent, and in fact I think it returns null for popup widgets. But the mParent pointer is still the toplevel Gecko widget for the auto-scroll popup.

Comment 62

[Timothy Nikkel (:tnikkel)]

Landed part 2
http://hg.mozilla.org/mozilla-central/rev/73855a9ef1fd

This bug as reported should be effectively resolved, leaving open to get the popup widget coordinate fix in in some form.

Comment 63

[e_master9]

This bug also occurs on Windows XP, although this bug is filed under Windows 7.

Comment 64

[Timothy Nikkel (:tnikkel)]

Are you still seeing this bug? It should be fixed in 2010-09-12 nightlies.

Comment 65

[Filip Zakrzewski]

It's fixed for my Win7 x64.
I think that XP is fixed too.

Comment 66

[Sunil]

running 4.0b6 - still have the problem.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

The fix will be in b7.

Comment 68

[Sunil]

fixed in sep 15th nightly build.

Comment 70

[Timothy Nikkel (:tnikkel)]

Jim, you may have missed comment 53 in reply to your review comment.

Comment 72

[Jim Mathies [:jimm]]

(In reply to comment #70)
> Jim, you may have missed comment 53 in reply to your review comment.

(In reply to comment #53)
> There are still plugin widgets at least.

I always forget about those dastardly plugins. Wish I could!

Do we have some sort of STR I could use to reproduce what this second patch fixes?

Comment 73

[Jim Mathies [:jimm]]

(A test might be good too!)

Comment 74

[Timothy Nikkel (:tnikkel)]

(In reply to comment #72)
> Do we have some sort of STR I could use to reproduce what this second patch
> fixes?

I don't know of any problems that you can see that part 1 fixes. If part 2 is backed out then applying part 1 would move the rendering artifact from being a few pixels offset from the autoscroll, to being exactly under it.

I will try to put together a test for the main bug here, but I don't know if it will work or not.

Comment 78

[Alastair Robertson]

Bug 598583 is a duplicate of this, but I can't mark it as one. Would someone else be able to please?

Comment 80

[Aakash Desai [:aakashd]]

This is being seen by users on various flavors of Windows over Fx4.0 Beta 6: 

http://input.mozilla.com/en-US/search/?product=firefox&sentiment=sad&date_end=&date_start=&version=&q=middle+button+page+scroll

Comment 83

[Filip Zakrzewski]

@Aakash Desai - it is fixed in incoming Beta 7 :)

Comment 84

[RNicoletto]

(In reply to comment #83)
> @Aakash Desai - it is fixed in incoming Beta 7 :)

Not completely as per comment 53.

Comment 85

[Timothy Nikkel (:tnikkel)]

This should be completely fixed from a user perspective. If not please let me know.

I noticed something else that should be fixed while working on this (what comment 53 is about and why this bug is still open), but as far as I know it doesn't cause any problems that a user could see.

Comment 86

[Timothy Nikkel (:tnikkel)]

(In reply to comment #74)
> I will try to put together a test for the main bug here, but I don't know if it
> will work or not.

Jim, I thought about how to write a test for this bug, but with our current testing infrastructure there is no way to. We invalidate the correct area, but the view manager intercepts it and subtracts an area wrongly before passing it to the widget. The invalidation before the view manager is the only one we have access to from reftests. So the incorrect invalidation isn't available to reftests. So I think we should just land the code correction here and resolve this.

Comment 87

[[not reading bugmail]]

Would looking at this test help?
toolkit/content/tests/browser/browser_bug295977_autoscroll_overflow.js

Comment 88

[Timothy Nikkel (:tnikkel)]

That test checks that autoscrolling actually scrolls. We would need to test that the correct things get drawn to the screen.

Comment 91

[Jim Mathies [:jimm]]

(In reply to comment #86)
> (In reply to comment #74)
> > I will try to put together a test for the main bug here, but I don't know if it
> > will work or not.
> 
> Jim, I thought about how to write a test for this bug, but with our current
> testing infrastructure there is no way to. We invalidate the correct area, but
> the view manager intercepts it and subtracts an area wrongly before passing it
> to the widget. The invalidation before the view manager is the only one we have
> access to from reftests. So the incorrect invalidation isn't available to
> reftests. So I think we should just land the code correction here and resolve
> this.


Hmm, I don't think an api like GetBounds should return different values for different window types. Is there any way the caller could differentiate the window type and call separate apis on the widget?

Comment 92

[Timothy Nikkel (:tnikkel)]

I agree that the current API is not great. Is it reasonable to take a small bug fix to the current API then to redesign the API?

Comment 93

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #85)
> This should be completely fixed from a user perspective. If not please let me
> know.

What blocks marking the bug fixed?  (And should that be a blocker?)

Comment 94

[Timothy Nikkel (:tnikkel)]

There is a correctness fix that I named part 1, it doesn't fix anything that I know about. That part should not be a blocker.

Comment 95

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

The unfixed part of this bug doesn't block.

Comment 96

[Jim Mathies [:jimm]]

(In reply to comment #92)
> I agree that the current API is not great. Is it reasonable to take a small bug
> fix to the current API then to redesign the API?

Hmm well if it doesn't actually fix anything we know of, do we really need the fix? Honestly I'm cool either way, it just seemed a little odd doing that one off for popups here.

Comment 97

[Timothy Nikkel (:tnikkel)]

The reason I noticed this bug was because the viewmanager set the size of a popup window based on the dimensions of its view, then later it checked if the window had the right size based on the view dimensions, found it did not (when it really did) and called window->Resize again. I didn't see anything visible that was affected but that just seems wrong to have happening. Who knows it might fix some issue some one sees, or it might become important in the future.

Comment 98

[Jim Mathies [:jimm]]

Comment on attachment 474244 [details] [diff] [review]
Part 1. Make nsWindow::GetBounds return in screen coordinates for popups (checked in)

Ok, sounds good. Sorry for the long delay on this.

Comment 99

[Timothy Nikkel (:tnikkel)]

Thanks Jim.

I don't think this needs to land before FF4 ships. I'll land it after we branch.

Comment 100

[Boris Zbarsky [:bzbarsky]]

Timothy, do you want to land this or should I try to push it to cedar?  If the latter, is it just the last part1 attachment that needs landing?

Comment 101

[Timothy Nikkel (:tnikkel)]

Yes, just part 1 needs pushing, its been through try many times and should be fine. If someone wants to land this that would be great.

Comment 102

[(no longer active)]

Part 1 landed on cedar: http://hg.mozilla.org/projects/cedar/rev/945c1c37b620

Comment 103

[(no longer active)]

Part 1 landed on m-c: http://hg.mozilla.org/mozilla-central/rev/2a8d41b42688

Comment 104

[Timothy Nikkel (:tnikkel)]

Part 2 landed ages ago, so we are all done here! Thanks!