1435658 - radio/checkbox widget don't resize to the default size from changing appearance none to its default appearance

Status: NEW

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Karl Dubost💡 :karlcow]

Attachment: radio button appearance issue

On top the reference test case. (working) 
   data:text/html,<input type="radio">
   The widget is 13x13 pixels (on macOS)


The second window. Here the steps to reproduce.

1. Start with 
   data:text/html,<input type="radio" style="-moz-appearance: none;">
2. open the devtools
3. select input
4. Change the value of "-moz-appearance" 
   from "none" 
   to   "radio"

The widget is 4x4 pixels.

It should be back to the default.

(Screenshot shows values with "!important", but this is not necessary to reproduce the issue. This is a spin-off of Bug 1434298 )


Testing other values:

Same thing with
data:text/html,<input type="checkbox" style="-moz-appearance: none;">

Comment 1

[Mats Palmgren (inactive)]

I can reproduce the problem on Linux too, but I get an 18x0 size
when removing the -moz-appearance:none.

However, if I start with data:text/html,<input type="radio">
and then add -moz-appearance:none in devtools and toggle that
declaration it works as expected, toggling between 18x18 and 0x0.

I'm guessing we don't re-create the frame as we should when
changing -moz-appearance between radio/checkbox and none.
I suspect this is now necessary because of bug 605985.

Comment 2

[Mats Palmgren (inactive)]

Emilio, is the C++ code still used to calculate change hints?
If so, then I think we could add something similar to this:
https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/layout/style/nsStyleStruct.cpp#3779

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Yes, it is. That looks fine to me, want me to write the patch?

Comment 4

[Mats Palmgren (inactive)]

Sure, if you feel you have the time.  Otherwise, I'll get to it eventually.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Okie dookie, shouldn't take too much time.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1435658: Deal with appearance changes from / to none correctly.

Review commit: https://reviewboard.mozilla.org/r/217838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217838/

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8948372 [details]
Bug 1435658: Deal with appearance changes from / to none correctly.

https://reviewboard.mozilla.org/r/217838/#review223618

Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c074e2c4b2ed
Deal with appearance changes from / to none correctly. r=mats

Comment 9

[Noemi Erli[:noemi_erli]]

Backed out changeset c074e2c4b2ed (bug 1435658) for failing in layout/generic/test/test_bug632379.xul on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c074e2c4b2ed6c733479cad11204244edb9e51ef&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=160446851&repo=autoland&lineNumber=2863

Backout: https://hg.mozilla.org/integration/autoland/rev/338ff3f5f22792b564e2e9c1a8bf97768e0a1e5a

Comment 10

[Emilio Cobos Álvarez (:emilio)]

For more fun, I can't repro them locally...

Comment 11

[Mats Palmgren (inactive)]

The failing test used to be intermittent (bug 1207914) so maybe that
has something to do with it?  I'd say that if the original STR in
bug 632379 can't be reproduced with this patch then we should reland
it and disable the test instead.  Clearly it's dependent on a reflow
happening/not happening which seems flaky anyway.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Hmm, so just pushed on top of something that isn't that mess of yesterday's trees.

There are a bunch of accessibility tests failing, and a couple of others, most of which are marked as intermittent...  I suspect we don't want to disable all of them, so I'll try to figure out what's up with them tomorrow, hopefully at least one repros...

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=06e547ff3ef1473de47865cc7391d81e9731f9a1

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Any more progress on this patch?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Milan Sreckovic [:milan] from comment #13)
> Any more progress on this patch?

It's on my list of things to do, need to take time for it though.

Comment 15

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P3 ]

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Will this make 60? It's a new regression in that release, it'd be good if we didn't ship it, then fix it later, since 60 is ESR.

Comment 17

[Jet Villegas (inactive)]

(In reply to Milan Sreckovic [:milan] from comment #16)
> Will this make 60? It's a new regression in that release

Bug 605985 was fixed in FF54, what landed in 60 to cause the new regression?

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Sounds like this doesn't affect 60.  Marking as such.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(I never got around to fix the tests, but Jwatt is looking at appearance and may come through these)

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Jonathan, any chance that you could take a look at this while you're looking at appearance? It was mostly bogus tests IIRC.

Comment 21

[rctgamer3]

I am still able to reproduce this with 118.0b2.