Closed
Bug 1166301
Opened 10 years ago
Closed 9 years ago
Background-attachment:fixed images jitter while scrolling with APZ on desktop
Categories
(Core :: Panning and Zooming, defect, P1)
Core
Panning and Zooming
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: kats, Assigned: botond)
References
()
Details
(Whiteboard: [gfx-noted])
Attachments
(12 files, 2 obsolete files)
3.06 KB,
text/html
|
Details | |
40 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
kats
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
mattwoodrow
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
botond
:
review+
mattwoodrow
:
review+
|
Details |
1016 bytes,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
Load the attached page and scroll with APZ enabled on desktop. The image jitters because it is getting shifted on repaints, but it shouldn't be doing that.
Comment 1•10 years ago
|
||
The right way to fix this is to remove the nsLayoutUtils::UsesAsyncScrolling() condition from nsDisplayBackgroundImage::ShouldFixToViewport and fix the remaining bugs related to clipping + visible regions, which should be possible once we have bug 1148582.
The tricky part here is that the layer itself needs to stay fixed during scrolling, but its clip + mask layer should move. And when you have nested scroll frames, the layer should stay fixed with respect to the outer one and the clip needs to move along when scrolling both the outer and the inner one.
I think we should make the visible region just always be the entire viewport when APZ is enabled.
Comment 2•10 years ago
|
||
Attachment 8456709 [details] is a testcase with a rounded clip / mask layer.
Assignee | ||
Updated•10 years ago
|
Whiteboard: [gfx-noted]
Comment 3•9 years ago
|
||
If I correctly understand this bug, it happens at Windows too, not only FireFoxOS.
Reporter | ||
Comment 4•9 years ago
|
||
Yeah it happens on all desktop platforms.
OS: Gonk (Firefox OS) → All
Comment 5•9 years ago
|
||
Some investigation:
Looks like issue is situated at:
nsCSSRendering::ComputeBackgroundPositioningArea(...)
{
...
if (NS_STYLE_BG_ATTACHMENT_FIXED == aLayer.mAttachment) {
...
}
...
}
Comment 6•9 years ago
|
||
Some investigation:
Very strange source code is situated at:
> nsDisplayBackgroundImage::ShouldFixToViewport(...)
> {
> // APZ doesn't (yet) know how to scroll the visible region for these type of
> // items, so don't layerize them if it's enabled.
> if (nsLayoutUtils::UsesAsyncScrolling(mFrame) ... )
> return false;
> }
> ...
> }
If this comparison will be commented looks like jittering of fixed background image will be stoped.
Maybe some issues are situated in related code, but this comparisons can resolve current bug.
Flags: needinfo?(bugmail.mozilla)
Comment 7•9 years ago
|
||
It also causes the clip to lag behind. I'm currently looking into fixing this.
Assignee: nobody → mstange
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(bugmail.mozilla)
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Updated•9 years ago
|
URL: http://ipon.hu
Comment 9•9 years ago
|
||
I was seeing the sidebars on facebook jitter when scrolling, which I assume is this bug.
Reporter | ||
Comment 10•9 years ago
|
||
Based on feedback from nightly users so far I'd say this and bug 1122916 are the two more noticeable regressions with APZ, so I'm bumping them to P1 to indicate that we should try to fix it sooner rather than later (relative to its siblings that are blocking bug 1178298).
Priority: -- → P1
Assignee | ||
Comment 14•9 years ago
|
||
Markus and I discussed this yesterday to come up with a plan for fixing this bug and related issues with position:fixed items. I summarized the plan on this etherpad:
https://etherpad.mozilla.org/apz-background-fixed
Any feedback is welcome.
Assignee | ||
Comment 18•9 years ago
|
||
Bug 1166301 - Layerize background images fixed to child elements. r=mstange
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
This simple patch to layerize background images fixed to child elements (as opposed to just viewport elements) already makes things a lot better. Since these layers are annotated as being fixed, AsyncCompositionManager doesn't apply any async scroll transforms to them, and as a result they no longer jitter.
I intend to follow this up with a reftest, as well as two further correctness fixes (and corresponding tests) to round out this bug:
- Clipping such layers correctly
- Making sure we paint all areas of such layers that could be
brought into view by async scrolling
While making this change, I noticed that in bug 1045864 we disabled the layerization of such background images with Basic compositing for performance reasons. With APZ, we need the layerization to happen for correctness, and so this patch undoes that change in the APZ-enabled case. Matt, does this seem reasonable to you?
Attachment #8652053 -
Flags: feedback?(matt.woodrow)
Comment 20•9 years ago
|
||
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
https://reviewboard.mozilla.org/r/17059/#review15247
Ship It!
Attachment #8652053 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8652053 -
Attachment description: MozReview Request: Bug 1166301 - Layerize background images fixed to child elements. r=mstange → MozReview Request: Bug 1166301 - Layerize background images fixed to child elements. r=mattwoodrow
Attachment #8652053 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Bug 1166301 - Layerize background images fixed to child elements. r=mattwoodrow
Assignee | ||
Comment 22•9 years ago
|
||
Bug 1166301 - Test that a fixed background on a child element doesn't move when a scroll frame containing the child element is async-scrolled. r=mstange
Attachment #8652990 -
Flags: review?(mstange)
Assignee | ||
Comment 23•9 years ago
|
||
Now with reftest. Carrying r+ from Matt on the fix patch.
Comment 24•9 years ago
|
||
Comment on attachment 8652990 [details]
MozReview Request: Bug 1166301 - Test that a fixed background on a child element doesn't move when a scroll frame containing the child element is async-scrolled. r=mstange
https://reviewboard.mozilla.org/r/17331/#review15409
Ship It!
Attachment #8652990 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 25•9 years ago
|
||
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Bug 1166301 - Layerize background images fixed to child elements. r=mattwoodrow
Assignee | ||
Comment 27•9 years ago
|
||
Comment on attachment 8652990 [details]
MozReview Request: Bug 1166301 - Test that a fixed background on a child element doesn't move when a scroll frame containing the child element is async-scrolled. r=mstange
Bug 1166301 - Test that a fixed background on a child element doesn't move when a scroll frame containing the child element is async-scrolled. r=mstange
Assignee | ||
Comment 28•9 years ago
|
||
Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Assignee | ||
Comment 29•9 years ago
|
||
Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Assignee | ||
Comment 30•9 years ago
|
||
Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Assignee | ||
Comment 31•9 years ago
|
||
Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow
This ensures that regions beyond the clip are painted, and async scrolling can reveal them by moving the layer-level clip.
Assignee | ||
Comment 32•9 years ago
|
||
Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Assignee | ||
Comment 33•9 years ago
|
||
Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Assignee | ||
Comment 34•9 years ago
|
||
Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Markus, I'm going to ask Matt to review these patches to have an extra set of eyes on this code, but first I wanted to run them by you to see if I've explained things properly or missed anything.
Attachment #8654298 -
Flags: feedback?(mstange)
Assignee | ||
Updated•9 years ago
|
Attachment #8654299 -
Flags: feedback?
Assignee | ||
Updated•9 years ago
|
Attachment #8654299 -
Flags: feedback? → feedback?(mstange)
Assignee | ||
Updated•9 years ago
|
Attachment #8654300 -
Flags: feedback?(mstange)
Assignee | ||
Updated•9 years ago
|
Attachment #8654302 -
Flags: feedback?(mstange)
Assignee | ||
Comment 36•9 years ago
|
||
Comment 37•9 years ago
|
||
Assignee | ||
Comment 38•9 years ago
|
||
(In reply to Pulsebot from comment #37)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/291cb35502bd
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2614ae6d32c5
I landed the first two patches as they can stand alone, and already constitute a significant improvement.
Setting leave-open while the remaining patches are reviewed.
Keywords: leave-open
Assignee | ||
Updated•9 years ago
|
Assignee: mstange → botond
Assignee | ||
Comment 41•9 years ago
|
||
I went through the test cases of the various duplicate bugs. They all seem to be working well with these patches, except for the site reported in bug 1197654 (https://www.amazon.com/clouddrive/home/) which looks better but still a bit jumpy. I'll investigate what's going on there.
Assignee | ||
Comment 42•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #36)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=431e9e8c519d
The -clip and -mask reftests are failing on B2G. Investigating.
Assignee | ||
Comment 43•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #42)
> (In reply to Botond Ballo [:botond] from comment #36)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=431e9e8c519d
>
> The -clip and -mask reftests are failing on B2G. Investigating.
The problem is that B2G uses containerful scrolling for the RCD-RSF, but with the current patches, the clip adjustment is only done correctly with containerless scrolling.
Comment 44•9 years ago
|
||
I just tested the patches and it looks like the border radius / mask layer in attachment 8456709 [details] isn't being applied. This happens both with and without APZ in my local build on HiDPI Mac.
Comment 45•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/291cb35502bd
https://hg.mozilla.org/mozilla-central/rev/2614ae6d32c5
Flags: in-testsuite+
Assignee | ||
Comment 46•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #43)
> (In reply to Botond Ballo [:botond] from comment #42)
> > (In reply to Botond Ballo [:botond] from comment #36)
> > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=431e9e8c519d
> >
> > The -clip and -mask reftests are failing on B2G. Investigating.
>
> The problem is that B2G uses containerful scrolling for the RCD-RSF, but
> with the current patches, the clip adjustment is only done correctly with
> containerless scrolling.
Specifically, the problem is that with containerful scrolling, there is no difference in layer tree structure between a fixed background layer attached to a child element, and a fixed position layer, so there's no way AsyncCompositionManager can know to only un-adjust the clip rect in the latter case.
To rectify this, we'll have to store a flag on the layer that tells us whether it's fixed background or fixed position.
Assignee | ||
Comment 47•9 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #44)
> I just tested the patches and it looks like the border radius / mask layer
> in attachment 8456709 [details] isn't being applied. This happens both with
> and without APZ in my local build on HiDPI Mac.
The problem here is that the removal of the clip from the display item messes with the code that builds a mask layer for border-radius, causing the mask layer to never be created.
Assignee | ||
Comment 48•9 years ago
|
||
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Attachment #8654298 -
Flags: feedback?(mstange)
Assignee | ||
Comment 49•9 years ago
|
||
Comment on attachment 8654299 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Attachment #8654299 -
Flags: feedback?(mstange)
Assignee | ||
Comment 50•9 years ago
|
||
Comment on attachment 8654300 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow
Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow
This ensures that regions beyond the clip are painted, and async scrolling can reveal them by moving the layer-level clip.
This patch also ensures that we continue creating mask layers for fixed background layers correctly, where appropriate.
Attachment #8654300 -
Flags: feedback?(mstange)
Assignee | ||
Comment 51•9 years ago
|
||
Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Assignee | ||
Comment 52•9 years ago
|
||
Comment on attachment 8654297 [details]
MozReview Request: Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Assignee | ||
Comment 53•9 years ago
|
||
Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
Assignee | ||
Comment 54•9 years ago
|
||
Comment on attachment 8654301 [details]
MozReview Request: Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
This also tests that regions of a fixed background layer that may be revealed by async scrolling are painted.
Assignee | ||
Comment 55•9 years ago
|
||
Comment on attachment 8654302 [details]
MozReview Request: Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Attachment #8654302 -
Flags: feedback?(mstange)
Assignee | ||
Comment 56•9 years ago
|
||
Comment on attachment 8654303 [details]
MozReview Request: Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Assignee | ||
Comment 57•9 years ago
|
||
The updated patch series should fix the issues in comment 42 and comment 44.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca066edd80a
Comment 58•9 years ago
|
||
https://reviewboard.mozilla.org/r/17641/#review16045
::: layout/base/nsDisplayList.cpp:2807
(Diff revision 2)
> + clipRect = nsRect(aBuilder->ToReferenceFrame(rootFrame), rootFrame->GetSize());
This is probably incorrect in the case where there's a transform between rootFrame and mFrame. For example, if you have background-attachment:fixed on an element with transform:scale(0.5), this will clip the rendering to half the viewport size.
Comment 59•9 years ago
|
||
https://reviewboard.mozilla.org/r/17641/#review16045
> This is probably incorrect in the case where there's a transform between rootFrame and mFrame. For example, if you have background-attachment:fixed on an element with transform:scale(0.5), this will clip the rendering to half the viewport size.
nsLayoutUtils::TransformRect(rootFrame, rootFrame->GetRectRelativeToSelf(), mFrame) + aBuilder->ToReferenceFrame(mFrame)
should do it.
Comment 60•9 years ago
|
||
https://reviewboard.mozilla.org/r/17641/#review16045
> nsLayoutUtils::TransformRect(rootFrame, rootFrame->GetRectRelativeToSelf(), mFrame) + aBuilder->ToReferenceFrame(mFrame)
> should do it.
Let's try that again:
nsRect rootRect = rootFrame->GetRectRelativeToSelf();
if (nsLayoutUtils::TransformRect(rootFrame, mFrame, rootRect) == nsLayoutUtils::TRANSFORM_SUCCEEDED) {
clipRect = rootRect + aBuilder->ToReferenceFrame();
}
Assignee | ||
Comment 61•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #57)
> The updated patch series should fix the issues in comment 42 and comment 44.
>
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca066edd80a
The b2g reftests from comment 42 were still failing because of a silly mistake (I forgot to propagate the new layer attribute in ShadowLayerForwarder::EndTransaction()). I have this fixed locally.
I also addressed comment 58 as suggested in comment 60.
New Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b31a5efbbb90
Assignee | ||
Comment 62•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #41)
> I went through the test cases of the various duplicate bugs. They all seem
> to be working well with these patches, except for the site reported in bug
> 1197654 (https://www.amazon.com/clouddrive/home/) which looks better but
> still a bit jumpy. I'll investigate what's going on there.
It turns out that site doesn't use background-attachment:fixed; rather, it's using a parallax-like effect to scroll the background image at a slower rate than the main page content. Therefore, we shouldn't expect this bug to improve that page.
Assignee | ||
Comment 63•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #61)
> New Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b31a5efbbb90
Looking good! Posting patch series for review.
Assignee | ||
Comment 64•9 years ago
|
||
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Attachment #8654298 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•9 years ago
|
Attachment #8654299 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 65•9 years ago
|
||
Comment on attachment 8654299 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Assignee | ||
Comment 66•9 years ago
|
||
Comment on attachment 8654300 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow
Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow
This ensures that regions beyond the clip are painted, and async scrolling can reveal them by moving the layer-level clip.
This patch also ensures that we continue creating mask layers for fixed background layers correctly, where appropriate.
Attachment #8654300 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 67•9 years ago
|
||
Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Attachment #8656258 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 68•9 years ago
|
||
Comment on attachment 8655185 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Attachment #8655185 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•9 years ago
|
Attachment #8654297 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 69•9 years ago
|
||
Comment on attachment 8654297 [details]
MozReview Request: Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Assignee | ||
Comment 70•9 years ago
|
||
Comment on attachment 8655186 [details]
MozReview Request: Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
Attachment #8655186 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 71•9 years ago
|
||
Comment on attachment 8654301 [details]
MozReview Request: Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
This also tests that regions of a fixed background layer that may be revealed by async scrolling are painted.
Attachment #8654301 -
Flags: review?(mstange)
Assignee | ||
Comment 72•9 years ago
|
||
Comment on attachment 8654302 [details]
MozReview Request: Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Attachment #8654302 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 73•9 years ago
|
||
Comment on attachment 8654303 [details]
MozReview Request: Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Attachment #8654303 -
Flags: review?(mstange)
Assignee | ||
Comment 74•9 years ago
|
||
Note to reviewers: if the order of the patches here in Bugzilla seems strange, it's because MozReview mixed them up. You can see the intended order in ReviewBoard.
Comment 75•9 years ago
|
||
Comment on attachment 8654301 [details]
MozReview Request: Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
https://reviewboard.mozilla.org/r/17645/#review16237
Attachment #8654301 -
Flags: review?(mstange) → review+
Comment 76•9 years ago
|
||
Comment on attachment 8654303 [details]
MozReview Request: Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
https://reviewboard.mozilla.org/r/17649/#review16239
Attachment #8654303 -
Flags: review?(mstange) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8656258 -
Flags: review?(bugmail.mozilla)
Reporter | ||
Comment 77•9 years ago
|
||
Comment on attachment 8656258 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
https://reviewboard.mozilla.org/r/18125/#review16241
::: gfx/layers/Layers.cpp:1705
(Diff revision 1)
> - aStream << nsPrintfCString(" [isFixedPosition scrollId=%d anchor=%s]",
> + aStream << nsPrintfCString(" [isFixedPosition scrollId=%lld anchor=%s%s]",
%lld is fine, but %s%s?
Reporter | ||
Comment 78•9 years ago
|
||
Comment on attachment 8654297 [details]
MozReview Request: Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
https://reviewboard.mozilla.org/r/17637/#review16243
Attachment #8654297 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 79•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #77)
> Comment on attachment 8656258 [details]
> MozReview Request: Bug 1166301 - Use the correct format flags for printing
> fixed position data in the layers dump. r=kats
>
> https://reviewboard.mozilla.org/r/18125/#review16241
>
> ::: gfx/layers/Layers.cpp:1705
> (Diff revision 1)
> > - aStream << nsPrintfCString(" [isFixedPosition scrollId=%d anchor=%s]",
> > + aStream << nsPrintfCString(" [isFixedPosition scrollId=%lld anchor=%s%s]",
>
> %lld is fine, but %s%s?
Oh, heh. The added "%s" was supposed to go into the "Store a flag on Layer..." patch, with the following hunk:
aStream << nsPrintfCString(" [isFixedPosition scrollId=%lld anchor=%s%s]",
GetFixedPositionScrollContainerId(),
- ToString(anchor).c_str()).get();
+ ToString(anchor).c_str(),
+ IsFixedBackground() ? " fixedBackground" : "").get();
}
I just didn't split it out properly.
Reporter | ||
Comment 80•9 years ago
|
||
Ah, gotcha. r+ with the split fixed.
Updated•9 years ago
|
Attachment #8654298 -
Flags: review?(matt.woodrow) → review+
Comment 81•9 years ago
|
||
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
https://reviewboard.mozilla.org/r/17639/#review16315
::: layout/base/FrameLayerBuilder.cpp:3757
(Diff revision 2)
> + animatedGeometryRootForScrollMetadata = nsLayoutUtils::GetAnimatedGeometryRootForFrame(
A comment about this version of the function *not* taking ShouldFixToViewport() into account wouldn't hurt.
Comment 82•9 years ago
|
||
Comment on attachment 8654299 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
https://reviewboard.mozilla.org/r/17641/#review16319
Attachment #8654299 -
Flags: review?(matt.woodrow) → review+
Comment 83•9 years ago
|
||
Comment on attachment 8654300 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow
https://reviewboard.mozilla.org/r/17643/#review16321
::: layout/base/FrameLayerBuilder.cpp:3185
(Diff revision 3)
> + // data->mCommonClipCount will be zero because we removed the clip from
Assert this?
Attachment #8654300 -
Flags: review?(matt.woodrow) → review+
Comment 84•9 years ago
|
||
Comment on attachment 8655185 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
https://reviewboard.mozilla.org/r/17859/#review16329
::: gfx/layers/Layers.h:1167
(Diff revision 2)
> + bool aFixedBackground)
I think I'd prefer to name this 'aIsClipFixed' or 'aIsClipScrolled' (no preference as to which).
Having the Layers API naming reflect the behaviour we want the layer to have seems better than the specific layout construct that required it.
I can probably be convinced otherwise though if you feel strongly.
Attachment #8655185 -
Flags: review?(matt.woodrow)
Comment 85•9 years ago
|
||
Comment on attachment 8655186 [details]
MozReview Request: Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
https://reviewboard.mozilla.org/r/17861/#review16333
Attachment #8655186 -
Flags: review?(matt.woodrow)
Comment 86•9 years ago
|
||
Comment on attachment 8655186 [details]
MozReview Request: Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
https://reviewboard.mozilla.org/r/17861/#review16335
Attachment #8655186 -
Flags: review+
Comment 87•9 years ago
|
||
Comment on attachment 8654302 [details]
MozReview Request: Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
https://reviewboard.mozilla.org/r/17647/#review16337
Attachment #8654302 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 88•9 years ago
|
||
Comment on attachment 8654298 [details]
MozReview Request: Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Bug 1166301 - Annotate fixed background layers with scroll metadata for the animated geometry root of the frame they're the background of. r=mattwoodrow
Assignee | ||
Comment 89•9 years ago
|
||
Comment on attachment 8654299 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Bug 1166301 - If APZ is enabled, only clip fixed background images to the viewport area. r=mattwoodrow
Assignee | ||
Comment 90•9 years ago
|
||
Comment on attachment 8654300 [details]
MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow
Bug 1166301 - If APZ is enabled, clip fixed background images at the layer level rather than the display item level. r=mattwoodrow
This ensures that regions beyond the clip are painted, and async scrolling can reveal them by moving the layer-level clip.
This patch also ensures that we continue creating mask layers for fixed background layers correctly, where appropriate.
Assignee | ||
Comment 91•9 years ago
|
||
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Attachment #8652053 -
Attachment description: MozReview Request: Bug 1166301 - Layerize background images fixed to child elements. r=mattwoodrow → MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
Attachment #8652053 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8656258 -
Attachment description: MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats → MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Attachment #8656258 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 92•9 years ago
|
||
Comment on attachment 8656258 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Assignee | ||
Comment 93•9 years ago
|
||
Comment on attachment 8654297 [details]
MozReview Request: Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Bug 1166301 - Update an old comment in AsyncCompositionManager::AlignFixedAndStickyLayers. r=kats
Assignee | ||
Comment 94•9 years ago
|
||
Comment on attachment 8655186 [details]
MozReview Request: Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
Bug 1166301 - Allow async scrolling to move the clip rects of fixed background layers. r=mattwoodrow
Assignee | ||
Comment 95•9 years ago
|
||
Comment on attachment 8654301 [details]
MozReview Request: Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Bug 1166301 - Test that the clip on a fixed background of a child element is moved correctly during async scrolling. r=mstange
This also tests that regions of a fixed background layer that may be revealed by async scrolling are painted.
Assignee | ||
Comment 96•9 years ago
|
||
Comment on attachment 8654302 [details]
MozReview Request: Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Bug 1166301 - When applying an async adjustment to a fixed layer, only adjust its mask layer under the same circumstances as its clip rect. r=mattwoodrow
Assignee | ||
Comment 97•9 years ago
|
||
Comment on attachment 8654303 [details]
MozReview Request: Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Bug 1166301 - Test that a mask layer on a fixed background of a child element is moved correctly during async scrolling. r=mstange
Assignee | ||
Updated•9 years ago
|
Attachment #8652990 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8655185 -
Attachment is obsolete: true
Reporter | ||
Comment 98•9 years ago
|
||
Comment on attachment 8652053 [details]
MozReview Request: Bug 1166301 - Use the correct format flags for printing fixed position data in the layers dump. r=kats
MozReview didn't do such a great job of lining up the flags..
Attachment #8652053 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 99•9 years ago
|
||
Comment on attachment 8656258 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
Carrying r+.
Attachment #8656258 -
Flags: review?(matt.woodrow) → review+
Comment 100•9 years ago
|
||
Comment on attachment 8656258 [details]
MozReview Request: Bug 1166301 - Store a flag on Layer to tell fixed background layers apart from fixed position layers. r=mattwoodrow
https://reviewboard.mozilla.org/r/18125/#review16437
::: gfx/layers/Layers.h:1165
(Diff revision 2)
> + * - |aIsClipFixed| is true if the clip rect of this layer should also
'clip' in this case also includes the mask layer as well as the clip rect, right?
Attachment #8656258 -
Flags: review+
Assignee | ||
Comment 101•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #98)
> MozReview didn't do such a great job of lining up the flags..
In this case, that doesn't surprise me. To fix the bad split, I had to histedit the patch series once to split a patch into two, and then histedit it again to roll the split-out commit into another. Who knows what that does to the commit metadata that MozReview uses to figure out how to map patches from the updated series to patches to the original series...
Assignee | ||
Comment 102•9 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #100)
> ::: gfx/layers/Layers.h:1165
> (Diff revision 2)
> > + * - |aIsClipFixed| is true if the clip rect of this layer should also
>
> 'clip' in this case also includes the mask layer as well as the clip rect,
> right?
Yup, thanks. I updated the comment locally.
Assignee | ||
Comment 103•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 105•9 years ago
|
||
Had some build failures, trying again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce044c5d9dee
Assignee | ||
Comment 106•9 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #83)
> Comment on attachment 8654300 [details]
> MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background
> images at the layer level rather than the display item level. r=mattwoodrow
>
> https://reviewboard.mozilla.org/r/17643/#review16321
>
> ::: layout/base/FrameLayerBuilder.cpp:3185
> (Diff revision 3)
> > + // data->mCommonClipCount will be zero because we removed the clip from
>
> Assert this?
And the Try run shows this assertion failing! Good call asking me to add it :)
Assignee | ||
Comment 107•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #106)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #83)
> > Comment on attachment 8654300 [details]
> > MozReview Request: Bug 1166301 - If APZ is enabled, clip fixed background
> > images at the layer level rather than the display item level. r=mattwoodrow
> >
> > https://reviewboard.mozilla.org/r/17643/#review16321
> >
> > ::: layout/base/FrameLayerBuilder.cpp:3185
> > (Diff revision 3)
> > > + // data->mCommonClipCount will be zero because we removed the clip from
> >
> > Assert this?
>
> And the Try run shows this assertion failing! Good call asking me to add it
> :)
The reason the assertions fails is that if we're in an inactive layer manager, we don't call UpdateCommonClipCount() at all [1], leaving mCommonClipCount at -1.
[1] https://dxr.mozilla.org/mozilla-central/rev/c0abc2a6e11f52761366e029eb1bae4c9864a8a3/layout/base/FrameLayerBuilder.cpp#4037
Assignee | ||
Comment 108•9 years ago
|
||
Fixed locally, new Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cfd97854f10
Assignee | ||
Comment 109•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #108)
> Fixed locally, new Try push:
>
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cfd97854f10
This looks much better, but the following Android reftest failures seem suspicious:
REFTEST TEST-UNEXPECTED-FAIL | http://10.26.131.22:30413/tests/layout/reftests/scrolling/fixed-table-1.html | assertion count 6 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | http://10.26.131.22:30413/tests/layout/reftests/scrolling/fixed-opacity-2.html | assertion count 3 is more than expected 0 assertions
The log doesn't say what the assertion failures were, so I'll have to run the tests locally and see.
Reporter | ||
Comment 110•9 years ago
|
||
The assertions show up in the logcar, which is linked from the job details pane on treeherder.
Assignee | ||
Comment 111•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #110)
> The assertions show up in the logcar, which is linked from the job details
> pane on treeherder.
Oh, thanks, I didn't know that!
Looks like the assertions are all this:
09-12 01:13:33.278 I/Gecko ( 2103): [2103] ###!!! ASSERTION: Bounds computation mismatch: 'mContainerBounds.Contains(mAccumulatedChildBounds)', file /builds/slave/try-and-api-11-d-0000000000000/build/src/layout/base/FrameLayerBuilder.cpp, line 4784
I'm not quite sure what to make of that. Will investigate in more detail on Monday.
Assignee | ||
Comment 112•9 years ago
|
||
I bisected the patch series to determine that the assertion is caused by the "clip fixed background images at the layer level rather than the display item level" patch.
I guess the rather unusual thing we're doing in this patch (removing a clip that has been set on a display item and then setting it on a layer) is upsetting some invariants that FrameLayerBuilder expects to hold.
Comment 113•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #112)
> I guess the rather unusual thing we're doing in this patch (removing a clip
> that has been set on a display item and then setting it on a layer) is
> upsetting some invariants that FrameLayerBuilder expects to hold.
I agree. I'd try inserting a "bounds = fixedToViewportClip.ApplyNonRoundedIntersection(bounds);" before the line "((nsRect&)mAccumulatedChildBounds).UnionRect(mAccumulatedChildBounds, bounds);".
Assignee | ||
Comment 114•9 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #113)
> (In reply to Botond Ballo [:botond] from comment #112)
> > I guess the rather unusual thing we're doing in this patch (removing a clip
> > that has been set on a display item and then setting it on a layer) is
> > upsetting some invariants that FrameLayerBuilder expects to hold.
>
> I agree. I'd try inserting a "bounds =
> fixedToViewportClip.ApplyNonRoundedIntersection(bounds);" before the line
> "((nsRect&)mAccumulatedChildBounds).UnionRect(mAccumulatedChildBounds,
> bounds);".
Thanks, Markus! That does indeed fix the problem, as shown by this Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=344e7e1c21f3
Matt, does this seem reasonable to you? I'm going to fold it into the "clip fixed background images at the layer level rather than the display item level" patch, I'm just posting it separately for ease of reviewing.
Attachment #8660923 -
Flags: review?(matt.woodrow)
Updated•9 years ago
|
Attachment #8660923 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 115•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #114)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=344e7e1c21f3
This Try push is clean, other than some "Android 4.0 API11+ opt" mochitest failures which are also happening without my patches [2].
I think this is ready to land!
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=996b7e0d8b6a
Comment 116•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/085754ce9f61
https://hg.mozilla.org/integration/mozilla-inbound/rev/1137bd3b5434
https://hg.mozilla.org/integration/mozilla-inbound/rev/63d5dac07434
https://hg.mozilla.org/integration/mozilla-inbound/rev/feb8c0e441f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4932d5e081d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f713731a9f3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2fda65a34f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6373ec1f943c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ed0b24e299
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24b11e4beb2
https://hg.mozilla.org/mozilla-central/rev/085754ce9f61
https://hg.mozilla.org/mozilla-central/rev/1137bd3b5434
https://hg.mozilla.org/mozilla-central/rev/63d5dac07434
https://hg.mozilla.org/mozilla-central/rev/feb8c0e441f5
https://hg.mozilla.org/mozilla-central/rev/4932d5e081d1
https://hg.mozilla.org/mozilla-central/rev/f713731a9f3b
https://hg.mozilla.org/mozilla-central/rev/4e2fda65a34f
https://hg.mozilla.org/mozilla-central/rev/6373ec1f943c
https://hg.mozilla.org/mozilla-central/rev/e8ed0b24e299
https://hg.mozilla.org/mozilla-central/rev/d24b11e4beb2
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•