The default bug view has changed. See this FAQ.

After landing Bug 758620, Some CSS causes a problem of re-painting(Tabstrip, Favicon, Label, etc.)

RESOLVED FIXED in Firefox 16

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Alice0775 White, Assigned: cwiiis)

Tracking

({regression})

16 Branch
mozilla17
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16+ fixed)

Details

(Whiteboard: fixed by bug 539356)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 637788 [details]
userChrome.css: Reduced css from "Tree Style tab"

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/9bf5e71c5746
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120628065213

After landing Bug 758620, Some CSS causes a problem of re-painting.

This problem affects "Tree Style Tab" add-on .

Steps to Reproduce:
1. Prepare a new profile
2. Save attached file as chrome\userChrome.css in the profile
3. Start Firefox with the profile
4. Open several tabs, open web page in existing tab
    --- You can see TabToolbar repaint fails
5. Try to blu rbrowser window  and re-focus
    --- Now, you can see re-painting is carried out

Actual Results:
 Repainting of the TabToolbar is not carried out.

Expected Results:
 Repaint properly
(Reporter)

Comment 1

5 years ago
Err:
s/5. Try to blu rbrowser/5. Try to blur browser/
(Reporter)

Comment 2

5 years ago
In addition to teh comment#0,

You need to enable MenuBar.
(Reporter)

Updated

5 years ago
Summary: After landing Bug 758620, Some CSS causes a problem of re-painting → After landing Bug 758620, Some CSS causes a problem of re-painting(Tabstrip, Favicon, Label, etc.)
(Assignee)

Comment 3

5 years ago
So this would be a bug in invalidation, I suppose - given that Firefox doesn't use scroll-ports, the layout hasn't changed here, just the display list (and resultant layers).

Can't immediately think what's wrong, but will take a look and ask around.
(Assignee)

Comment 4

5 years ago
We're going to see if DLBI fixes this.
Depends on: 539356
(Assignee)

Comment 5

5 years ago
Now that bug 539356 has landed, does this bug still occur?
Keywords: qawanted
(Reporter)

Comment 6

5 years ago
I confirmed that this was fixed by bug 539356.

http://hg.mozilla.org/mozilla-central/rev/d9d61d199b11
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120701030537
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: qawanted
Whiteboard: fixed by bug 539356
(Assignee)

Comment 7

5 years ago
DLBI was backed out and has no fixed schedule for relanding - will take a look at this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

5 years ago
It took me a while to figure out how to reproduce this, I don't think the given instructions are clear. Here are some altered steps:

1. Prepare a new profile
2. Save attached file as chrome\userChrome.css in the profile
3. Restart Firefox with the profile
4. Keep opening new tabs until the tab bar starts scrolling
5. Observe that once the tab bar starts scrolling, tabs to the left of the newly opened one aren't invalidated (they appear as if they're still selected)
6. Scrolling the tab bar corrects the drawing, as does blurring the window.
(Assignee)

Comment 9

5 years ago
I've discovered another invalidation bug that I think may also be the cause of this one;

It seems if a fixed position frame's origin moves, it gets invalidated incorrectly..

This now matters as fixed position elements get their own layers, and so this wouldn't have occurred before (I don't think).

