1345432 - Clean up XUL checkbox styling on Linux

Status: RESOLVED FIXED

(Toolkit :: Themes, enhancement)

Description

[Dão Gottwald [:dao]]

The checkbox-with-spacing binding was implemented in bug 247631 when we transitioned from gtk1 to gtk2. I can't seem to figure out what this code is good for these days, so this is an attempt to clean this up. If this goes well, I'll try to do the same for <radio>.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1345432 - Clean up XUL checkbox styling, remove fallback styling and fix the focus indicator.

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

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8844860 [details]
Bug 1345432 - Clean up XUL checkbox styling, remove fallback styling and fix the focus indicator.

https://reviewboard.mozilla.org/r/118182/#review120726

This is probably good, thanks, but I don't have a good feel for what uses this and there seem to be some behavior changes, rather than this being a pure tidy up, so please document those change in the commit message.

::: toolkit/themes/linux/global/checkbox.css:20
(Diff revision 1)
> -  -moz-box-align: center;
>  }
>  
>  .checkbox-label-box {
>    -moz-appearance: checkbox-label;
> +  -moz-box-align: center;

Preventing the icon and label from stretching to the height of the other is a behavior change, isn't it?

I don't know what icon/src is for and that sounds reasonable, but please document the change in the commit message.

I wonder why checkbox has box-align center only when in-content/common.css is used?

::: toolkit/themes/linux/global/checkbox.css
(Diff revision 1)
> -checkbox[checked="true"] > .checkbox-spacer-box > .checkbox-check {
> -  background-image: url("chrome://global/skin/checkbox/cbox-check.gif");
> -}

This and other changes are removing support for checkboxes when native theming is disabled and in-content/common.css is not included, aren't they?

If so, please document that in the commit message.

What actually caused these images to be hidden when native theming was enabled and in-content/common.css was not included?
(I don't see a -moz-appearance for .checkbox-check in existing styling.)

Comment 3

[Dão Gottwald [:dao]]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> >  .checkbox-label-box {
> >    -moz-appearance: checkbox-label;
> > +  -moz-box-align: center;
> 
> Preventing the icon and label from stretching to the height of the other is
> a behavior change, isn't it?
> 
> I don't know what icon/src is for and that sounds reasonable, but please
> document the change in the commit message.

toolkit/themes/windows/global/checkbox.css doesn't do this so I guess I'll just drop it.

> I wonder why checkbox has box-align center only when in-content/common.css
> is used?

I'll add it here to be consistent with toolkit/themes/windows/global/checkbox.css.

> ::: toolkit/themes/linux/global/checkbox.css
> (Diff revision 1)
> > -checkbox[checked="true"] > .checkbox-spacer-box > .checkbox-check {
> > -  background-image: url("chrome://global/skin/checkbox/cbox-check.gif");
> > -}
> 
> This and other changes are removing support for checkboxes when native
> theming is disabled and in-content/common.css is not included, aren't they?
> 
> If so, please document that in the commit message.

Yes, this motivated me to look into this in the first place. See bug 1341048.

> What actually caused these images to be hidden when native theming was
> enabled and in-content/common.css was not included?
> (I don't see a -moz-appearance for .checkbox-check in existing styling.)

I believe -moz-appearance: checkbox; on the parent (.checkbox-spacer-box) took care of this.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8844860 [details]
Bug 1345432 - Clean up XUL checkbox styling, remove fallback styling and fix the focus indicator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118182/diff/1-2/

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8844860 [details]
Bug 1345432 - Clean up XUL checkbox styling, remove fallback styling and fix the focus indicator.

https://reviewboard.mozilla.org/r/118182/#review121370

::: toolkit/themes/linux/global/checkbox.css:49
(Diff revision 2)
>  
>  /* ::::: checkmark image ::::: */
>  
>  .checkbox-check {
> -  border: 2px solid;
> -  -moz-border-top-colors: ThreeDShadow ThreeDDarkShadow;
> +  -moz-appearance: checkbox;
> +  margin: 2px;

I guess in-content/common.css should override this margin to keep current behavior? 
It has "margin-inline-end: 10px", but I don't see anything else.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8844860 [details]
Bug 1345432 - Clean up XUL checkbox styling, remove fallback styling and fix the focus indicator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118182/diff/2-3/

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8844860 [details]
Bug 1345432 - Clean up XUL checkbox styling, remove fallback styling and fix the focus indicator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118182/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/613fb48bd2a3
Clean up XUL checkbox styling, remove fallback styling and fix the focus indicator. r=karlt

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/613fb48bd2a3

Comment 10

[Laszlo Ersek]

Dão,

can this patch (git commit 40034ab5b7a7, "Bug 1345432 - Clean up XUL checkbox styling, remove fallback styling and fix the focus indicator. r=karlt", 2017-03-13) be the cause of 1362875 perhaps?

Thanks
Laszlo

Comment 11

[Laszlo Ersek]

I meant to write "... the cause of bug 1362875".

Comment 12

[Dão Gottwald [:dao]]

(In reply to Laszlo Ersek (RH) from comment #11)
> I meant to write "... the cause of bug 1362875".

No, afaik we don't use the <checkbox> element to render these checkmarks.