Filippo Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170906100107

Steps to reproduce:

Open about:addons.
Select "Extensions" tab.
Click on the "gear" button.


Actual results:

Firefox shows a menu with a font that is bigger than the font that Windows uses for menus.


Expected results:

Firefox should show menu with the same font that Windows uses for menus.

	:Gijs (he/him)
	Comment 1 • 7 years ago

Is this being tracked in bug 1397121? Ricky?

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 2 • 7 years ago

This is definitely a regression from bug 1392532, but it doesn't get fixed by bug 1397121. I'm taking at look of this now. Thanks for reporting.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review182212

::: toolkit/themes/osx/global/in-content/common.css:132
(Diff revision 1)
> -html|html *,
> +/* html|html *,
>  xul|page *,
>  xul|window * {
>    font-size: 1.36rem;
> +} */

I'm guessing you meant to delete this?

::: toolkit/themes/osx/global/in-content/common.css:145
(Diff revision 1)
> +xul|textbox,
> +xul|tree {
> +  font-size: 1.36rem;
>  }
>  
> -*|*.header-name {
> +xul|label.header-name {

about:debugging uses an <h1 class="header-name"/>, I think this change will break stop applying this style there. Is that intentional?

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 11 • 7 years ago

All broken font size of in-content pages such as about:preferences, about:debugging, about:config, about:support, about:addons...etc, which is a regression from bug 1392532 that will be addressed here.

Patch is ready for review and I've addressed open issues and plus a groupbox alignment issue (see vbox groupbox + groupbox in patch) in colors.xul.

Thanks

	Dão Gottwald [:dao]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review182312

::: browser/themes/shared/incontentprefs/preferences.inc.css:33
(Diff revision 6)
>    padding: 0;
>    font: message-box;
>    color: currentColor;
>  }
>  
> -groupbox + groupbox {
> +vbox groupbox + groupbox {

What exactly is this about? "A groupbox preceded by a groupbox, contained in a vbox" isn't a meaningful UI pattern, because a vbox doesn't have any meaning associated with it; it's just a random container.

::: toolkit/themes/linux/global/in-content/common.css:124
(Diff revision 6)
>  
> -html|html *,
> -xul|page *,
> -xul|window * {
> +xul|*:not(menuitem) > xul|label,
> +xul|description,
> +xul|textbox,
> +xul|tree,
> +*|button {

This looks fragile and like a recipe for new inconsistencies. Font sizes are inherited by default, so I'm not really sure why we're trying to set an explicit size on every single element. Can these selectors be changed to just html|html, xul|page and xul|window without the universal selector?

	Gary [:streetwolf52]
	Comment 13 • 7 years ago

FYI... Context menus still have oversized fonts such as on about:downloads.  Also, the gear icon on about:addons has the same large font.  I'm on inbound which has this patch.

	Gary [:streetwolf52]
	Comment 14 • 7 years ago

(In reply to Gary [:streetwolf] from comment #13)
> FYI... Context menus still have oversized fonts such as on about:downloads. 
> Also, the gear icon on about:addons has the same large font.  I'm on inbound
> which has this patch.

Posted in the wrong bug report.  I posted at Bug 1397121

	Jared Wein [:jaws] (please needinfo? me)
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review182418

Marking r- due to comment #12.

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 16 • 7 years ago
mozreview-review-reply

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review182312

> What exactly is this about? "A groupbox preceded by a groupbox, contained in a vbox" isn't a meaningful UI pattern, because a vbox doesn't have any meaning associated with it; it's just a random container.

This change is aiming at fixing "and align groupbox for colors.xul" in commit message. groupbox elements in colors.xul are under a hbox which should be vertical alignment. groupbox + groupbox rule is taking effect to groupbox which are vertical layout under vbox in main.xul or privacy.xul...etc.

I've turned `vbox groupbox + groupbox` into `prefpane > groupbox + groupbox` instead since `prefpane` is groupbox's parent.

> This looks fragile and like a recipe for new inconsistencies. Font sizes are inherited by default, so I'm not really sure why we're trying to set an explicit size on every single element. Can these selectors be changed to just html|html, xul|page and xul|window without the universal selector?

Yep, it could be fragile to some extent. But `html|html, xul|page and xul|window` exactly cause many font size broken in in-content pages (about:addons, about:support, about:download...etc). So that's the reason why I removed it.

There are many in-content pages (e.g. about:support, context menu in about:addons) using `em` value as their font size unit, therefore changing the base font size with `html|html *, xul|page * and xul|window *` will mess up all child elements font-size unexpectedly. That's the main root cause of this regression and user keeps reporting font-size issues recently.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 18 • 7 years ago

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

Redirecting to Dao since he already has taken a pass through this.

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 25 • 7 years ago

The groupbox alignment issue in colors.xul will be addressed in https://bugzilla.mozilla.org/show_bug.cgi?id=1398050#c9

	Dão Gottwald [:dao]
	Comment 27 • 7 years ago
mozreview-review

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review184356

::: toolkit/themes/linux/global/in-content/common.css:124
(Diff revision 14)
>  
> -html|html *,
> -xul|page *,
> -xul|window * {
> +xul|*:not(menuitem) > xul|label,
> +xul|description,
> +xul|textbox,
> +xul|tree,
> +*|button {

I still don't think this makes sense. We should instead set the font-size on the root element and rely on inheritance (not the universal selector). If this affects pages it wasn't supposed to affect, then we should introduce a class for this, or move it away from common.css.

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 33 • 7 years ago

Dao,

I've reverted back to universal selector and moved them into /in-content/preferences.css. The current changes has been verified on all platforms. I'm sure this will not affect to other in-content pages.

	Dão Gottwald [:dao]
	Comment 34 • 7 years ago
mozreview-review

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review184916

::: browser/themes/linux/preferences/in-content/preferences.css:11
(Diff revision 19)
>  
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +@namespace html url("http://www.w3.org/1999/xhtml");
> +
> +html|html *,
> +page *,

Why are you using the universal selector rather than relying on inheritance?

::: browser/themes/linux/preferences/in-content/preferences.css:12
(Diff revision 19)
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +@namespace html url("http://www.w3.org/1999/xhtml");
> +
> +html|html *,
> +page *,
> +window * {

Is this stylesheet actually used in a document where the root element is <html> or <window>?

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 35 • 7 years ago
mozreview-review-reply

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review184916

> Why are you using the universal selector rather than relying on inheritance?

Per discussion with Evan (He was working on bug 1392532 at the beginning), this universal selector is intentional and in order to override some existing font-size. If we rely on font-size inheritance, there would be some exceptional and unobvious elements which need to be override.

You can just give it a try. Just remove * and see what happen.

> Is this stylesheet actually used in a document where the root element is <html> or <window>?

Some sub-dialogs content will be loaded as html or window. Like "Saved Addresses" sub-dialog in privacy panel. Remove them will break the sub-dialog.

	Dão Gottwald [:dao]
	Comment 36 • 7 years ago

(In reply to Ricky Chien [:rickychien] from comment #35)
> Comment on attachment 8905378 [details]
> Bug 1397325 - Fix broken font size for in-content pages
> 
> https://reviewboard.mozilla.org/r/177162/#review184916
> 
> > Why are you using the universal selector rather than relying on inheritance?
> 
> Per discussion with Evan (He was working on bug 1392532 at the beginning),
> this universal selector is intentional and in order to override some
> existing font-size.

Well, can we get rid of those rules? For instance this one seems to be causing problems:
https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/toolkit/themes/shared/in-content/common.inc.css#107

It seems like we should remove this, or adjust its value using em (not rem which common.inc.css seems to overuse).

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 38 • 7 years ago

Sure, I've removed it. Thanks

	Dão Gottwald [:dao]
	Comment 39 • 7 years ago

(In reply to Ricky Chien [:rickychien] from comment #38)
> Sure, I've removed it. Thanks

So, if we consistently remove or fix up similar rules, can we get rid of the universal selector?

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 40 • 7 years ago

After checking, I'd like to revert this change back since we can't ensure this kind of removal won't break other in-contnet pages.

For example, my current patch has reverted `font-size: 1.25rem;` back for osx (toolkit/themes/osx/global/in-content/common.css). This value will take effect on about:addons's description element. Therefore, I believe there are many elements like that we can't just get rid of them.

In addition to consistently removing these similar font-size rules for common.css, we have to do the same removal for dialogs.css, manageDialog.css, editDialog.css, editAddress.css...and so on to make sure we can cover all exceptions. There are all beyond preferences scope so I'm afraid I can ensure to not cause regression.

I hope this change makes sense to you and it's safe enough to ride to the train (Merge day is approaching!).

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 42 • 7 years ago

Patch update with reverting comment 38.

	Dão Gottwald [:dao]
	Comment 43 • 7 years ago
mozreview-review

Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review185006

	Pulsebot
	Comment 44 • 7 years ago

Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fdcf075106c
Fix broken font size for in-content pages r=dao

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 45 • 7 years ago

Backed out for failing browser-chrome's browser_parsable_css.js:

https://hg.mozilla.org/integration/autoland/rev/beec84e86e7e0537db2b12f2e4d12e383910936a

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6fdcf075106c85a71ff1668d93df4b971159988d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=131042253&repo=autoland

[task 2017-09-14T14:13:11.043885Z] 14:13:11     INFO -  198 INFO Ignored error for chrome://browser/content/aboutaccounts/main.css because of filter.
[task 2017-09-14T14:13:11.046787Z] 14:13:11     INFO -  Buffered messages finished
[task 2017-09-14T14:13:11.049888Z] 14:13:11    ERROR -  199 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for chrome://browser/skin/preferences/in-content/preferences.css: Unrecognized at-rule or error parsing at-rule ‘@namespace’. -
[task 2017-09-14T14:13:11.055102Z] 14:13:11     INFO -  Stack trace:
[task 2017-09-14T14:13:11.057423Z] 14:13:11     INFO -  chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:messageIsCSSError:187
[task 2017-09-14T14:13:11.060959Z] 14:13:11     INFO -  chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:359

	Pulsebot
	Comment 47 • 7 years ago

Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebd6591b323f
Fix broken font size for in-content pages r=dao

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 48 • 7 years ago
bugherder

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

	Hani Yacoub, Desktop QA
	Comment 49 • 7 years ago

Build ID: 20170914220209
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.