After speaking with ajuma, we think that frame-tree invalidation needs to take into account the origin shifting, or something along these lines.
You should be hitting this path in nsAbsoluteContainingBlock::ReflowAbsoluteFrame:
  if (oldRect.TopLeft() != rect.TopLeft() || 
      (aDelegatingFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
    // The frame moved
    aKidFrame->GetParent()->Invalidate(oldOverflowRect);
    aKidFrame->InvalidateFrameSubtree();
(Assignee)

Comment 11

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> You should be hitting this path in
> nsAbsoluteContainingBlock::ReflowAbsoluteFrame:
>   if (oldRect.TopLeft() != rect.TopLeft() || 
>       (aDelegatingFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
>     // The frame moved
>     aKidFrame->GetParent()->Invalidate(oldOverflowRect);
>     aKidFrame->InvalidateFrameSubtree();

Right - we're hitting this path and checking all the numbers being passed around, everything looks correct... The next thing I need to verify is that these invalidations are actually hitting the right layers.

When you invalidate a frame, does it propagate towards its descendants, or only upwards? If the former, the Invalidate call isn't doing what we want, if the latter, then the InvalidateFrameSubtree call isn't.
When aKidFrame has its own ContainerLayer, aKidFrame->InvalidateFrameSubtree() should be invalidating all ThebesLayers in the layer subtree rooted at that ContainerLayer. Isn't that what you need here?
(Assignee)

Comment 13

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> When aKidFrame has its own ContainerLayer,
> aKidFrame->InvalidateFrameSubtree() should be invalidating all ThebesLayers
> in the layer subtree rooted at that ContainerLayer. Isn't that what you need
> here?

It is, but it doesn't appear to be what's happening - instead, only the newly visible area is invalidated.
Does it show up with GPU compositing enabled? If so, then we're failing to repaint the whole ThebesLayer(s) somehow. If not, then we're probably failing to invalidate the right widget area for BasicLayers.
(Assignee)

Comment 15

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Does it show up with GPU compositing enabled? If so, then we're failing to
> repaint the whole ThebesLayer(s) somehow. If not, then we're probably
> failing to invalidate the right widget area for BasicLayers.

I'm testing this on mobile, so this is BasicLayers - I'll see if it's possible to put together a simpler test-case that will expose the problem on desktop.
OK, so we must be failing to repaint the ThebesLayer(s) for the fixed-pos element. You can verify this with a little logging.
(Assignee)

Comment 17

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> OK, so we must be failing to repaint the ThebesLayer(s) for the fixed-pos
> element. You can verify this with a little logging.

Stuck in some logging in layers InvalidateRegion - seems for the incorrect updates, the invalidation seems to get zeroed.

My current test is a page that has a very tall div in the background (to make the page scrollable) and a small fixed-pos div aligned at the bottom-right (http://chrislord.net/files/mozilla/fixed2.html). If I zoom out very slightly so that the contentWidth is smaller than the screenWidth and let go so it bounces back, two draws get triggered (one after letting go, the next after bouncing back).

These draws have mostly identical viewports, but a shorter scroll-port clamping size - So the fixed-pos item starts off lower, then moves up slightly when the clamping scroll-port shrinks in height.

Both of these paths take the 'frame moved' branch in nsAbsoluteContainer, as they should. I see these ThebesLayer invalidations on the first reflow:

I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 0, 0x0
I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 1536, 1280x1792
I/Gecko   (24726): CCC InvalidateRegion bounds: 1173, 611, 86x85

and these on the second:

I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 0, 0x0
I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 1536, 1280x1792
I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 0, 0x0

So that last invalidation was almost certainly the invalidation of the fixed-pos layer, and is certainly wrong.

I'm tracing further to find out why.
(Assignee)

Comment 18

5 years ago
I've put an easier test-case at http://chrislord.net/files/mozilla/fixed3.html - Just changing the 'bottom' style attribute exposes the incorrect invalidation. This test doesn't seem to break on desktop, unfortunately, so this may be a different problem.
(Assignee)

Comment 19

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #18)
> I've put an easier test-case at
> http://chrislord.net/files/mozilla/fixed3.html - Just changing the 'bottom'
> style attribute exposes the incorrect invalidation. This test doesn't seem
> to break on desktop, unfortunately, so this may be a different problem.

Another note, this doesn't trigger the move or resize block - I guess because the abs container isn't moving or resizing, but the fixed-pos child is the one moving... So I guess this is a problem with the invalidation done in the child's reflow(?)
(Assignee)

Comment 20

5 years ago
And another note - if I disable the creation of nsDisplayFixedPosition, the thebes layer invalidations are correct (I don't get the 3rd, zero-sized invalidation).

Replacing nsDisplayFixedPosition with nsDisplayOwnLayer has no effect on the problem, as expected (thankfully).
(Assignee)

Comment 21

5 years ago
Tracing further, in the situation where we don't use nsDisplayFixedPosition, we can't really compare - the invalidations happen 'correctly', as mInvalidateAllThebesContent ends up getting set, so it's actually just invalidating the entire layer.
(Assignee)

Comment 22

5 years ago
On pressing the button in my test, I see two reflows happening on the absolute containing block that has the fixed-pos element - the first has the current coordinates, and seems to invalidate the area correctly, the second has the new coordinates and does no invalidation (I guess because the first phase did it?)

This seems to me like it ought to produce the correct results, but when it comes to applying the thebes layer invalidation, the region for the frame associated with the nsDisplayFixedPosition is empty... So I guess the BuildLayer implementation is handing the wrong frame to BuildContainerLayerFor(?)
(Assignee)

Comment 23

5 years ago
Ok, so that last realisation seems to have been the breakthrough moment - Altering nsDisplayFixedPosition's BuildLayer function so that it builds a layer associated with the first principal child of its associated frame fixes the invalidation. Haven't tested to see if it fixes the title bug yet, but I expect it does.

I assume that this isn't the right solution though(?) - any suggestions roc? I'll continue to look at this on Monday.
(In reply to Chris Lord [:cwiiis] from comment #19)
> Another note, this doesn't trigger the move or resize block - I guess
> because the abs container isn't moving or resizing, but the fixed-pos child
> is the one moving... So I guess this is a problem with the invalidation done
> in the child's reflow(?)

No, when 'bottom' changes nsAbsoluteContainingBlock::ReflowAbsoluteFrame should still be called and oldRect.TopLeft() != rect.TopLeft() should be true.

(In reply to Chris Lord [:cwiiis] from comment #23)
> Ok, so that last realisation seems to have been the breakthrough moment -
> Altering nsDisplayFixedPosition's BuildLayer function so that it builds a
> layer associated with the first principal child of its associated frame
> fixes the invalidation.

That wouldn't be right. The frame we're building the container layer frame seems to be the right one already.

The fixed-pos frame should get aKidFrame->InvalidateFrameSubtree called on it from nsAbsoluteContainingBlock. That calls FrameLayerBuilder::InvalidateThebesLayersInSubtree; InternalInvalidateThebesLayersInSubtree should find (aFrame->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT) and (aFrame->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER). Then we do aFrame->Properties().Delete(ThebesLayerInvalidRegionProperty()). Next time we do a layer transaction, ApplyThebesLayerInvalidation for that frame finds invalidThebesContent is null and does:
    // The region was deleted to indicate that everything should be
    // invalidated.
    aState.SetInvalidateAllThebesContent();
That's what should happen, anyway.
(Assignee)

Updated

5 years ago
Depends on: 772079
(Assignee)

Comment 25

5 years ago
After debugging this with roc's help, we discovered this was happening due to bug #772079.

roc suggested checking IsProperAncestorFrame in ScrollLayerWrapper::WrapItem, but this means that fixed position layers end up as siblings of the root scroll layer, instead of children.

Really, this is correct, but all of our async scroll code assumes otherwise (both in native fennec, and xul-fennec), and so this isn't a simple fix for this bug.

This wouldn't be a problem if we set our transforms on the root layer instead of the root scroll layer (and that may solve some other problems too), but this causes odd problems where things start disappearing when you scroll. I've not looked into why this is yet, but it isn't immediately obvious to me.

I'm looking into a fix/work-around for this, but I've marked this as depending on bug #772079 in case we come up with a viable fix for that first.
(Assignee)

Comment 26

5 years ago
Just to note, the patch in bug 772079 fixes this.
(Reporter)

Comment 27

5 years ago
The patch in bug 772079 landed on Inbound tinderbox build.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a7f80f6408ed
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120714005020

However, This is still reproducible. this is not fixed...
(Assignee)

Comment 28

5 years ago
(In reply to Alice0775 White from comment #27)
> The patch in bug 772079 landed on Inbound tinderbox build.
> http://hg.mozilla.org/integration/mozilla-inbound/rev/a7f80f6408ed
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0
> ID:20120714005020
> 
> However, This is still reproducible. this is not fixed...

ugh, unfortunately this is correct - I just double-checked and it does solve the separate bug I was seeing in Fennec, but I guess either the fix isn't complete or this was caused by another issue.

I'll take a look at this.
Assignee: nobody → chrislord.net
(Assignee)

Updated

5 years ago
Blocks: 771529
(Assignee)

Comment 29

5 years ago
Just a quick note, the attached userChrome can just set position:fixed on TabsToolbar and do nothing else and the bug will manifest in the way I describe in comment #8. Keeping -moz-appearance makes it easier to look at, but isn't necessary to reproduce the bug.
(Assignee)

Comment 30

5 years ago
Debugging, but not found anything obvious that fixes this yet - Seeing a lot of empty invalidations, that makes me wonder a bit...
(Assignee)

Comment 31

5 years ago
More notes;

The invalidations are happening (via InvalidateThebesLayerContents), but somehow they aren't being propagated to the layer until some other change(?) triggers that to happen.

An interesting thing, when the bug is triggered, invalidations on all tabs except the new tab stop working altogether - the calls still happen, just they have no visible effect.

Continuing to debug.
(Assignee)

Comment 32

5 years ago
More notes; this problem only occurs on active layers.
(Assignee)

Comment 33

5 years ago
This is the layer tree when invalidations are working and the tabbar is innactive:

BasicLayerManager (0x7f0d72be4160)
  BasicContainerLayer (0x7f0d71ddec00) [visible=< (x=0, y=0, w=805, h=890); >] [opaqueContent] [metrics={ viewport=(x=0, y=0, w=805, h=890) viewportScroll=(x=0, y=0) displayport=(x=0, y=0, w=0, h=0) scrollId=0 }] [usesTmpSurf]
    BasicThebesLayer (0x7f0d71d29800) [visible=< (x=0, y=0, w=805, h=30); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=890); >]
    BasicContainerLayer (0x7f0d6e121600) [clip=(x=0, y=30, w=805, h=860)] [visible=< (x=0, y=30, w=805, h=860); >] [opaqueContent] [isFixedPosition]
      BasicThebesLayer (0x7f0d6e15f400) [visible=< (x=0, y=30, w=805, h=860); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=30, w=805, h=860); >]
    BasicContainerLayer (0x7f0d71ddf000) [visible=< (x=0, y=0, w=805, h=67); >] [isFixedPosition]
      BasicThebesLayer (0x7f0d71d2c400) [visible=< (x=0, y=0, w=805, h=67); >] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=67); >]
    BasicThebesLayer (0x7f0d71d2c800) [visible=< (x=0, y=0, w=805, h=28); >] [componentAlpha] [isFixedPosition]


And this is the layer-tree when invalidations are not working and the tabbar is active:

BasicLayerManager (0x7f0d72be4160)
  BasicContainerLayer (0x7f0d71ddec00) [visible=< (x=0, y=0, w=805, h=890); >] [opaqueContent] [metrics={ viewport=(x=0, y=0, w=805, h=890) viewportScroll=(x=0, y=0) displayport=(x=0, y=0, w=0, h=0) scrollId=0 }] [usesTmpSurf]
    BasicThebesLayer (0x7f0d71d29800) [visible=< (x=0, y=0, w=805, h=30); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=890); >]
    BasicContainerLayer (0x7f0d6e121600) [clip=(x=0, y=30, w=805, h=860)] [visible=< (x=0, y=30, w=805, h=860); >] [opaqueContent] [isFixedPosition]
      BasicThebesLayer (0x7f0d6e15f400) [visible=< (x=0, y=30, w=805, h=860); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=30, w=805, h=860); >]
    BasicContainerLayer (0x7f0d71ddf000) [visible=< (x=0, y=0, w=805, h=67); >] [isFixedPosition]
      BasicThebesLayer (0x7f0d71d2c400) [visible=< (x=0, y=0, w=805, h=67); >] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=67); >]
    BasicThebesLayer (0x7f0d71d2c800) [visible=< (x=0, y=0, w=805, h=28); >] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=28); >]
    BasicThebesLayer (0x7f0d6d229400) [transform=[ 1 0; 0 1; -618 0; ]] [visible=< (x=641, y=0, w=759, h=28); >] [isFixedPosition] [valid=< (x=641, y=0, w=759, h=28); >]
    BasicThebesLayer (0x7f0d6d229c00) [transform=[ 1 0; 0 1; -618 0; ]] [visible=< (x=641, y=4, w=755, h=21); >] [componentAlpha] [isFixedPosition]


