697215 - Cannot scroll panel on OS X

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

To repro:

1. Open https://builder.addons.mozilla.org/addon/1022414/revision/3/

2. try to scroll vertically via either the mouse wheel or by grabbing the scroll bar.

Result: 

 - display corruption / content gets 'stuck' and does not display correctly.

Specifically, I am running 10.7 on an i7 MBAir.

Comment 1

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

Eddy, can you see if there's a platform issue here?

Comment 2

[Eddy Bruel [:ejpbruel]]

I cannot reproduce this bug. Using OSX 10.6.7 on my MacBook Pro, and running Firefox 7.0.1, I don't see any vertical scrollbars. If I make the window much smaller the vertical scrollbar appears, but when I use it I don't see anything that indicates display corruption.

With respect to content getting 'stuck': I cannot add or change text in the editor window. It looks like it's totally frozen.

Comment 3

[Will Bamberg [:wbamberg]]

Attachment: Screenshot showing the problem

It's reproducible for me too. See the attachment.

(In reply to Eddy Bruel [:ejpbruel] from comment #2)
> I cannot reproduce this bug. Using OSX 10.6.7 on my MacBook Pro, and running
> Firefox 7.0.1, I don't see any vertical scrollbars. If I make the window
> much smaller the vertical scrollbar appears, but when I use it I don't see
> anything that indicates display corruption.
> 
> With respect to content getting 'stuck': I cannot add or change text in the
> editor window. It looks like it's totally frozen.

I think there's a misunderstanding, or a couple of implicit steps between Jeff's 1 and 2. The link he's attached is a link to a Builder add-on that can be used to repro it, not a direct repro. You need to run the add-on, then click the Firefox widget to get it to show the panel, then scroll in the panel.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Eddy contacted me on email about this issue, and I produced this quick screencast of it:

http://www.youtube.com/watch?v=KIM1mfuI72w

Comment 5

[Eddy Bruel [:ejpbruel]]

Disregard my last comment. I can definitely reproduce it now. I was indeed looking in the wrong place, but Jeff's screencast quickly pointed out my mistake. Thanks, Jeff.

From the looks of it, this is an issue with the renderer. Sadly, I don't know anything about that part of the platform, so I have no idea what could cause this behavior. Next step will be to ask around on irc until I find somebody who can point me in the right direction.

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Well, it's definitely a mac problem, and definitely not a problem on other platforms. Hopefully that and the video narrow the problem scope down enough.

Comment 7

[Paul Sawaya]

Did a git-bisect and found these changes responsible: https://github.com/mozilla/addon-sdk/commit/97b79af12284454986b4b87590860ae6911c8564

Uncommenting those style changes fixes the issue on my system.

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: test.xpi to reproduce the problem

I've attached a test.xpi to make it easy for the graphics team to reproduce
this issue. I've also made a couple of observations that might help in 
pinpointing the source of the problem:

1. Install the add-on by dragging the test.xpi to the browser window. This 
   should cause a small Firefox icon to appear in the add-on bar (usually in the 
   lower-right corner of the window).
2. Open a panel by clicking on the Firefox icon. The panel's content should be
   http://news.google.com.
3. Slowly scroll down all the way to the bottom by dragging the vertical
   scrollbar. Notice how the panel's content isn't updated until you've almost
   reached the bottom.
4. Slowly scroll up all the way to the top again. Notice how the panel's content
   is initially updated until you're some distance from the bottom of the page.
5. Move your mouse over the panel at random. Notice how this causes the panel's
   content to be updated in areas where the cursor touched it.
6. Make sure the panel is scrolled up all the way to the top. Scroll right by 
   dragging the horizontal scrollbar. Notice how this time, the panel's
   content is updated properly.
7. Make sure the panel is scrolled up all the way to the top *and* all the way
   to the right. Scroll down again. Notice how this time, the panel's content
   is updated properly.

Summarizing: it looks like the panel's content is not redrawn properly when 
scrolling vertically, unless you've also scrolled completely to the right. The 
way the panel is updated when moving over it with your mouse suggests that the 
renderer is at least aware of the position of the scrollbars, so the problem is 
not that the renderer thinks we didn't move. Rather, the problem seems to be 
that some part of the repaint mechanism is not triggered in response to a scroll 
event.

My next step will be to do some stack tracing in order to figure out what part 
of the repaint mechanism is not triggered.

Oh, one final remark. Making the panel bigger so that horizontal scrolling is not possible does not fix the problem. The only thing that does by making it
impossible to scroll all the way to the right, it's become impossible to get vertical scrolling to work in the way I just described.

Comment 9

[Eddy Bruel [:ejpbruel]]

I've managed to pinpoint the problem to nsDisplayList::ComputeVisibilityForSublist. For some reason, after this function is called, the visible region of the panel is reduced to 0, which causes it not to be redrawn.

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Man this bug is getting epic. Thanks Eddy for following up!

Comment 11

[Eddy Bruel [:ejpbruel]]

Disregard my latest comment; it looks like it might have been premature.

I've worked with tn from the gfx team tonight and he gave me a rundown on how buffers and layers actually work. Based on that information, I now doubt that the problem is with the visible region.

What we've discovered though is that FrameLayerBuilder::DrawThebesLayer is called 3 times for each paint event when scrolling is working properly, but only 2 times when it is not. In other words, one layer is not being rendered. I have no idea why this is the case, but tn seems to have a hunch. From what I know, the layer that is not being rendered is a dependent layer, which is something that we do not expect. According to tn, the only way that layer could end up being dependent is by using border-radius, which is exactly the style property that caused the problem in the first place! This looks promising.

tn needs some time to think on the problem. I'm having a day off tomorrow, so my plan is to contact him again this thursday and see what else we can come up with.

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I've debugged the css on the reddit .compact page and reduced it to a specific set of 3 css rules that, if they are all commented out, allow scrolling to work:

https://github.com/canuckistani/bug_697215/blob/master/data/compact.xrmhC5EAhEY.css#L27-38

( they are the ones that are currently commented out in that block )

Comment 13

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

If I open up a plain Firefox build, run DOM Inspector and set border-radius:50px on xul:browser in the chrome document, then I get rounded corners on my content document and I see the bug --- the window doesn't repaint when I scroll.

Comment 14

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

The bug is that in nsGfxScrollFrame.cpp, CanScrollWithBitBlt is returning true even though the scrollframe is inside a frame that's clipping to a border-radius. It should return false in that situation.

It returns true because its logic assumes that we clip to border-radius only for scrollframes. But in fact replaced elements with overflow not "visible" don't induce scrollframes and we also clip to border-radius for them. Most such elements can't contain scrollframes, but <iframe>s can.

Comment 15

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

Attachment: fix

Comment 16

[Mats Palmgren (inactive)]

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

> ... replaced elements with overflow not "visible"
> don't induce scrollframes and we also clip to border-radius for them. Most
> such elements can't contain scrollframes, but <iframe>s can.

Worth documenting with a code comment to explain the eReplaced test there, IMO.

Comment 17

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
That's awesome! Really appreciate you taking a look at this issue.

Comment 18

[Eddy Bruel [:ejpbruel]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
A big thanks from me too for helping us out on this issue!

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/026ec6345ae3

Comment 20

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

Note that scrolling a panel that's clipped to a border-radius will not be fast.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a63669b8aa4

Backed out because the test failed on Linux.

Comment 22

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

On Linux we didn't scroll down far enough?
Maybe the IFRAME content in the test isn't quite tall enough to be scrolled? I dunno. Did a try push with taller content.

https://tbpl.mozilla.org/?tree=Try&rev=e3839c0ccdaf

Comment 23

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

The problem is that the test always removed the reftest-wait in the first doTest because I omitted an early return. Pushed a try build with that fixed.
https://tbpl.mozilla.org/?tree=Try&rev=fc1b7ac48457

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a81c89bb466

Comment 25

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

Backed out again, another (different) test failure. Just a couple of pixels this time. Trying disabling scrollbars to make that go away.

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

https://tbpl.mozilla.org/?tree=Try&rev=b7361d5b7d4b

Comment 26

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

https://tbpl.mozilla.org/?tree=Try&rev=f64df0a61b31

Comment 27

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

That try push is green.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b83a0ecb986

Comment 28

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

If you need this on FF11 (or 10) please nominate it.

Comment 29

[Jeff Griffiths (:canuckistani) (:⚡︎)]

roc: thanks. I'm going to nominate it for FF 10, the issue affects basic functionality in the SDK in a way that renders SDK component behaviour inconsistent on different platforms with a pretty common use case.

Comment 30

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

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

You'll want this on Aurora as well.

Comment 31

[Jeff Griffiths (:canuckistani) (:⚡︎)]

roc: It occurs to me that this patch hasn't landed yet - is it too early to add the nomination flag? I suppose I should wait for it to land and sit on nightly for a few days and, you know, do some testing.

Comment 32

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

I think we can leave the noms in place

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8b83a0ecb986

Comment 34

[LegNeato]

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

[triage comment]
Approved for aurora and beta

Comment 35

[LegNeato]

[triage comment]
I should say why: Fixes and issue on reddit, fixes display corruption on  https://builder.addons.mozilla.org

Comment 36

[LegNeato]

http://hg.mozilla.org/releases/mozilla-aurora/rev/b64a8da1413a
http://hg.mozilla.org/releases/mozilla-beta/rev/7a0d36baf5be

Comment 37

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Works for me in the latest Aurora build, using the builder link in the original report.

Comment 38

[LegNeato]

We backed this out in http://hg.mozilla.org/releases/mozilla-beta/rev/01ef9195f79b to see if it caused bug 714320. If not, we'll put it back in.

Comment 39

[Paul Silaghi, QA [:pauly]]

This is verified fixed on Firefox 11b1:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0