Closed Bug 1301500 Opened 3 years ago Closed 3 years ago

Div with transform: translate(-50%, -50%) isn't centered correctly

Categories

(Core :: Layout, defect, P1)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 + fixed
firefox52 --- fixed

People

(Reporter: evilpie, Assigned: dbaron)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

This presentation was just posted (I think this even is a Mozilla presentation template :). The div should be centered, but instead the presentation content is in the bottom right corner.
Bisect says:

The first bad revision is:
changeset:   311679:c206d60dc0bf
user:        L. David Baron <dbaron@dbaron.org>
date:        Mon Aug 29 11:43:29 2016 -0700
summary:     Bug 1251075 - Optimize away nsChangeHint_UpdateContainingBlock in nsStyleContext::CalcStyleDifference when possible.  r=bz
Blocks: 1251075
No longer blocks: 1258911
Flags: needinfo?(dbaron)
So the one time I see us hit the code from bug 1251075 on this testcase, we're going from "no transform" to "has a transform".  Both the old and the new style test true for HasPerspectiveStyle(), which is why we optimize out the containing-block change.

I bet the problem is that in RestyleManagerBase::ProcessRestyledFrames the handling of nsChangeHint_UpdateContainingBlock does something other than just twiddling the containing block bits: it also updates the NS_FRAME_MAY_BE_TRANSFORMED bit.  I had missed that somehow.  And I expect in this case we're failing to update it...
It seems like the obvious thing is to add another changehint for updating the NS_FRAME_MAY_BE_TRANSFORMED flag, and not mask it out even if we mask out nsChangeHint_UpdateContainingBlock.  Not sure what the pitfalls of adding a new changehint are at this point.
Tracking 51+ for this layout regression.
Hi Astley,
Not sure if you can provide any help here?
Flags: needinfo?(aschen)
Keywords: regression
Priority: -- → P1
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #8789655 - Attachment is obsolete: true
Attached patch Add reftestSplinter Review
Test by bzbarsky; reference by dbaron.

The test passes with the patch.  With the changes to
RestyleManagerBase::ProcessRestyledFrames reverted, the test shows the
expected mispositioning resulting from not applying the transform.

MozReview-Commit-ID: 7oIQFD8QKUn
Attachment #8795045 - Flags: review?(cam)
Flags: needinfo?(aschen)
Flags: needinfo?(dbaron)
Attachment #8795045 - Flags: review?(cam) → review+
Comment on attachment 8795044 [details] [diff] [review]
Separate change hint for adding/removing transform from UpdateContainingBlock

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

::: layout/base/RestyleManagerBase.cpp
@@ +1125,5 @@
>            // Normally frame construction would set state bits as needed,
>            // but we're not going to reconstruct the frame so we need to set them.
>            // It's because we need to set this state on each affected frame
>            // that we can't coalesce nsChangeHint_UpdateContainingBlock hints up
>            // to ancestors (i.e. it can't be an inherited change hint).

While you're here, could you do a s/inherited change hint/change hint that is handled for descendants/ or similar?

@@ +1147,5 @@
> +           cont = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
> +        if (cont->StyleDisplay()->HasTransform(cont)) {
> +          cont->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
> +        }
> +        // Don't remove NS_FRAME_MAY_BE_TRANSFORMED since it may still by

May as well s/by/be/ while moving this comment.
Attachment #8795044 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/5fd6dd2bdbfa
https://hg.mozilla.org/mozilla-central/rev/56055d850969
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8795044 [details] [diff] [review]
Separate change hint for adding/removing transform from UpdateContainingBlock

Approval Request Comment
[Feature/regressing bug #]: bug 1251075
[User impact if declined]: incorrect layout of web pages
[Describe test coverage new/current, TreeHerder]: reftest added
[Risks and why]: reasonably low risk; makes sure we run the code that we were previously running
[String/UUID change made/needed]: no
Attachment #8795044 - Flags: approval-mozilla-aurora?
Attachment #8795045 - Flags: approval-mozilla-aurora?
Comment on attachment 8795044 [details] [diff] [review]
Separate change hint for adding/removing transform from UpdateContainingBlock

Fix a regression related to layout and add tests. Take it in 51 aurora.
Attachment #8795044 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8795045 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Version: unspecified → 51 Branch
You need to log in before you can comment on or make changes to this bug.