Although all the invalidations seem to be happening, and with the correct numbers, I don't believe in the active layers case that they're going to the correct layers. If I replace every non-empty invalidation with a full-layer invalidation in InvalidatePostTransformRegion, I still get the case where hovering over the tabs produces no visible effect until the layers are marked inactive (whereby everything redraws and looks correct).

I'll continue looking into this tomorrow.
In the active case you should be getting InvalidateThebesLayerContents that invalidates for the container layer 0x7f0d71ddf000 when you hover over the tabs.
(Assignee)

Comment 35

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> In the active case you should be getting InvalidateThebesLayerContents that
> invalidates for the container layer 0x7f0d71ddf000 when you hover over the
> tabs.

In both the active and inactive case, the only non-zero invalidations go to layer 0x7f0d71d2c400.

This seems odd to mee. In the active case, the two extra layers are on the root container (where, quite correctly I suppose, there are no invalidations) - I think there are two as the tab-bar wraps with the userChrome I'm using to test it.

Should these two extra layers not have ended up on the 0x7f0d71ddf000 container layer?
(Assignee)

Comment 36

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #35)
> I think there are two as the tab-bar wraps with the
> userChrome I'm using to test it.

Just to correct myself, the two extra layers are the tab-bar and the menu-bar (which is displayed with a transparent background on top of the tab-bar).

