1062127 - Style the in-content preference dialogs

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

Style the in-content preference dialogs after bug 1036090 which has the design specs

Comment 1

[Richard Marti (:Paenglab)]

Attachment: incontentDialogStyling.patch

I added a special style sheet for the dialogs to not need to add always 
dialog something,
window something,
prefpane something,
prefwindow something,
.windowDialog something {}
for special styles only used in the dialogs.

I also renamed the included preferences.css to preferences.inc.css to be correct with the naming.

I also added the needed FiraSans fonts to skin/global/fonts. Is this okay like this? I used this place because the font are now also used in global/in-content/common.css. I also used the woff files because they have the half size of the ttf files.

Jared, please can you look why in-content/tests/browser_subdialogs.js fails with: All expectedStyleSheetURLs should have been found - Got 1, strictly expected 0

The new style sheet is correctly added in subdialogs.js. What is wrong here?

https://tbpl.mozilla.org/?tree=Try&rev=1516e913df31

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8483618 [details] [diff] [review]
incontentDialogStyling.patch

> I also added the needed FiraSans fonts to skin/global/fonts. Is this okay
> like this? I used this place because the font are now also used in
> global/in-content/common.css. I also used the woff files because they have
> the half size of the ttf files.

Bug 1007629 is on file for this. Please remove it from this patch.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: incontentDialogStyling.patch

Now without the FiraSans fonts. I checked the new patch on Linux and Win 7 and the dimensions (paddings, positions) are still the same.

I forgot to write before I'm using a border-radius of 2.5px instead 2px for the dialogs. With 2px I saw no radius.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8483654 [details] [diff] [review]
incontentDialogStyling.patch

>+  skin/classic/browser/preferences/in-content/incontentDialogs.css (../shared/incontentprefs/incontentDialogs.css)

nit: please rename this file to just dialog.css

>+/**
>+ * Sub-dialog
>+ */

"Dialog"

>+.close-icon {
>+  margin-top: -32px;
>+  -moz-margin-end: -15px;
>+}

Not sure what's going on with all these negative margins. Seems like the layout around the title bar is somewhat whacky, but I'm willing to let this pass for now.

>+label,
>+button,
>+description,
>+.tab-text,
>+caption > label {
>+  font-size: 14px;
>+  line-height: 158%;
>+}

Please use em instead of px.

Why do you need to specify a line-height here? I'm concerned that this will mess up the text for exotic scripts.

>+tabbox {
>+  /* override the rule in certManager.xul */
>+  margin: 0 0 5px !important;
>+}

While this is a fine short-term hack, can you please file a bug on cleaning this up? It's not clear to me why certManager.xul is doing that in the first place.

>+hbox button + button {
>+  -moz-margin-start: 6px;
>+}

Not sure what part of the UI you're targeting here. Can you be more specific than "hbox"? Also, can you avoid the descendent selector?

>+prefpane .groupbox-body {
>+  padding: inherit;
>+}

Inheriting a padding seems strange to me. Where do you mean to inherit this from anyway?

Comment 5

[Richard Marti (:Paenglab)]

Attachment: incontentDialogStyling.patch

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 8483654 [details] [diff] [review]
> incontentDialogStyling.patch
> 
> >+  skin/classic/browser/preferences/in-content/incontentDialogs.css (../shared/incontentprefs/incontentDialogs.css)
> 
> nit: please rename this file to just dialog.css

Done

> >+/**
> >+ * Sub-dialog
> >+ */
> 
> "Dialog"
> 
> >+.close-icon {
> >+  margin-top: -32px;
> >+  -moz-margin-end: -15px;
> >+}
> 
> Not sure what's going on with all these negative margins. Seems like the
> layout around the title bar is somewhat whacky, but I'm willing to let this
> pass for now.

This is because of the different system fonts with their different baseline position.

> >+label,
> >+button,
> >+description,
> >+.tab-text,
> >+caption > label {
> >+  font-size: 14px;
> >+  line-height: 158%;
> >+}
> 
> Please use em instead of px.
> 
> Why do you need to specify a line-height here? I'm concerned that this will
> mess up the text for exotic scripts.

