changing style due to :hover and then scrolling part of style-changed element in shows old style in scrolled-in area

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dbaron, Assigned: tnikkel)

Tracking

(Depends on: 2 bugs)

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
I'm presuming this is a regression from retained layers (bug 564991).

Steps to reproduce:
 1. load attachment 478026 [details]
 2. make the window small enough (in height) to scroll
 3. scroll the browser window down with the scrollbar so only a small slice of the upper blue element is visible
 4. move the mouse over that visible part of the upper blue element
 5. use the scrollwheel to scroll more of that element into view

Expected results:
 4. element turns from blue to fuchsia
 5. parts scrolled into view are also fuchsia (as long as the pointer is in the element)

Actual results:
 4. same as expected
 5. parts scrolled into view are blue

(It wouldn't surprise me if this is the same issue as some of the other regressions marked as dependencies of bug 564991, but this is a particularly simple testcase so it may be useful.)
(Reporter)

Comment 1

7 years ago
Confirmed that this regressed between Linux x86_64 nightlies 2010-07-15-03-mozilla-central and 2010-07-16-03-mozilla-central, as expected.
(Reporter)

Updated

7 years ago
blocking2.0: --- → ?
(Assignee)

Comment 2

7 years ago
I can reproduce this some of the time on Windows, with a small horizontal blue strip being painted.
(Assignee)

Comment 3

7 years ago
I guess we need to not intersect the invalidate rect with the scroll port for the purposes of invalidating thebes layers?
Correct!!!
Assignee: nobody → tnikkel
blocking2.0: ? → betaN+
(Assignee)

Comment 5

7 years ago
So I guess we need to pass both the intersected and non-intersected rect up the invalidate chain then?
(Reporter)

Comment 6

7 years ago
Why?  If there's a layer associated with the scrollframe, then you need to invalidate appropriately in that layer, but why would anything above it care?
I think what Timothy's thinking is that InvalidateInternal on the scrolled frame should have invalidated the layer before InvalidateInternal on the scrollframe restricted the invalid rect.
For some reason I can only reproduce this with smooth scrolling enabled.
(Assignee)

Comment 9

7 years ago
(In reply to comment #7)
> I think what Timothy's thinking is that InvalidateInternal on the scrolled
> frame should have invalidated the layer before InvalidateInternal on the
> scrollframe restricted the invalid rect.

Yeah, but the scrollframe doesn't have a container layer. I think we need to do FrameLayerBuilder::InvalidateThebesLayerContents(frame, r) where r is the unrestricted rect, and frame is the frame that has the container layer for the scrollframe.
ah, you mean the scrolled frame doesn't have a container layer. Right.

Don't forget to fix nsXULScrollFrame::InvalidateInternal as well.
(Assignee)

Comment 11

7 years ago
Created attachment 482444 [details] [diff] [review]
patch

So this fixes the problem and passes try server. It's a little more complicated than I wanted. What do you think of this?
Attachment #482444 - Flags: feedback?(roc)
Instead, how about in nsHTMLScrollFrame::InvalidateInternal, if scrollport clipping changes the invalid rect from the scrolled child and mScrollingActive, walk up to the nearest frame with a container layer and do an InvalidateThebesLayerContents on it?
(Assignee)

Comment 13

7 years ago
We'd lose the special processing we get with the other implementations of InvalidateInternal. block frame's implementation has an optimization, but I think SVG effects and transforms rely on it for correctness.
But those have (active or inactive) layers of their own, so scrolled-out-of-view stuff can't be cached in a retained ThebesLayer if there's an SVG effect or transform around it.
(Assignee)

Comment 15

7 years ago
Unless I'm missing something, I don't think SVG effects get their own layer currently.
We create a BasicLayerManager for the affected subtree, and then render all that content into the destination. Again, there can be no retained ThebesLayer caching content that is clipped out by scrollframes under SVG effects.
Hmm, I think I'm wrong here.

How about this: if scrolling is active and we need to invalidate ThebesLayers, just don't clip invalidation to the scrollport?
I guess that's bad.
What if we had a flag to invalidate ONLY ThebesLayers? Then when we clip to the scrollport, split the invalidation into two parts: invalidation of ThebesLayers, which uses the unclipped rect, and invalidation of the window, which uses the clipped rect. We can stop propagating when we get to a point where both "only ThebesLayers" and "no ThebesLayers" are set. That's similar to what you've got, but with less code changes.
(Assignee)

Comment 20

7 years ago
Yes, I like that, that's better.
(Assignee)

Updated

7 years ago
Blocks: 583387
(Assignee)

Updated

7 years ago
Attachment #482444 - Attachment is obsolete: true
Attachment #482444 - Flags: feedback?(roc)
(Assignee)

Comment 21

7 years ago
Created attachment 482795 [details] [diff] [review]
Part 1. (cleanup) Remove unused scrolling code for widgets
Attachment #482795 - Flags: review?(roc)
(Assignee)

Comment 22

7 years ago
Created attachment 482796 [details] [diff] [review]
Part 2. (cleanup) Remove unused parameter to nsGfxScrollFrameInner::ScrollVisual
Attachment #482796 - Flags: review?(roc)
(Assignee)

Comment 23

7 years ago
Created attachment 482797 [details] [diff] [review]
Part 3. The real fix.
Attachment #482797 - Flags: review?(roc)
Attachment #482795 - Flags: review?(roc) → review+
Attachment #482796 - Flags: review?(roc) → review+
Comment on attachment 482797 [details] [diff] [review]
Part 3. The real fix.

+          aFlags | seperateThebes ? INVALIDATE_NO_THEBES_LAYERS : 0);

It's not immediately obvious to me how the precedence works here, so for clarity please put parens around the ?: expression.

Is it possible to write a reasonable test here?
Attachment #482797 - Flags: review?(roc) → review+
(Assignee)

Comment 25

7 years ago
(In reply to comment #24)
> +          aFlags | seperateThebes ? INVALIDATE_NO_THEBES_LAYERS : 0);
> 
> It's not immediately obvious to me how the precedence works here, so for
> clarity please put parens around the ?: expression.

Good call.

> Is it possible to write a reasonable test here?

I think so. I remembered after posting the patches that I should write a test for this.
(Assignee)

Comment 26

7 years ago
(In reply to comment #25)
> (In reply to comment #24)
> > +          aFlags | seperateThebes ? INVALIDATE_NO_THEBES_LAYERS : 0);
> > 
> > It's not immediately obvious to me how the precedence works here, so for
> > clarity please put parens around the ?: expression.
> 
> Good call.

Indeed, the parens are needed to make it correct!
(Assignee)

Comment 27

7 years ago
Created attachment 483018 [details] [diff] [review]
Part 4. Test

Here is the test I plan to add.
(Assignee)

Comment 28

7 years ago
http://hg.mozilla.org/mozilla-central/rev/d8485927c4cc
http://hg.mozilla.org/mozilla-central/rev/bed182d7f22d
http://hg.mozilla.org/mozilla-central/rev/8188183a6402
http://hg.mozilla.org/mozilla-central/rev/417a9fc988ec
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Depends on: 604969
(Assignee)

Updated

7 years ago
Depends on: 608930

Updated

6 years ago
Depends on: 705174
You need to log in before you can comment on or make changes to this bug.