I've stuck up a couple of logs and the userChrome at http://chrislord.net/files/mozilla/769541/
(Assignee)

Comment 37

5 years ago
The display-list generated with this userChrome doesn't seem right to me - it appears that the tab-bar ends up outside of the FixedPosition display-item, but they really ought to be children to it?
Yes, that is odd.
(Assignee)

Comment 39

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> Yes, that is odd.

I'll spend some time tomorrow correlating the frame-tree with the display-list and see if/where it's going wrong.

It'd be great to get a reduced test-case for this, I failed to put one together when I tried on Monday, but I'll give that another go too.
(Assignee)

Comment 40

5 years ago
http://chrislord.net/files/mozilla/769541/frame-displaylist-layertree.txt - Here's a log (interspersed with a bit of debug output, but ignore that) of the frame-tree, display-list and resultant layer tree.

What seems wrong to me is that a child of the FixedList ends up outside of the FixedPosition item - this accounts for the weird resultant layer tree and problems - the invalidations are going to the layer of the FixedList, as they should, but when animating, layers on the items in the FixedList become active and end up creating layers on top of the layer of the FixedList.
As these active layers are only updated drawn when they're created, that explains why they don't respond to mouse movement.

Now we just need to figure out why a child of the FixedList is creating a display item that's out of tree...
(Assignee)

