1352406 - Back out bug 418833 and friends for 53 (beta)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

See bug 605985 comment 98 for rationale.

Here's, roughly, what I'm going to do:

0) Back out bug 1320809 from beta (https://hg.mozilla.org/mozilla-central/rev/b55a8d9517c8), since this removed some dead code that is going to be used again once bug 418833 is backed out.

1) Back out as much of bug 418833 on beta as needed.

The list of revisions for that bug, in the order to back out, are:

https://hg.mozilla.org/mozilla-central/rev/5df1453ded4c
https://hg.mozilla.org/mozilla-central/rev/61f417a156ac
https://hg.mozilla.org/mozilla-central/rev/0691e88c6ff2
https://hg.mozilla.org/mozilla-central/rev/9f7debc99bf8
https://hg.mozilla.org/mozilla-central/rev/ec772ba1b1d9
https://hg.mozilla.org/mozilla-central/rev/a9da196a7884
https://hg.mozilla.org/mozilla-central/rev/eecb0af8a88f

Note that bug 1258916 changed the same reftest.list rule as 5df1453ded4c, so I'm skipping that part of the backout for now. I'll do some try runs to see if we still need the fuzziness bump that was added in bug 1258916. If not, we can revert back to no fuzziness.

Note that bug 1319628 changed the test that 0691e88c6ff2 added the longer timeout to, so backing this out was a no-op.

