Closed Bug 1357169 Opened 7 years ago Closed 7 years ago

Back out bug 418833 and friends for 54 and 55 too.

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(8 files, 2 obsolete files)

Bug 1357169 - Back out changeset b55a8d9517c8 (bug 1320809).
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1357169 - Backed out changeset 61f417a156ac (Bug 418833).
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1357169 - Back out changeset 9f7debc99bf8 (bug 418833).
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1357169 - Manual back out of changeset eecb0af8a88f (bug 418833).
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1357169 - Remove parts of flexbox-align-self-baseline-horiz-3 reftest that are broken by the backout of bug 418833.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1357169 - Don't apply some unnecessary style rules to checkboxes or radio inputs on Android.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1357169 - Make radio-stretched reftest pass expectedly, and disable two reftests for Android.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1357169 - Remove checkmark.svg, indeterminate-checkmark.svg and radio.svg from browser_all_files_referenced.js test.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
gchang
: approval-mozilla-beta+
Details 
+++ This bug was initially created as a clone of Bug #1352406 +++

This was done for 53 on the beta channel.

See bug 605985 comment 98 for rationale. See bug 1352238 comment 12 for rationale for landing on 55 and 54.

I'm essentially going to be backporting the commits from bug 1352406 onto Nightly, and then request uplift to Aurora.

Oh boy.
Hey mats,

So part of the backout involves landing commits like this: https://hg.mozilla.org/releases/mozilla-beta/rev/52a40cd84b9a and https://hg.mozilla.org/releases/mozilla-beta/rev/9e2440ee6715#l7.2 which conflict with recent changes in forms.css (like bug 605985).

Could you advise me on how best to resolve these conflicts?

Alternatively, it might make more sense if you drive this backout. The revisions you need to graft from mozilla-beta are in bug 1352406 comment 32.
Flags: needinfo?(mats)
https://hg.mozilla.org/releases/mozilla-beta/rev/52a40cd84b9a 
Bug 605985 moved these into the MOZ_WIDGET_ANDROID section.
I think you can skip these changes because those declarations will soon
be gone anyway.

https://hg.mozilla.org/releases/mozilla-beta/rev/9e2440ee6715#l7.2 
We want all changes for
layout/forms/nsGfxCheckboxControlFrame.*
layout/forms/nsGfxRadioControlFrame.*
but please wrap them in "#ifdef MOZ_WIDGET_ANDROID".
(I don't think there are any real conflicts here, just that the surrounding
code changed.)

layout/style/jar.mn + *.svg:
I think we can remove these SVG images now since they won't be used,
at least not in the UA sheet.

layout/style/res/forms.css:
Again, I think the background-color/border declarations are now under MOZ_WIDGET_ANDROID,
but you can leave them as is b/c they'll be removed soon.
But you should remove the SVG background rules since we'll paint them in C++ now:
http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/layout/style/res/forms.css#639-669

Once you land this, I can do the "part 2/3" as outlined in bug 1352238 comment 6
to fix bug 605985 also for Android.  But we can do that here too if it proves hard
to get tests to pass without those changes.
There's already a WIP patch for that in bug 1352238, all that is needed in addition
to that is removing all the MOZ_WIDGET_ANDROID stuff in forms.css (AFAIK).
Flags: needinfo?(mats)
> I think we can remove these SVG images now since they won't be used,
> at least not in the UA sheet.

FYI, I haven't looked at all at the dependencies on bug 418833, so there
might be some HTML for about:preferences and such pages that still want
to style their checkboxes using these images.  If so, those pages should
include them in their style sheets as normal.
It turns out that we don't even need

https://hg.mozilla.org/releases/mozilla-beta/rev/cfc4e84b9679
https://hg.mozilla.org/releases/mozilla-beta/rev/6e2ce3dd9d87

since the hardcoded background colours and borders are, as mats says, Android-only for now. That's nice. We did need them for 53 though.
Getting some reftest failures, it seems. Hey mats, see any obvious fixes?
Flags: needinfo?(mats)
layout/reftests/bugs/1174332-1.html
looks like the checkbox in the test have a border-radius?
odd, since it's a checkbox, not a radio

layout/reftests/forms/input/checkbox/radio-stretched.html
unexpected PASS -- dunno why

layout/reftests/forms/input/checkbox/checkbox-radio-auto-sized.html 
this is a test for bug 1344395, which was a regression from bug 605985,
so probably not your fault, just comment out this test in the manifest
and I'll take a look when I fix bug 605985 for Android.

