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

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Tom S [:evilpie]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Regression range on nightlies: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1a5b53a831e5a6c20de1b081c774feb3ff76756c&tochange=26e22af660e543ebb69930f082188b69ec756185

Probably regression from bug 1258911....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Bisect says:

The first bad revision is:
changeset:   311679:c206d60dc0bf
user:        L. David Baron <[e-mail]>
date:        Mon Aug 29 11:43:29 2016 -0700
summary:     Bug 1251075 - Optimize away nsChangeHint_UpdateContainingBlock in nsStyleContext::CalcStyleDifference when possible.  r=bz

Comment 3

[Boris Zbarsky [:bzbarsky]]

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...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple testcase showing the problem

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Marcia Knous [:marcia]]

Tracking 51+ for this layout regression.

Comment 7

[Gerry Chang [:gchang]]

Hi Astley,
Not sure if you can provide any help here?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase I meant to attach

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Separate change hint for adding/removing transform from UpdateContainingBlock

MozReview-Commit-ID: FwEQDA327EI

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Add reftest

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

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd6dd2bdbfaafbd7c48d3a804c15ec7fc769782
Bug 1301500 - Separate change hint for adding/removing transform from UpdateContainingBlock.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/56055d850969c7ce96aeb988079c8685428172f5
Bug 1301500 - Add reftest.  r=heycam

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fd6dd2bdbfa
https://hg.mozilla.org/mozilla-central/rev/56055d850969

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 15

[Gerry Chang [:gchang]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/08a7b0b71f4e
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8c205387357