2) Graft the patch from bug 1321302 (https://hg.mozilla.org/releases/mozilla-aurora/rev/f6f2e2b9fa87) on to beta, as this wallpapers over the lack of bug 418833 (which we did for an earlier Aurora).

3) Graft the patch from bug 1321302 (https://hg.mozilla.org/releases/mozilla-aurora/rev/f6f2e2b9fa87) on to beta, as this also wallpapers over the lack of bug 418833.


Note that this is a slight departure from the plan I originally laid out in bug 605985 comment 98, because it's not necessary to back out bug 1317795 as I had originally suspected (we never did that originally for Aurora 52 either).

I also had to add the bug 1320809 backout due to the dead code removal which broke my backout.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey mats,

I've got some reftest orange on my try push of the backouts:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ab0c1cc629a0668cbdee9cfb6c1e3a51523ad24

Reftest Analyzer: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/YN3z-2BBT1WflV7IJl6LHA/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

This test was added by you in bug 1322698. Should we expect it to fail here? Or did I somehow break the patch you landed in bug 1322698 with this backout?

Comment 2

[Mats Palmgren (inactive)]

I suspect the failures comes from adding back !important on the borders in the UA sheet.
The test depends on being able to set the border - does it pass if you add !important here:
https://dxr.mozilla.org/mozilla-beta/source/layout/reftests/forms/input/checkbox/checkbox-baseline.html#26,33

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mats Palmgren (:mats) from comment #2)
> I suspect the failures comes from adding back !important on the borders in
> the UA sheet.
> The test depends on being able to set the border - does it pass if you add
> !important here:
> https://dxr.mozilla.org/mozilla-beta/source/layout/reftests/forms/input/
> checkbox/checkbox-baseline.html#26,33

Nope, it does not. Which is strange - I don't know why the UA !important isn't being overridden here by the page author !important.

Did I break your fix for bug 1322698? Or is it expected to fail with this set of backouts?

Comment 4

[Mats Palmgren (inactive)]

Hmm, odd.  But it seems checkbox border styles have no effect in v52 either:
data:text/html,<input type=checkbox style="-moz-appearance:none; border:5px solid red !important">
so I think we should just disable that test.

Comment 5

[Mats Palmgren (inactive)]

Oh, I forgot the cascading makes it impossible for authors to override UA !important declarations:
https://drafts.csswg.org/css-cascade-4/#cascade-origin
(I haven't had my morning coffee yet! ;-) )

Comment 6

[Mats Palmgren (inactive)]

To be clear: this backout won't break the fix in bug 1322698, so please
go ahead and land your patches (with that test disabled).

Comment 7

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c7b0760083d

Comment 8

[Mats Palmgren (inactive)]

Regarding the layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3.xhtml failure -
reverting the first hunk in this patch should fix it I think:
https://hg.mozilla.org/mozilla-central/rev/326d947d6bb6

Otherwise, we should consider backing out bug 1322698 too if that's easier.
I don't think it's particularly important to ship that in v53.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Tweaking the test didn't help. Here's a try push with bug 1322698 backed out.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7821a2c49e765daadaab6ca0e7e0d7ca698b7922

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Hm. Backing out bug 1322698 doesn't help either - flexbox-align-self-baseline-horiz-3-ref.xhtml still fails. Any ideas, mats?

Reftest analyzer: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-7821a2c49e765daadaab6ca0e7e0d7ca698b7922/try-macosx64-debug/try_yosemite_r7-debug_test-reftest-e10s-2-bm133-tests1-macosx-build668.txt.gz&only_show_unexpected=1

Comment 11

[Mats Palmgren (inactive)]

I suggest we keep bug 1322698 then and simply remove the part of the test
that fails, i.e. the second flexbox:
https://dxr.mozilla.org/mozilla-beta/source/layout/reftests/flexbox/flexbox-align-self-baseline-horiz-3.xhtml#47-51
(and in -ref)

Comment 12

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1694c4f2b39a0d1891cc3b14e10ff13a1a453abb

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 1 - Back out changeset b55a8d9517c8 (bug 1320809)

MozReview-Commit-ID: LCiH5P1L7bD

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 2 - Backed out changeset 61f417a156ac (Bug 418833)

MozReview-Commit-ID: 4R6PYCBDQXy

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 3 - Back out changeset 9f7debc99bf8 (bug 418833)

MozReview-Commit-ID: EThljjMniOt

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 4 - Back out changeset ec772ba1b1d9 (bug 418833)

MozReview-Commit-ID: JdVvSHMlZMG

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 5 - Back out changeset a9da196a7884 (bug 418833)

MozReview-Commit-ID: FlnWSlJ9Lvz

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 6 - Back out changeset eecb0af8a88f (bug 418833)

MozReview-Commit-ID: BEyRfEmwjZY

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 7 - Remove parts of flexbox-align-self-baseline-horiz-3 reftest that are broken by the backout of bug 418833

MozReview-Commit-ID: H0tmuypTMbg

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 8 - Disable checkbox-baseline.html reftest due to backout of bug 418833

MozReview-Commit-ID: 6IcMypjRLQh

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 9 - Re-apply fix from bug 1321302 to make in-content checkboxes pseudoelements again

Originally reviewed by Gijs at https://bugzilla.mozilla.org/show_bug.cgi?id=1321302#c3

Checkboxes (and radio inputs) haven't traditionally been very stylable in Gecko, even with
-moz-appearance: none;. That was fixed in bug 418833, but those patches are being backed out
of 53 (see bug 1352406).

In bug 1309316, I had counted on getting bug 418833 being in 53, and so had written some CSS
to switch us to using traditional checkboxes (with the new styling abilities) rather than the
old mechanism, which was a hacky pseudoelement solution. Since bug 418833 was backed out, we
have to go back to the old mechanism. That's what this patch does.

MozReview-Commit-ID: 1NfvHWpioxF

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch 10 - Re-apply bug 1321306 to make sure text wrapping works correctly in about:tabcrashed

Originally reviewed by Gijs at https://bugzilla.mozilla.org/show_bug.cgi?id=1321306#c2

MozReview-Commit-ID: Aw0xBh5Q9n0

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8854266 [details] [diff] [review]
Patch 7 - Remove parts of flexbox-align-self-baseline-horiz-3 reftest that are broken by the backout of bug 418833

Try looking pretty green.

Hey mats, I guess I'm hoping you have the chance to put another pairs of eyes on this to make sure I've disabled things correctly. The backouts in the previous patches were all mechanical, and done completely by hg, so I won't request reviews on those (if it makes the sheriffs feel better, they can just use the SHA's I put in the commit message of each preceding patch and do the backouts themselves).

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8854266 [details] [diff] [review]
Patch 7 - Remove parts of flexbox-align-self-baseline-horiz-3 reftest that are broken by the backout of bug 418833

Approval Request Comment
[Feature/Bug causing the regression]:

Back out of bug 418833

[User impact if declined]:

None.

[Is this code covered by automated tests?]:

This is an automated test.

[Has the fix been verified in Nightly?]:

No, and doesn't need to be - this is going straight up to beta.

[Needs manual test from QE? If yes, steps to reproduce]: 

Nope.

[List of other uplifts needed for the feature/fix]:

The rest of the backouts and patches in this bug.

[Is the change risky?]:

Not this one, no.

[Why is the change risky/not risky?]:

Simple test change.

[String changes made/needed]:

None.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8854267 [details] [diff] [review]
Patch 8 - Disable checkbox-baseline.html reftest due to backout of bug 418833

Approval Request Comment
[Feature/Bug causing the regression]:

Back out of bug 418833

[User impact if declined]:

None.

[Is this code covered by automated tests?]:

This is just disabling an automated test.

[Has the fix been verified in Nightly?]:

No, and doesn't need to be - this is going straight up to beta.

[Needs manual test from QE? If yes, steps to reproduce]: 

Nope.

[List of other uplifts needed for the feature/fix]:

The rest of the backouts and patches in this bug.

[Is the change risky?]:

Not this one, no.

[Why is the change risky/not risky?]:

Simple test disabling.

[String changes made/needed]:

None.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8854266 [details] [diff] [review]
Patch 7 - Remove parts of flexbox-align-self-baseline-horiz-3 reftest that are broken by the backout of bug 418833

OK for beta, Let the wild rumpus begin.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Wes, or tomcat, can you uplift all the patches here to beta and do the backouts as mconley describes? Thanks!

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Landing instructions:

If I get beta uplift approval on attachment 8854266 [details] [diff] [review] and attachment 8854267 [details] [diff] [review], as well as beta uplift approval in bug 1321302 and bug 1321306, then we're ready to go.

Here's what one needs to do:

1) Check out beta
2) Back out the following revisions, in this order:

