Closed
Bug 1086723
Opened 10 years ago
Closed 10 years ago
Problems with fixed position element on forbes.com
Categories
(Firefox for Android Graveyard :: Toolbar, defect)
Tracking
(firefox33 unaffected, firefox34+ wontfix, firefox35+ verified, firefox36+ verified, fennec34+)
VERIFIED
FIXED
Firefox 36
People
(Reporter: snorp, Assigned: kip)
References
Details
(Keywords: regression, reproducible)
Attachments
(4 files, 5 obsolete files)
5.15 MB,
video/mp4
|
Details | |
427 bytes,
text/html
|
Details | |
1.66 KB,
patch
|
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.18 KB,
patch
|
kats
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
If you scroll around an article on forbes.com on a phone, they have a little fixed position button in the bottom right. We seem to have some issues rendering that. See attached video.
Reporter | ||
Comment 1•10 years ago
|
||
Works on release
tracking-fennec: --- → ?
status-firefox33:
--- → unaffected
status-firefox36:
--- → affected
Keywords: regressionwindow-wanted
Comment 2•10 years ago
|
||
Broken since July Last good revision: 82df3654cd80 (2014-07-23) First bad revision: a91ec42d6a9c (2014-07-24) Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82df3654cd80&tochange=a91ec42d6a9c Can't get inbounds because our infra is hosed Retrieving valid builds from u'http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android/1405995031/' generated an exception: 503 Server Error: Service Unavailable: Back-end server is at capacity
status-firefox34:
--- → affected
status-firefox35:
--- → affected
OS: Mac OS X → Android
Hardware: x86 → ARM
Comment 3•10 years ago
|
||
Kats, Matt, I've seen enough bugs go through with clipping and ellipses and... is this a dupe or a different manifestation of something we know about?
Updated•10 years ago
|
Assignee: milan → nobody
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugmail.mozilla)
Comment 4•10 years ago
|
||
Maybe bug 1039926 or bug bug 1042423 fallout?
Comment 5•10 years ago
|
||
(In reply to Aaron Train [:aaronmt] from comment #2) > > Can't get inbounds because our infra is hosed > > Retrieving valid builds from > u'http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/mobile/ > tinderbox-builds/mozilla-inbound-android/1405995031/' generated an > exception: 503 Server Error: Service Unavailable: Back-end server is at > capacity Seems to be working now. Mind narrowing down the window? The two bugs Milan mentioned are both possible suspects. (In reply to Milan Sreckovic [:milan] from comment #3) > Kats, Matt, I've seen enough bugs go through with clipping and ellipses > and... is this a dupe or a different manifestation of something we know > about? We've seen various bugs but usually once we track down the regressing changeset they get fixed pretty quickly. I don't think we have anything open that this would be a dupe of.
Flags: needinfo?(bugmail.mozilla)
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=326c91338df0&tochange=06b540d3667a Don't have a environment currently to bi-sect further.
Blocks: 1042423
Comment 7•10 years ago
|
||
Since matt is perpetually swamped, moving needinfo to kip who is looking at bug 1067286. That doesn't sound related but it was also a regression from bug 1042423 so I figure kip might know this code too.
Flags: needinfo?(matt.woodrow) → needinfo?(kgilbert)
Assignee | ||
Comment 8•10 years ago
|
||
I can take this one.
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)
Updated•10 years ago
|
tracking-fennec: ? → 34+
Comment 9•10 years ago
|
||
[Tracking Requested - why for this release]: regression in 34
Updated•10 years ago
|
Comment 10•10 years ago
|
||
Kip - Are you working on a fix this week? Can we try to get this into beta6, which requires a fix ready for uplift by Sunday?
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 11•10 years ago
|
||
Have reproduced, am narrowing it down now
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 12•10 years ago
|
||
The issue is introduced in the Part 4 patch of Bug 1042423. Perhaps it is the same root cause as Bug 1067286.
Assignee | ||
Comment 13•10 years ago
|
||
Can reproduce on OSX and Fennec with APZC enabled using the attached minimal test case. When scrolling, the transform of the clipping appears to update only when the main thread re-paints. Intermediate composited frames are rendered incorrectly.
Assignee | ||
Comment 14•10 years ago
|
||
- Layer::ComputeEffectiveTransformForMaskLayer now uses the shadow transform. - AsyncCompositionManager::TranslateShadowLayer2D now also updates the shadow transform for any mask layer associated to the layer. - CompositorParent::CompositeCallback now updates the shadow properties of the mask layers with the content-side counterparts.
Attachment #8522449 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 15•10 years ago
|
||
I have pushed to try with a fairly wide range of tests, as this patch may affect multiple platforms: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a03f695a5e9a
Assignee | ||
Comment 16•10 years ago
|
||
I am new to this area of the code base, so would appreciate your feedback on if this is the best manner to apply this fix. (In reply to :kip (Kearwood Gilbert) from comment #14) > Created attachment 8522449 [details] [diff] [review] > Bug 1086723 - Enable asynchronous update of mask layer transforms > > - Layer::ComputeEffectiveTransformForMaskLayer now uses the shadow transform. > - AsyncCompositionManager::TranslateShadowLayer2D now also updates the > shadow transform for any mask layer associated to the layer. > - CompositorParent::CompositeCallback now updates the shadow properties of > the mask layers with the content-side counterparts.
Flags: needinfo?(bugmail.mozilla)
Comment 17•10 years ago
|
||
You don't need to needinfo me if you already flagged me for review :)
Flags: needinfo?(bugmail.mozilla)
Comment 18•10 years ago
|
||
Comment on attachment 8522449 [details] [diff] [review] Bug 1086723 - Enable asynchronous update of mask layer transforms Review of attachment 8522449 [details] [diff] [review]: ----------------------------------------------------------------- So based on looking through the code and a brief discussion with mattwoodrow on IRC, mask layers are probably pretty busted when they're applied to any layer with an async transform. This includes layers affected by OMTA and layers affected by APZC transforms (which include fixed-position layers). Your patch will probably fix the fixed-position layers part of it but I think with a little more work we can probably fix it for a wider set of things. I'm thinking we can put more code into the SetShadowTransform function to deal with this but I'm not sure if that's the cleanest way. I'll think it over and post back tomorrow. Leaving the review flag on me for now.
Comment 19•10 years ago
|
||
Comment on attachment 8522449 [details] [diff] [review] Bug 1086723 - Enable asynchronous update of mask layer transforms Review of attachment 8522449 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.cpp @@ +805,5 @@ > #ifdef DEBUG > bool maskIs2D = mMaskLayer->GetTransform().CanDraw2D(); > NS_ASSERTION(maskIs2D, "How did we end up with a 3D transform here?!"); > #endif > + mMaskLayer->mEffectiveTransform = mMaskLayer->GetLocalTransform() * mMaskLayer->mEffectiveTransform; So I think perhaps the best thing to do is to just change this line to be this: mMaskLayer->mEffectiveTransform = mMaskLayer->GetTransform() * GetTransform().Inverse() * GetLocalTransform() * mMaskLayer->mEffectiveTransform; And then you shouldn't need the other changes in this patch either. The reason I expect this to works is this: GetLocalTransform() will return the shadow transform, which simplifies to "GetTransform() * async-transform-stuff". We're interested in applying the "async-transform-stuff" to the mask layer. If we pre-multiply the inverse of GetTransform() to GetLocalTransform() then we isolate just the "async-transform-stuff" component, and can tack that on to the mask layer's existing transform. My only concern here is that because mMaskLayer->GetTransform() is a 2D translation matrix, we might need to do a ChangeBasis() on the "async-transform-stuff" before we can combine it like this. I'm not sure about that. If this works you shouldn't need to touch the TranslateShadowLayer2D function (because the translation there will be included in the "async-transform-stuff") nor the SetShadowProperties function (because we won't need to reset the mask layer's shadow transform, although that bit would just be harmless). Does that make sense? If it turns out this approach doesn't work then I'd probably be ok with landing this patch for now until we come up with a better/more comprehensive solution. Minusing for now.
Attachment #8522449 -
Flags: review?(bugmail.mozilla) → review-
Assignee | ||
Comment 20•10 years ago
|
||
- Layer::ComputeEffectiveTransformForMaskLayer now computes a delta transform for the mask layer's effective transform using the masked layer's shadow transform and base transform. This delta is applied to the effective transform of the mask layer, with the same effect as if it were also transformed by the APZC. V2 Patch: - Taking a simpler approach, based on feedback in Comment 19
Attachment #8522449 -
Attachment is obsolete: true
Attachment #8523122 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 21•10 years ago
|
||
I have manually verified the fix in the V2 patch with the attached minimal test case on OSX (With APZ enabled) and on Fennec. Due to the platform specific nature, I have also pushed to try for good measure: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7185863dbce7
Comment 22•10 years ago
|
||
Comment on attachment 8523122 [details] [diff] [review] Bug 1086723 - Enable asynchronous update of mask layer transforms (V2 Patch) Review of attachment 8523122 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.cpp @@ +810,5 @@ > + // mask layer's effective transform, as though it was also transformed by > + // the APZC. > + // > + // Note: This will fail if the base transform is degenerate. Currently, this > + // is not expected for APZC transformed layers. s/APZC/APZ and OMTA/ (two instances)
Attachment #8523122 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 23•10 years ago
|
||
- Layer::ComputeEffectiveTransformForMaskLayer now computes a delta transform for the mask layer's effective transform using the masked layer's shadow transform and base transform. This delta is applied to the effective transform of the mask layer, with the same effect as if it were also transformed by the APZC. V3 Patch: - Updated comment as per Comment 22
Attachment #8523122 -
Attachment is obsolete: true
Comment 24•10 years ago
|
||
The v3 patch is the same as the v2 patch
Assignee | ||
Comment 25•10 years ago
|
||
- Layer::ComputeEffectiveTransformForMaskLayer now computes a delta transform for the mask layer's effective transform using the masked layer's shadow transform and base transform. This delta is applied to the effective transform of the mask layer, with the same effect as if it were also transformed by the APZ. V4 Patch: - Updated comment as per Comment 22
Attachment #8523205 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24) > The v3 patch is the same as the v2 patch Thanks for spotting this. I have re-uploaded as V4 Patch.
Assignee | ||
Comment 27•10 years ago
|
||
I have attempted to create a reftest to verify the fix utilizing reftest-async-scroll reftest-async-scroll-y attributes. I can visibly see the issue reproduced by the reftest on OSX (with APZ enabled). Unfortunately (tested on Fennec and OSX), the screen capture occurs after the mask layer has been repositioned correctly. Would you recommend another method for testing this or perhaps a manual test is needed here?
Flags: needinfo?(bugmail.mozilla)
Comment 28•10 years ago
|
||
Can you attach the test? AFAIK you should be able to test this fine with reftest-async-scroll.
Flags: needinfo?(bugmail.mozilla) → needinfo?(kgilbert)
Assignee | ||
Comment 29•10 years ago
|
||
- Reftest added to perform an async scroll and verify that the mask used to render the rounded corners of a fixed position element are correctly offset.
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 30•10 years ago
|
||
(In reply to :kip (Kearwood Gilbert) from comment #29) > Created attachment 8525495 [details] [diff] [review] > Bug 1086723 - Part 2: Test > > - Reftest added to perform an async scroll and verify that the mask used > to render the rounded corners of a fixed position element are correctly > offset. Please note that this test is currently always passing, so will need some refinement before commit
Assignee | ||
Comment 31•10 years ago
|
||
I have pushed to try just the test: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2f343ff7940e And also, the test combined with the fix: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=af59c09541b3 Although the test did not detect the bug on Fennec, it did identify the same issue happening on B2G. The logged Base64 encoded image shows the failure that was expected; however, the test will need to be updated to accommodate scroll bar movement. Would it be acceptable for the test to identify the issue, although not on the platform (Fennec) that the bug was reported on?
Flags: needinfo?(bugmail.mozilla)
Comment 32•10 years ago
|
||
So it looks to me like your test works just fine. Currently B2G is the only platform on which the reftest-async-scroll stuff actually works by default, because that's the only one where the C++ APZ code is enabled by default. And that's the only platform that your test fails on (without the patch). It's currently also failing with the patch but that's just because of the scrollbar on the top-level page. If you make the body/html overflow:hidden that should fix that problem. So yes, your test is fine, and it catches the problem on B2G, which is good enough.
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 33•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32) > So it looks to me like your test works just fine. Currently B2G is the only > platform on which the reftest-async-scroll stuff actually works by default, > because that's the only one where the C++ APZ code is enabled by default. > And that's the only platform that your test fails on (without the patch). > It's currently also failing with the patch but that's just because of the > scrollbar on the top-level page. If you make the body/html overflow:hidden > that should fix that problem. > > So yes, your test is fine, and it catches the problem on B2G, which is good > enough. Thanks Kats, I am validating an updated version of the test to make sure the scrollbars don't trigger failures, then will post it for review.
Assignee | ||
Comment 34•10 years ago
|
||
- Reftest added to perform an async scroll and verify that the mask used to render the rounded corners of a fixed position element are correctly offset. V2 Patch: Now using overflow:hidden to hide scroll bars.
Attachment #8525495 -
Attachment is obsolete: true
Attachment #8525555 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 35•10 years ago
|
||
I have pushed the V2 Patch to try, running the B2G reftests. Test only (Expected to fail): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee4a17617c40 Test and fix (Expected to pass): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7cede6a23fc1
Comment 36•10 years ago
|
||
Comment on attachment 8525555 [details] [diff] [review] Bug 1086723 - Part 2: Test (V2 Patch) Review of attachment 8525555 [details] [diff] [review]: ----------------------------------------------------------------- r=me with style fixed ::: gfx/tests/reftest/1086723.html @@ +16,5 @@ > + height: 5000px; > + } > + body,html { > + overflow: hidden; > + } This file has tabs which is messing up the indenting. Please replaces the tabs with spaces and clean up the indentation. Same for the -ref.html file.
Attachment #8525555 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 37•10 years ago
|
||
- Reftest added to perform an async scroll and verify that the mask used to render the rounded corners of a fixed position element are correctly offset. V3 Patch: - Replaced tabs with spaces
Attachment #8525555 -
Attachment is obsolete: true
Attachment #8525560 -
Flags: review?(bugmail.mozilla)
Updated•10 years ago
|
Attachment #8525560 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 38•10 years ago
|
||
(In reply to :kip (Kearwood Gilbert) from comment #35) > I have pushed the V2 Patch to try, running the B2G reftests. > > Test only (Expected to fail): > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee4a17617c40 > > Test and fix (Expected to pass): > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7cede6a23fc1 Try runs show that test is working.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 39•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e552e9152594 https://hg.mozilla.org/integration/fx-team/rev/1e3a1de38584
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e552e9152594 https://hg.mozilla.org/mozilla-central/rev/1e3a1de38584
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Comment 41•9 years ago
|
||
Verified as fixed in Firefox for Android 36.0a1 (2014-11-23) Device: Asus Transformer Pad TF300T (Android 4.2.1)
Comment 42•9 years ago
|
||
Too late to take this in 34. Let's see about uplifting to 35.
Comment 43•9 years ago
|
||
Please nominate for beta uplift by Mon Dec 22 if it is low risk enough, we can take it in next week's beta, otherwise it will have to wait and ship with FF36.
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8523256 [details] [diff] [review] Bug 1086723 - Enable asynchronous update of mask layer transforms (V4 Patch), r=kats Approval Request Comment [Feature/regressing bug #]: Regressed by 1042423, which increased the scope of elements that could be assigned to layers. These layers exposed existing bugs in APZ code which failed to update the transforms assigned to masks used to round edges of layers. [User impact if declined]: If declined, existing websites, including forbes.com, will render incorrectly, including content not being visible. [Describe test coverage new/current, TBPL]: Manual testing on Fennec and testing on OSX (with APZ enabled) reproduced the issue and verified the fix in the original test case. A reftest has been added with a minimized version of the testcase. [Risks and why]: Low risk, impact specific to rounded rects that are positioned absolutely or animated on APZ platforms. [String/UUID change made/needed]:
Flags: needinfo?(kgilbert)
Attachment #8523256 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•9 years ago
|
Attachment #8525560 -
Attachment description: Bug 1086723 - Part 2: Test (V3 Patch) → Bug 1086723 - Part 2: Test (V3 Patch),r=kats
Assignee | ||
Comment 45•9 years ago
|
||
Comment on attachment 8525560 [details] [diff] [review] Bug 1086723 - Part 2: Test (V3 Patch),r=kats Approval Request Comment [Feature/regressing bug #]: Regressed by 1042423, which increased the scope of elements that could be assigned to layers. These layers exposed existing bugs in APZ code which failed to update the transforms assigned to masks used to round edges of layers. [User impact if declined]: If declined, existing websites, including forbes.com, will render incorrectly, including content not being visible. [Describe test coverage new/current, TBPL]: Manual testing on Fennec and testing on OSX (with APZ enabled) reproduced the issue and verified the fix in the original test case. A reftest has been added with a minimized version of the testcase. [Risks and why]: Low risk, impact specific to rounded rects that are positioned absolutely or animated on APZ platforms. [String/UUID change made/needed]:
Attachment #8525560 -
Flags: approval-mozilla-beta?
Updated•9 years ago
|
Attachment #8523256 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•9 years ago
|
Attachment #8525560 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 46•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/ab950c65d1c8 https://hg.mozilla.org/releases/mozilla-beta/rev/8da12cb91608
Flags: in-testsuite+
Comment 47•9 years ago
|
||
Using Fennec 35 Beta 6 I do not see the share button in the bottom right page. Is this because of the patch or it is something forbes decided to take out?
Comment 48•9 years ago
|
||
Nexus 5 ( Android 5.0.1) and ZTE Open ( Android 4.0.4) Fennec 35 Beta 8 build 1 Aurora 36 ( 2014-12-29) Nightly 37 ( 2014-12-30) Using the above devices and build I am not able to see the element mentioned. Closing it as verified ( should be worksforme but prefer the fixed as there is a confirmed patch aka verified on 36)
Status: RESOLVED → VERIFIED
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•