Closed Bug 1352406 Opened 3 years ago Closed 3 years ago

Back out bug 418833 and friends for 53 (beta)

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox53 + fixed

People

(Reporter: mconley, Assigned: mconley)

Details

Attachments

(10 files)

1.69 KB, patch
Details | Diff | Splinter Review
1.33 KB, patch
Details | Diff | Splinter Review
1.58 KB, patch
Details | Diff | Splinter Review
3.68 KB, patch
Details | Diff | Splinter Review
1.75 KB, patch
Details | Diff | Splinter Review
13.16 KB, patch
Details | Diff | Splinter Review
1.93 KB, patch
mats
: review+
Details | Diff | Splinter Review
1.47 KB, patch
mats
: review+
Details | Diff | Splinter Review
3.29 KB, patch
Details | Diff | Splinter Review
1.71 KB, patch
Details | Diff | Splinter Review
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.
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
Flags: needinfo?(mats)
(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?
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
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! ;-) )
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).
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.
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
Flags: needinfo?(mconley)
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)
Flags: needinfo?(mats)
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 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).
Flags: needinfo?(mconley)
Attachment #8854266 - Flags: review?(mats)
Attachment #8854266 - Flags: review?(mats) → review+
Attachment #8854267 - Flags: review?(mats) → review+
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.
Attachment #8854266 - Flags: approval-mozilla-beta?
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.
Attachment #8854267 - Flags: approval-mozilla-beta?
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.
Attachment #8854266 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8854267 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Wes, or tomcat, can you uplift all the patches here to beta and do the backouts as mconley describes? Thanks!
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
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.
(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!
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
also waiting for the approvals in bug 1321302 and bug 1321306
Flags: needinfo?(jcristau)
Flags: needinfo?(gchang)
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.
You need to log in before you can comment on or make changes to this bug.