Comment 41

5 years ago
The comment,

    // A pseudo-stacking context (e.g., a positioned element with z-index auto).
    // We allow positioned descendants of the child to escape to our parent
    // stacking context's positioned descendant list, because they might be
    // z-index:non-auto

in nsIFrame::BuildDisplayListForChild sounds like it may be the culprit? Is this just an optimisation? Could we just check if child is of type FIXED_POSITION and tell it not to do that? Continuing...
(Assignee)

Comment 42

5 years ago
Created attachment 643480 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames

It was indeed the behaviour stated in the last comment - solution as discussed with roc on IRC.
Attachment #643480 - Flags: review?(roc)
Comment on attachment 643480 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames

Review of attachment 643480 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +2182,5 @@
> +          aLists.PositionedDescendants()->AppendNewToTop(
> +            new (aBuilder) nsDisplayFixedPosition(aBuilder, item->GetUnderlyingFrame(),
> +                                                  child, &fixedPosDescendantList));
> +        }
> +        item = new (aBuilder) nsDisplayFixedPosition(aBuilder, child, child, &list);

This puts the wrapped extraPositionedDescendents below the main fixed-pos item, instead of above it. That's bad.
(Assignee)

Comment 44

5 years ago
Created attachment 643500 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames

Whoops, right you are.
Attachment #643480 - Attachment is obsolete: true
Attachment #643480 - Flags: review?(roc)
Attachment #643500 - Flags: review?(roc)
(Assignee)

Comment 45

5 years ago
Created attachment 643507 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames

Sorry, in my haste I let the scope of a variable creep a bit - fixed.
Attachment #643500 - Attachment is obsolete: true
Attachment #643500 - Flags: review?(roc)
Attachment #643507 - Flags: review?(roc)
Attachment #643507 - Flags: review?(roc) → review+
(Assignee)

Comment 46

