1264143 - "No notification will be shown…" text is no longer aligned with the "Do not disturb me" checkbox label

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Matthew N. [:MattN]]

Attachment: Image diff 03_prefsContent.png

Compare 03_prefsContent at https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=d62963756d9a9d19cbbb5d8f3dd3c7cfa8fdef88&newProject=mozilla-central&newRev=e847cfcb315f511f4928b03fd47dcf57aad05e1e

I suspect this is from the specificity change in https://hg.mozilla.org/mozilla-central/rev/1af4c6693497#l2.1 which was unreviewed in bug 1263016. It seems like the change to toolkit/themes/shared/in-content/common.inc.css wasn't intended for that bug.

I think there's a non-trivial chance this will regress other in-content pages too with the specificity change.

Comment 1

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

Indeed that change was very unintentional and somehow slipped in to that patch from some work on bug 1247214 which landed on top of it. See https://hg.mozilla.org/mozilla-central/rev/4eea4fcaf495#l4.13

Comment 2

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

Attachment: Patch

Comment 3

[:Gijs (he/him)]

Comment on attachment 8740767 [details] [diff] [review]
Patch

Review of attachment 8740767 [details] [diff] [review]:
-----------------------------------------------------------------

Why are we setting this to 0 so broadly in the first place? From bug 993369, which introduced these, seems like this was originally intended for use in menulists? Of course, by now loads of random other labels rely on this rule to be aligned properly with the subheadings. :-\

Increasing the specificity will fix the .indent case, but not any hypothetical other issues alluded to in comment #0. r=me assuming no such cases exist, probably worth doing a mozscreenshots trypush to find that out. Leaving r?MattN in case he has better ideas.

Comment 4

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

Richard, do you see a way that we could make those selectors more specific?

Comment 5

[Richard Marti (:Paenglab)]

Not really. What would work would be adding an id in front like #mainPrefPane .indent {} but this would make the parsing slower and #mainPrefPane would not include the dialogs (I don't know if they use .indent).

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8740767 [details] [diff] [review]
Patch

(In reply to :Gijs Kruitbosch from comment #3)
> Why are we setting this to 0 so broadly in the first place? From bug 993369,
> which introduced these, seems like this was originally intended for use in
> menulists? Of course, by now loads of random other labels rely on this rule
> to be aligned properly with the subheadings. :-\

I also wonder why we don't revisit the error in bug 993369 instead of a band-aiding.

> Increasing the specificity will fix the .indent case, but not any
> hypothetical other issues alluded to in comment #0. r=me assuming no such
> cases exist, probably worth doing a mozscreenshots trypush to find that out.

Instructions are at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots#Try_pushes for try pushes.

> Leaving r?MattN in case he has better ideas.

I don't have better ideas other than revisiting the change from bug 993369.

Comment 7

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

I ran this through mozscreenshots and there were no differences reported.

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=86730d0a82093d705e44f33a34973d28b269f1ea&newProject=try&newRev=e4efe4a4a291878fc587737d2edbefe49c19316d

Comment 8

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> I ran this through mozscreenshots and there were no differences reported.

But note that you ran the primary UI configurations but didn't run the preference ones which are what caught this regression:
https://mxr.mozilla.org/mozilla-central/source/browser/tools/mozscreenshots/preferences/browser_preferences.js

You want `--setenv MOZSCREENSHOTS_SETS=Preferences`

Comment 9

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

Sorry, I didn't realize that. Repushed to tryserver, https://treeherder.mozilla.org/#/jobs?repo=try&revision=a785d53a5349

Comment 10

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

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=86730d0a82093d705e44f33a34973d28b269f1ea&newProject=try&newRev=a785d53a5349aeeb3c591094e691a4ca7e11bf53&filter=.*prefs.*

Shows no difference on Linux, where do-not-disturb is not supported/implemented. Only differences on OSX/Windows is the DND line. Previous mozscreenshots run showed no differences in any of the other tests.

Between the two mozscreenshot runs, only differences were expected.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ec30b1b889eb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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