Closed Bug 1086723 Opened 5 years ago Closed 5 years ago

Problems with fixed position element on forbes.com

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox33 --- unaffected
firefox34 + wontfix
firefox35 + verified
firefox36 + verified
fennec 34+ ---

People

(Reporter: snorp, Assigned: kip)

References

Details

(Keywords: regression, reproducible)

Attachments

(4 files, 5 obsolete files)

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.
Works on release
tracking-fennec: --- → ?
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
OS: Mac OS X → Android
Hardware: x86 → ARM
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?
Assignee: milan → nobody
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bugmail.mozilla)
(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)
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)
I can take this one.
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)
tracking-fennec: ? → 34+
[Tracking Requested - why for this release]: regression in 34
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)
Have reproduced, am narrowing it down now
Flags: needinfo?(kgilbert)
The issue is introduced in the Part 4 patch of Bug 1042423.  Perhaps it is the same root cause as Bug 1067286.
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.
- 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)
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
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)
You don't need to needinfo me if you already flagged me for review :)
Flags: needinfo?(bugmail.mozilla)
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 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-
- 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)
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 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+
- 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
The v3 patch is the same as the v2 patch
- 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
(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.
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)
Can you attach the test? AFAIK you should be able to test this fine with reftest-async-scroll.
Flags: needinfo?(bugmail.mozilla) → needinfo?(kgilbert)
Attached patch Bug 1086723 - Part 2: Test (obsolete) — Splinter Review
- 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)
(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
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)
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)
(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.
- 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)
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 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+
- 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)
Attachment #8525560 - Flags: review?(bugmail.mozilla) → review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e552e9152594
https://hg.mozilla.org/mozilla-central/rev/1e3a1de38584
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Verified as fixed in Firefox for Android 36.0a1 (2014-11-23)
Device: Asus Transformer Pad TF300T (Android 4.2.1)
Too late to take this in 34. Let's see about uplifting to 35.
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)
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?
Attachment #8525560 - Attachment description: Bug 1086723 - Part 2: Test (V3 Patch) → Bug 1086723 - Part 2: Test (V3 Patch),r=kats
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?
Attachment #8523256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8525560 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
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
Depends on: 1145730
You need to log in before you can comment on or make changes to this bug.