1317795 - Checkbox looks inconsistent in "Insecure Connection" warning & about:debugging/networking/performance/privatebrowsing, compared to the other Firefox checkboxes

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

STR:
1. Open this website page - https://forum.doom9.org/showthread.php?t=166689
2. See that checkbox is rendered differently compared to the other checkboxes in about:preferences

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: bad checkbox.png

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: good checkbox.png

Comment 3

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

This is fallout from bug 1309316. I hope to fix this with bug 418833 (and hopefully get the uplift).

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Thank you very much for finding the regression range.

Comment 5

[Mike Taylor [:miketaylr]]

(ah, cool, I was gonna get around to looking into why the checkbox in about:debugging looks different... seems like this is why!)

Comment 6

[Tim Nguyen :ntim]

Please don't forget to fix the switch in about:privatebrowsing

Comment 7

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Adding also to it about:networking and about:performance

Comment 9

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

The regression for about:privatebrowsing can (and should) be fixed separately, so I've re-opened bug 1317804.

Comment 10

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

Attachment: Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

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

Comment 11

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

Note that this work relies on the patches in bug 418833 to look fully appropriate.

ntim, are you the right reviewer for in-content changes like this?

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

https://reviewboard.mozilla.org/r/94224/#review94372

I think Gijs or jaws would be more appropriate to review this kind of changes, but here are a few comments anyway.

::: devtools/client/aboutdebugging/aboutdebugging.css:185
(Diff revision 1)
>    flex: 1;
>  }
>  
>  .addons-debugging-label {
>    display: inline-block;
> -  margin: 0 5px 5px 0;
> +  margin-right: 5px;

nit: Use RTL aware prop (margin-inline-end).

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml:148
(Diff revision 1)
> +      .options > input[type="checkbox"]:not(:first-child) {
> +        margin-left: 10px;
> +      }

hmm, this is an ugly hack. I'd rather have two containers as children containing the 2 checkboxes.

::: toolkit/themes/shared/in-content/common.inc.css:557
(Diff revision 1)
>  
>  xul|richlistitem > xul|*.checkbox-check {
>    margin: 3px 6px;
>  }
>  
> +html|*.checkbox-container-with-text {

This is a class we'll probably be able to use with radios as well, so maybe rename it as toggle-with-label ?

::: toolkit/themes/shared/in-content/common.inc.css:565
(Diff revision 1)
> +
> +html|*.checkbox-container-with-text > label,
> +html|*.checkbox-container-with-text > span,
> +html|*.checkbox-container-with-text > a {
> +  margin-top: auto;
> +  margin-bottom: auto;

Instead of specifying margins, does using one of those flex properties work (you might need to apply them on the parent) ? I'm thinking of align-self/content

Comment 13

[Tim Nguyen :ntim]

(In reply to Mike Conley (:mconley) from comment #11)
> Note that this work relies on the patches in bug 418833 to look fully
> appropriate.
> 
> ntim, are you the right reviewer for in-content changes like this?

I think :Gijs or :jaws would be more appropriate to review this since they are browser peer. Plus I'm not accepting review requests right now :)

Comment 14

[Johann Hofmann [:johannh]]

Drive-by: I haven't tried this patch locally but it doesn't seem to touch the aboutNetError/CertError page, which also has an unstyled checkbox now. Please make sure that that one is also fixed :)

Comment 15

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

Good call, thank you!

Comment 16

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

Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

https://reviewboard.mozilla.org/r/94224/#review94372

> This is a class we'll probably be able to use with radios as well, so maybe rename it as toggle-with-label ?

I chose "toggle-container-with-text", since we're dealing with non-label nodes as well (<span>, <a>, at least)

Comment 17

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

Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

https://reviewboard.mozilla.org/r/94224/#review94372

> Instead of specifying margins, does using one of those flex properties work (you might need to apply them on the parent) ? I'm thinking of align-self/content

Ah, align-items on the container was the rule I wanted. Thank you!

Comment 18

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

Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

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

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

https://reviewboard.mozilla.org/r/94224/#review94710

::: devtools/client/aboutdebugging/aboutdebugging.css:185
(Diff revision 2)
>    flex: 1;
>  }
>  
>  .addons-debugging-label {
>    display: inline-block;
> -  margin: 0 5px 5px 0;
> +  margin-inline-end: 5px;

Please use 1ch here instead of 5px.

::: toolkit/components/aboutperformance/content/aboutPerformance.xhtml:152
(Diff revision 2)
> +      }
> +      .options > .toggle-container-with-text {
> +        display: inline-flex;
> +      }
> +      .options > .toggle-container-with-text:not(:first-child) {
> +        margin-left: 10px;

margin-inline-start: 2ch;

Comment 20

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

Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e241e4cec47a
Fix broken alignment of in-content UI checkboxes. r=jaws

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e241e4cec47a

Comment 23

[Marcia Knous [:marcia]]

Tracking 53- since this is now resolved fixed.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Mike, can you please request Aurora approval on this when you get a chance?

Comment 25

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

Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

Approval Request Comment
[Feature/regressing bug #]:

Bug 1309316, bug 418833

[User impact if declined]:

Checkboxes and their labels will be slightly misaligned.

[Describe test coverage new/current, TreeHerder]:

All manual, as this is a styling thing.

[Risks and why]: 

Very low risk. This just adjusts the majority of the in-content checkboxes to use the new unstyled checkbox properly.

[String/UUID change made/needed]:

None.

Comment 26

[Julien Cristau [:jcristau]]

tracking for 52 as new regression

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

styling fix for aurora52

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c3bd290fee9

Comment 29

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

Note that it looks like bug 418833 is not going to uplift. This bugfix kinda depended on that, so this might get backed out in short order. I'm going to work out a good solution with UX before making a move.

Comment 30

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

Here is my proposal for fixing this on Aurora 52:

1) Back out this patch from Aurora, since bug 418833 will not uplift.
2) Undo the changes from bug 1309316 that switch us to using the standard checkbox, and put us back to the pseudoelement checkbox
3) Use some margin and text-indent hackery to make text wrapping work acceptably well on about:tabcrashed. This will probably have to do until 53. I'll need to get UX sign-off on this.

The first two steps can be done immediately. Step 1 is trivial. Step 2, I will have a patch prepared for today.

Comment 31

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

I've filed bug 1321302 for Part 2, and bug 1321306 for Part 3.