5 years ago
Try run looked green, pushed to inbound:

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

Will request for aurora, as bug 758620 shouldn't really be there without both this and bug 772079.
Backed out for intermittent but frequent reftest failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13662423&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13660753&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13664498&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13663050&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13665417&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13665176&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=13665064&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfdfb713816e
(Assignee)

Comment 48

5 years ago
Just opening up the clipping-3.html in a Firefox with this patch makes the fault very obvious - it displays correctly initially, but very quickly afterwards, the red square disappears. Investigating.
(Assignee)

Comment 49

5 years ago
Actually, to correct myself, it's not 'very quickly afterwards', it's as soon as you trigger another paint. Quite possibly another invalidation bug?
(Assignee)

Comment 50

5 years ago
(and quite possibly another invalidation bug that I've introduced, to be clear :))
(Reporter)

Updated

5 years ago
Blocks: 776247
(Assignee)

Comment 51

5 years ago
Looking at the differences between the frame-tree, display-list and layer tree when this is drawn, the bad draw has two missing layers (a parent and its child). The only difference in anything else between a bad and a good draw is that layers have been retained, the trees are otherwise identical. Also interestingly, the layer that gets lost is a container layer that's a single-child of a container layer.

I wonder if the parent of the lost container layer is being marked as being empty, as it only has another container layer as a child?

Either way, looks like the problem is with retaining layers, so that's where I'll be looking for now.
(Assignee)

Comment 52

5 years ago
More weirdness, the items that disappear are associated with a layer that isn't in the final tree (I assume it's the layer that would've been the leaf of the missing container). Not figured it out yet, but hopefully getting there.
(Assignee)

Comment 53

5 years ago
Stopping old layers from being reused in FrameLayerBuilder::BuildContainerLayerFor fixes the error, so that lends further credence that something is happening when retaining layers...
(Assignee)

Comment 54

5 years ago
So the problem here is that the display-list for the div that disappears is

Clip
FixedPosition
FixedPosition
Background
Border

(the problem clearly being the FixedPosition/FixedPosition) - I guess we could fix this by checking if the item in the extra positioned descendants list is already TYPE_FIXED_POSITION in nsIFrame::BuildDisplayListForChild, or implementing a merge in nsDisplayFixedPosition that merges when the underlying frame is identical - I've gone with the former, as it seems simpler/cleaner/more correct.

Verified that this fixes the problem, updated patch incoming.
(Assignee)

Comment 55

5 years ago
Created attachment 644934 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v2)
Attachment #643507 - Attachment is obsolete: true
Attachment #644934 - Flags: review?(roc)
(In reply to Chris Lord [:cwiiis] from comment #46)
> Try run looked green, pushed to inbound:
> 
> http://hg.mozilla.org/integration/mozilla-inbound/rev/e6068bdf8268
> 
> Will request for aurora, as bug 758620 shouldn't really be there without
> both this and bug 772079.

It seems worth nominating both for tracking as well in that case (with a brief explanation of why that's the case as well), so that they don't get lost.
(Assignee)

Comment 57

5 years ago
Nominating for tracking, as per comment #56.
tracking-firefox16: --- → ?
No longer depends on: 539356
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 58

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b9cad627572c - Try run is looking good so far.
(Assignee)

Comment 59

5 years ago
Created attachment 644978 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v2, rebased)

Rebase on current m-c.
Attachment #644934 - Attachment is obsolete: true
Attachment #644934 - Flags: review?(roc)
Attachment #644978 - Flags: review?(roc)
tracking-firefox16: ? → +
Comment on attachment 644978 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v2, rebased)

Review of attachment 644978 [details] [diff] [review]:
-----------------------------------------------------------------

I think this wouldn't work when there is a clipping display item between the two fixed-pos display items.

I think this approach isn't going to work when we have nested fixed-pos. But that should be rare. So how about tracking whether we're in a fixed-pos context with a flag in nsDisplayListBuilder, and when we are, refusing to create a nested fixed-pos display item?
(Assignee)

Comment 61

5 years ago
Created attachment 645229 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v3)

Do as suggested in comment #60. Verified fixes this bug and the behaviour desired in bug 758620 still works.
Attachment #644978 - Attachment is obsolete: true
Attachment #644978 - Flags: review?(roc)
Attachment #645229 - Flags: review?(roc)
Comment on attachment 645229 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v3)