layout/reftests/webcomponents/input-transition-1.html
some minor anti-aliasing issue
Flags: needinfo?(mats)
I think the first failure above is a bit surprising...
Can post a rollup patch please?
Flags: needinfo?(mconley)
Attached patch bug1357169-rollup.diff (obsolete) — — Splinter Review 
Sure, here you are.
Flags: needinfo?(mconley)
"border-radius: var(--form_border_radius)" in mobile/android/themes/core/content.css
looks a bit suspicious to me.  The selector used to exclude checkbox/radio but your
patch now includes them again AFAICT.  Perhaps that rule depended on some other
declaration that is now gone?  Does the tests pass if you leave this rule as is?
> Perhaps that rule depended on some other declaration that is now gone?

The rejected lines in forms.css perhaps?
https://hg.mozilla.org/releases/mozilla-beta/rev/9e2440ee6715#l7.2
Maybe the "!important" on the border there made it override this rule?
(In reply to Mats Palmgren (:mats) from comment #16)

Thanks for the extra pair of eyes, I'll fiddle with both and see what I get today.
Hey mats, I was able to fix most of the reftests with this additional patch: https://hg.mozilla.org/try/rev/778f81cdb60975fd6c5015bae4d3511d78ac6856

But I've got 2 more reftest failures that I don't currently believe are things I can fix in the content.css file.

Specifically, I'm failing:

layout/reftests/forms/input/checkbox/checkbox-radio-auto-sized.html which you added in bug 1344395

and

layout/reftests/writing-mode/1134744-radio-checkbox-baseline-1.html which seems to have something to do with how margins are applied to checkboxes and radios when under different orientations.

Got any tips to how I can approach these so we can get this stuff landed?
Flags: needinfo?(mats)
> layout/reftests/forms/input/checkbox/checkbox-radio-auto-sized.html

I suspect this now fails due to the padding we're adding back in content.css.
We should either just tweak the hardcoded values in the -ref file or mark it
as skip/fail for now.  We can take a look again after we have the native theme.

> layout/reftests/writing-mode/1134744-radio-checkbox-baseline-1.html

Looks like the same.
Flags: needinfo?(mats)
Blocks: 1328474
Attachment #8859319 - Attachment is obsolete: true
Comment on attachment 8859316 [details]
Bug 1357169 - Back out changeset b55a8d9517c8 (bug 1320809).

https://reviewboard.mozilla.org/r/131340/#review136554
Attachment #8859316 - Flags: review?(mats) → review+
Comment on attachment 8859317 [details]
Bug 1357169 - Backed out changeset 61f417a156ac (Bug 418833).

https://reviewboard.mozilla.org/r/131342/#review136556
Attachment #8859317 - Flags: review?(mats) → review+
Comment on attachment 8859318 [details]
Bug 1357169 - Back out changeset 9f7debc99bf8 (bug 418833).

https://reviewboard.mozilla.org/r/131344/#review136558
Attachment #8859318 - Flags: review?(mats) → review+
Comment on attachment 8859320 [details]
Bug 1357169 - Manual back out of changeset eecb0af8a88f (bug 418833).

https://reviewboard.mozilla.org/r/131348/#review136560
Attachment #8859320 - Flags: review?(mats) → review+
Comment on attachment 8859321 [details]
Bug 1357169 - Remove parts of flexbox-align-self-baseline-horiz-3 reftest that are broken by the backout of bug 418833.

https://reviewboard.mozilla.org/r/131350/#review136562
Attachment #8859321 - Flags: review?(mats) → review+
Comment on attachment 8861276 [details]
Bug 1357169 - Don't apply some unnecessary style rules to checkboxes or radio inputs on Android.

https://reviewboard.mozilla.org/r/133228/#review136564
Attachment #8861276 - Flags: review?(mats) → review+
Comment on attachment 8861277 [details]
Bug 1357169 - Make radio-stretched reftest pass expectedly, and disable two reftests for Android.

https://reviewboard.mozilla.org/r/133230/#review136566
Attachment #8861277 - Flags: review?(mats) → review+
Comment on attachment 8861616 [details]
Bug 1357169 - Remove checkmark.svg, indeterminate-checkmark.svg and radio.svg from browser_all_files_referenced.js test.

https://reviewboard.mozilla.org/r/133588/#review136568
Attachment #8861616 - Flags: review?(mats) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a44e180f9d1
Back out changeset b55a8d9517c8 (bug 1320809). r=mats
https://hg.mozilla.org/integration/autoland/rev/d5b5afcd6c58
Backed out changeset 61f417a156ac (Bug 418833). r=mats
https://hg.mozilla.org/integration/autoland/rev/16a97f073369
Back out changeset 9f7debc99bf8 (bug 418833). r=mats
https://hg.mozilla.org/integration/autoland/rev/d9698ac70397
Manual back out of changeset eecb0af8a88f (bug 418833). r=mats
https://hg.mozilla.org/integration/autoland/rev/b874be12f102
Remove parts of flexbox-align-self-baseline-horiz-3 reftest that are broken by the backout of bug 418833. r=mats
https://hg.mozilla.org/integration/autoland/rev/6d97b699a54f
Don't apply some unnecessary style rules to checkboxes or radio inputs on Android. r=mats
https://hg.mozilla.org/integration/autoland/rev/0799721b455f
Make radio-stretched reftest pass expectedly, and disable two reftests for Android. r=mats
https://hg.mozilla.org/integration/autoland/rev/681f14e0e953
Remove checkmark.svg, indeterminate-checkmark.svg and radio.svg from browser_all_files_referenced.js test. r=mats
ni?ing myself to request uplift.
Flags: needinfo?(mconley)
Attachment #8859731 - Attachment is obsolete: true
Flags: needinfo?(mconley)
Comment on attachment 8859316 [details]
Bug 1357169 - Back out changeset b55a8d9517c8 (bug 1320809).

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

Bug 418833 and the things that landed afterwards.

[User impact if declined]:

There are some cases where CSS rules can render checkboxes and radio buttons invisible on Android.

[Is this code covered by automated tests?]:

Reftests, yes.

[Has the fix been verified in Nightly?]:

Yes.

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

I don't think so, no. This is mostly a back out of a few things. We did a similar back out for beta 53 in bug 1352406, which we just shipped to release, so I think this is pretty safe.

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

Thankfully, I think I've got all of the required patches contained within this bug.

[Is the change risky?]:

No.

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

We're essentially reverting to a known good state that we've shipped for a long time (though there's been some slight re-organization in where the Android-specific rules are going).

[String changes made/needed]:

None.
Attachment #8859316 - Flags: approval-mozilla-beta?
Comment on attachment 8859317 [details]
Bug 1357169 - Backed out changeset 61f417a156ac (Bug 418833).

See comment 42.
Attachment #8859317 - Flags: approval-mozilla-beta?
Comment on attachment 8859318 [details]
Bug 1357169 - Back out changeset 9f7debc99bf8 (bug 418833).

See comment 42.
Attachment #8859318 - Flags: approval-mozilla-beta?
Comment on attachment 8859320 [details]
Bug 1357169 - Manual back out of changeset eecb0af8a88f (bug 418833).

See comment 42.
Attachment #8859320 - Flags: approval-mozilla-beta?
Comment on attachment 8859321 [details]
Bug 1357169 - Remove parts of flexbox-align-self-baseline-horiz-3 reftest that are broken by the backout of bug 418833.

See comment 42.
Attachment #8859321 - Flags: approval-mozilla-beta?
Comment on attachment 8861276 [details]
Bug 1357169 - Don't apply some unnecessary style rules to checkboxes or radio inputs on Android.

See comment 42.
Attachment #8861276 - Flags: approval-mozilla-beta?
Comment on attachment 8861277 [details]
Bug 1357169 - Make radio-stretched reftest pass expectedly, and disable two reftests for Android.

See comment 42.
Attachment #8861277 - Flags: approval-mozilla-beta?
Comment on attachment 8861616 [details]
Bug 1357169 - Remove checkmark.svg, indeterminate-checkmark.svg and radio.svg from browser_all_files_referenced.js test.

See comment 42.
Attachment #8861616 - Flags: approval-mozilla-beta?
Comment on attachment 8859316 [details]
Bug 1357169 - Back out changeset b55a8d9517c8 (bug 1320809).

Revert to a known good state. Beta54+. Should be in 54 beta 4.
Attachment #8859316 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8859317 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8859318 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8859320 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8859321 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8861276 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8861277 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8861616 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1352238
(In reply to Mike Conley (:mconley) from comment #42)
> [User impact if declined]:
> 
> There are some cases where CSS rules can render checkboxes and radio buttons
> invisible on Android.
> 
> [Is this code covered by automated tests?]:
> 
> Reftests, yes.
> 
> [Has the fix been verified in Nightly?]:
> 
> Yes.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> 
> I don't think so, no. This is mostly a back out of a few things. We did a
> similar back out for beta 53 in bug 1352406, which we just shipped to
> release, so I think this is pretty safe.

Setting qe-verify- based on Mike's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: