Closed Bug 1317795 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Theme, defect)

52 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + verified
firefox53 --- verified

People

(Reporter: Virtual, Assigned: mconley)

References

()

Details

(Keywords: nightly-community, regression, ux-consistency)

Attachments

(3 files)

[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
Has STR: --- → yes
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
This is fallout from bug 1309316. I hope to fix this with bug 418833 (and hopefully get the uplift).
Blocks: 1309316
Depends on: 418833
Thank you very much for finding the regression range.
Has Regression Range: --- → yes
Version: 53 Branch → 52 Branch
(ah, cool, I was gonna get around to looking into why the checkbox in about:debugging looks different... seems like this is why!)
Please don't forget to fix the switch in about:privatebrowsing
Adding also to it about:networking and about:performance
Summary: Checkbox looks inconsistent in "Insecure Connection" warning compared to the other Firefox checkboxes → Checkbox looks inconsistent in "Insecure Connection" warning & about:debugging/networking/performance/privatebrowsing, compared to the other Firefox checkboxes
Component: Untriaged → General
Duplicate of this bug: 1317804
The regression for about:privatebrowsing can (and should) be fixed separately, so I've re-opened bug 1317804.
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?
Flags: needinfo?(ntim.bugs)
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
(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 :)
Flags: needinfo?(ntim.bugs)
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 :)
Good call, thank you!
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 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 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;
Attachment #8812499 - Flags: review?(jaws) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e241e4cec47a
Fix broken alignment of in-content UI checkboxes. r=jaws
https://hg.mozilla.org/mozilla-central/rev/e241e4cec47a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Tracking 53- since this is now resolved fixed.
Mike, can you please request Aurora approval on this when you get a chance?
Assignee: nobody → mconley
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Attachment #8812499 - Flags: approval-mozilla-aurora?
tracking for 52 as new regression
Comment on attachment 8812499 [details]
Bug 1317795 - Fix broken alignment of in-content UI checkboxes.

styling fix for aurora52
Attachment #8812499 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(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.
I've filed bug 1321302 for Part 2, and bug 1321306 for Part 3.
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.