Review of attachment 645229 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/generic/nsFrame.cpp
@@ +2107,5 @@
> +  // nsDisplayFixedPosition. We check if we're already building a fixed-pos
> +  // item and disallow nesting, to prevent the situation of bug #769541
> +  // occurring.
> +  bool isFixedPosition = !aBuilder->IsInFixedPosition() && !isSVG &&
> +    !child->GetParent()->GetParent() && disp->mPosition == NS_STYLE_POSITION_FIXED;

Call this buildFixedPositionItem. Also, put the simplest and most frequently false conditions first. So, I'd say
  disp->mPosition == NS_STYLE_POSITION_FIXED &&
  !child->GetParent()->GetParent() && !isSVG &&
  !aBuilder->IsInFixedPosition()
Attachment #645229 - Flags: review?(roc) → review+
(Assignee)

Comment 63

5 years ago
Created attachment 645595 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (final)

Uploading final patch and carrying r+.

Can't push to inbound at the moment, preemptively requesting aurora approval as we probably want to get this in asap and try runs are green.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Fixed position elements can sometimes fail to update correctly.
User impact if declined: Possibly UI and page drawing glitches (mostly manifesting in things either not drawing, or drawing and then failing to update)
Testing completed (on m-c, etc.): try run is green, tested locally - will make sure it lands on central before landing on aurora.
Risk to taking this patch (and alternatives if risky): Not particularly risky, this should only affect the broken situation. Slight risk that I have something wrong, but it's quite a small patch that's gone through several revisions now.
String or UUID changes made by this patch: None.
Attachment #645229 - Attachment is obsolete: true
Attachment #645595 - Flags: review+
Attachment #645595 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 64

5 years ago
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/79727970b987
https://hg.mozilla.org/mozilla-central/rev/79727970b987
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Reporter)

Updated

5 years ago
Blocks: 777371
Comment on attachment 645595 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (final)

[Triage Comment]
Given where we are in the cycle, and the fact that multiple devs have seen these issues in their own work, fast tracking to Aurora 16.
Attachment #645595 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0b7e9bf59fc
status-firefox16: --- → fixed

Updated

5 years ago
Duplicate of this bug: 778001

Comment 69

5 years ago
This bug seems to be not completely fixed yet, about animated throbber in pinned tabs.

Steps to reproduce:
1. In a new profile, install Tree Style Tab only.
2. Right-click on the vertical tab bar and check "Auto hide tab bar".
3. Go to TST's configuration dialog and choose the skin of tab bar
   "Default (specified by the Theme)".
4. Pin a tab.
5. Focus on a non-pinned tab, then right click on the pinned tab and
   select "Reload Tab".

Actual result: The throbber stops animating.
Expected result: The throbber is animated.

Note: This problem doesn't appear when "Mixed" and other built-in skins.
(Assignee)

Comment 70

5 years ago
(In reply to SHIMODA Hiroshi from comment #69)
> This bug seems to be not completely fixed yet, about animated throbber in
> pinned tabs.
> 
> Steps to reproduce:
> 1. In a new profile, install Tree Style Tab only.
> 2. Right-click on the vertical tab bar and check "Auto hide tab bar".
> 3. Go to TST's configuration dialog and choose the skin of tab bar
>    "Default (specified by the Theme)".
> 4. Pin a tab.
> 5. Focus on a non-pinned tab, then right click on the pinned tab and
>    select "Reload Tab".
> 
> Actual result: The throbber stops animating.
> Expected result: The throbber is animated.
> 
> Note: This problem doesn't appear when "Mixed" and other built-in skins.

I'll have a look at this - in the meantime though, it'd be super-helpful if you could verify that this is a bug caused by bug 758620 and not something pre-existing (and even more helpful if you could find a simpler way of reproducing, or a simplified test-case).
(Reporter)

Updated

5 years ago
Depends on: 786740
(Assignee)

Updated

5 years ago
No longer depends on: 786740
(Assignee)

Updated

5 years ago
Depends on: 786740
(Reporter)

Updated

5 years ago
Blocks: 793998
(Reporter)

Updated

5 years ago
Depends on: 802470
(Reporter)

Updated

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