The 14px and the line-height are coming from the Design Specs form bug 1036090 (http://people.mozilla.org/~mmaslaney/incontent/Preferences-dialog.png). I removed the line-height but leaved the 14px because it's harder to override all the rules from chrome://global with relative sizes (for example a description doesn't have the same size when directly below root or below groupbox). This would need a special rule for every case and also be different on platforms.

> >+tabbox {
> >+  /* override the rule in certManager.xul */
> >+  margin: 0 0 5px !important;
> >+}
> 
> While this is a fine short-term hack, can you please file a bug on cleaning
> this up? It's not clear to me why certManager.xul is doing that in the first
> place.
> 
> >+hbox button + button {
> >+  -moz-margin-start: 6px;
> >+}
> Not sure what part of the UI you're targeting here. Can you be more specific
> than "hbox"? Also, can you avoid the descendent selector?

This is to become 10px space between horizontal buttons (see Design Specs). There are only 2px more than default. I can also remove this if you are more comfortable with this.

> 
> >+prefpane .groupbox-body {
> >+  padding: inherit;
> >+}
> 
> Inheriting a padding seems strange to me. Where do you mean to inherit this
> from anyway?

This is a copy from http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/groupbox.css#21 to make it look the same on all platforms. It inherits the padding from groupbox which is defined in common.css and in dialog.css directly above this rule. If you like more I can change this to padding: 0 0 5px;.

Comment 6

[Richard Marti (:Paenglab)]

Dão, please can you give me a hint why the bc1 test is failing? With my limited JS knowledge I don't see why it can't find all stylesheets. I don't see a second list with the stylesheets than subdialogs.js where I have to add dialog.css, so it should all find.

Comment 7

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

Hey Paenglab,

Sorry nobody got back to you on this - I took a quick peek at the test. I'm not familiar with this stuff, but it looks like the test is failing because one of the stylesheets that's expected to be loaded is not loading (as in, it's not found in content.document.styleSheets).

I'm guessing the stylesheet in question is chrome://browser/skin/preferences/in-content/dialog.css, but I'm not sure why it wouldn't be loading. Needinfo'ing MattN who wrote this code.

Comment 8

[Richard Marti (:Paenglab)]

In normal use the stylesheet is loaded. Could it be a string length issue in test?

Comment 9

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

I tracked down the issue with your bc1 tests failing and filed bug 1071964 to fix it. I have a patch in that bug that is up for review. Your tests all pass with that patch applied :)

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8488997 [details] [diff] [review]
incontentDialogStyling.patch

(In reply to Richard Marti (:Paenglab) from comment #5)
> > >+.close-icon {
> > >+  margin-top: -32px;
> > >+  -moz-margin-end: -15px;
> > >+}
> > 
> > Not sure what's going on with all these negative margins. Seems like the
> > layout around the title bar is somewhat whacky, but I'm willing to let this
> > pass for now.
> 
> This is because of the different system fonts with their different baseline
> position.

Sorry, I don't understand. How does hardcoding those negative margins take care of different baseline positions?

> > >+label,
> > >+button,
> > >+description,
> > >+.tab-text,
> > >+caption > label {
> > >+  font-size: 14px;
> > >+  line-height: 158%;
> > >+}
> > 
> > Please use em instead of px.
> > 
> > Why do you need to specify a line-height here? I'm concerned that this will
> > mess up the text for exotic scripts.
> 
> The 14px and the line-height are coming from the Design Specs form bug
> 1036090
> (http://people.mozilla.org/~mmaslaney/incontent/Preferences-dialog.png). I
> removed the line-height but leaved the 14px because it's harder to override
> all the rules from chrome://global with relative sizes (for example a
> description doesn't have the same size when directly below root or below
> groupbox).

I'm not sure why a description would have different font sizes in those case; maybe we should address that directly on whichever container (unexpectedly?) changes the font size there. Either way it makes sense that the font size of those elements are context sensitive. I.e. changing the font size of a container is expected to affect its content. So please use em.

> > >+hbox button + button {
> > >+  -moz-margin-start: 6px;
> > >+}
> > Not sure what part of the UI you're targeting here. Can you be more specific
> > than "hbox"? Also, can you avoid the descendent selector?
> 
> This is to become 10px space between horizontal buttons (see Design Specs).
> There are only 2px more than default. I can also remove this if you are more
> comfortable with this.

I'd prefer if you removed this unless you can make the selector saner.

> > >+prefpane .groupbox-body {
> > >+  padding: inherit;
> > >+}
> > 
> > Inheriting a padding seems strange to me. Where do you mean to inherit this
> > from anyway?
> 
> This is a copy from
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> groupbox.css#21 to make it look the same on all platforms. It inherits the
> padding from groupbox which is defined in common.css and in dialog.css
> directly above this rule. If you like more I can change this to padding: 0 0
> 5px;.

I have a slight preference for explicitly setting the padding.

Comment 11

[Richard Marti (:Paenglab)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> I tracked down the issue with your bc1 tests failing and filed bug 1071964
> to fix it. I have a patch in that bug that is up for review. Your tests all
> pass with that patch applied :)

Thanks a lot for your help, Jared!

Comment 12

[Richard Marti (:Paenglab)]

Attachment: incontentDialogStyling.patch

(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 8488997 [details] [diff] [review]
> incontentDialogStyling.patch
> 
> > > >+label,
> > > >+button,
> > > >+description,
> > > >+.tab-text,
> > > >+caption > label {
> > > >+  font-size: 14px;
> > > >+  line-height: 158%;
> > > >+}
> > > 
> > > Please use em instead of px.

Changed to em. On Linux and Windows I get 14.4px and on OS X 14.3px.

> > > >+hbox button + button {
> > > >+  -moz-margin-start: 6px;
> > > >+}
> 
> I'd prefer if you removed this unless you can make the selector saner.

Removed.

> > > >+prefpane .groupbox-body {
> > > >+  padding: inherit;
> > > >+}
> 
> I have a slight preference for explicitly setting the padding.

Using now padding: 0 0 5px;

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8495357 [details] [diff] [review]
incontentDialogStyling.patch

>+#dialogTitle {
>+  font-size: 1em;
> }

>+button {
>+  font-size: 1em;
>+}

>+tabpanels {
>+  font-size: 1em;
>+}

>+groupbox {
>+  font-size: 1em;

font-size: 1em; is a no-op, unless a font-size was set elsewhere for those elements and you're overriding it here. Is this the case for all rules cited above?

>+label,
>+textbox,
>+description,
>+.tab-text,
>+caption > label {
>+  font-size: 1.3em;
>+}

Do you need 'caption > label' and '.tab-text' when you already have 'label'?

Also, wouldn't it make more sense to set font-size: 1.3em; on a container rather than individual content nodes?

Comment 14

[Richard Marti (:Paenglab)]

(In reply to Dão Gottwald [:dao] from comment #13)
> Comment on attachment 8495357 [details] [diff] [review]
> incontentDialogStyling.patch
> 
> >+#dialogTitle {
> >+  font-size: 1em;
> > }

Needed to override http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#36

> >+button {
> >+  font-size: 1em;
> >+}

Needed to override http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/dialog.css#22. font: menu makes a bigger font of 15.6px.

> >+tabpanels {
> >+  font-size: 1em;
> >+}

Needed to override http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#73

> >+groupbox {
> >+  font-size: 1em;

Needed to override http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#54

> font-size: 1em; is a no-op, unless a font-size was set elsewhere for those
> elements and you're overriding it here. Is this the case for all rules cited
> above?
> 
> >+label,
> >+textbox,
> >+description,
> >+.tab-text,
> >+caption > label {
> >+  font-size: 1.3em;
> >+}
> 
> Do you need 'caption > label' and '.tab-text' when you already have 'label'?

'caption > label' is needed to override http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#36 and '.tab-text' to override http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#114

> Also, wouldn't it make more sense to set font-size: 1.3em; on a container
> rather than individual content nodes?

As the examples are shown above set the size on a container wouldn't be enough as some individual nodes have set the font-size.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: incontentDialogStyling.patch

Patch unbitrotted.

Comment 16

[Richard Marti (:Paenglab)]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1a06145fd3e

Comment 17

[Matthew N. [:MattN]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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