b55a8d9517c8 (bug 1320809)
61f417a156ac (bug 418833)
9f7debc99bf8 (bug 418833)
ec772ba1b1d9 (bug 418833)
a9da196a7884 (bug 418833)
eecb0af8a88f (bug 418833)

3) Land the two test fixes in this bug

And then finally,

4) Graft f6f2e2b9fa87 (bug 1321302) from the aurora repository
5) Graft f10d78addd57 (bug 1321306) from the aurora repository

And you're done.

Comment 29

[Carsten Book [:Tomcat]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #27)
> Wes, or tomcat, can you uplift all the patches here to beta and do the
> backouts as mconley describes? Thanks!

will take this!

Comment 30

[Carsten Book [:Tomcat]]

also waiting for the approvals in bug 1321302 and bug 1321306

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Tomcat wrote me in IRC last night:

"hey :) just to be safe the test fixes are part 7 and part 8 right ?"

That's correct. They should be part 7 and 8 of the patches being applied to beta.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5322792c38b3
https://hg.mozilla.org/releases/mozilla-beta/rev/1f94cae5a272
https://hg.mozilla.org/releases/mozilla-beta/rev/6232ab8c49a1
https://hg.mozilla.org/releases/mozilla-beta/rev/756c82559f94
https://hg.mozilla.org/releases/mozilla-beta/rev/52a40cd84b9a
https://hg.mozilla.org/releases/mozilla-beta/rev/9e2440ee6715
https://hg.mozilla.org/releases/mozilla-beta/rev/c8a0dd29b2bb
https://hg.mozilla.org/releases/mozilla-beta/rev/531bff516821
https://hg.mozilla.org/releases/mozilla-beta/rev/cfc4e84b9679
https://hg.mozilla.org/releases/mozilla-beta/rev/6e2ce3dd9d87