Closed Bug 1397325 Opened 2 years ago Closed 2 years ago

Fix broken font size in in-content pages

Categories

(Firefox :: Theme, defect, P1)

57 Branch
Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + verified

People

(Reporter: filippo.mannino, Assigned: rickychien)

References

Details

(Keywords: regression, Whiteboard: [photon-preference])

Attachments

(2 files)

Attached image Immagine.png
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.
Component: Untriaged → Theme
OS: Unspecified → Windows 10
Is this being tracked in bug 1397121? Ricky?
Flags: needinfo?(rchien)
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.
Assignee: nobody → rchien
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(rchien) → qe-verify+
Priority: -- → P1
Target Milestone: --- → Firefox 57
Whiteboard: [photon-preference][triage]
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?
Attachment #8905378 - Flags: review?(jaws) → review-
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage] → [photon-preference]
Duplicate of this bug: 1397352
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
Summary: Extensions manager menu font is too big → Fix broken font size in in-content pages and align groupbox for colors.xul
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?
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.
(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
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.
Attachment #8905378 - Flags: review?(jaws) → review-
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.
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.
Attachment #8905378 - Flags: review?(jaws) → review?(dao+bmo)
The groupbox alignment issue in colors.xul will be addressed in https://bugzilla.mozilla.org/show_bug.cgi?id=1398050#c9
Summary: Fix broken font size in in-content pages and align groupbox for colors.xul → Fix broken font size in in-content pages
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.
Attachment #8905378 - Flags: review?(dao+bmo) → review-
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.
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>?
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.
(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).
Sure, I've removed it. Thanks
(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?
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!).
Flags: needinfo?(dao+bmo)
Patch update with reverting comment 38.
Comment on attachment 8905378 [details]
Bug 1397325 - Fix broken font size for in-content pages

https://reviewboard.mozilla.org/r/177162/#review185006
Attachment #8905378 - Flags: review?(dao+bmo) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fdcf075106c
Fix broken font size for in-content pages r=dao
Flags: needinfo?(dao+bmo)
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
Flags: needinfo?(rchien)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebd6591b323f
Fix broken font size for in-content pages r=dao
https://hg.mozilla.org/mozilla-central/rev/ebd6591b323f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Flags: needinfo?(rchien)
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.