Closed
Bug 1412743
Opened 7 years ago
Closed 7 years ago
Stylo: Font Scaling Regression in FF57 Betas on Win7 (64-bit)
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | + | fixed |
firefox58 | --- | fixed |
People
(Reporter: arbonarbot, Assigned: emilio)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
jfkthame
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171024165158
Steps to reproduce:
1. Launched Firefox after upgrading from 56 stable to the latest 57 beta release (12).
2. Visited some pages and noticed that font-scaling was way off, and text was obscured on a number of pages.
3. Verified that it was a setting I have been using in my user.js file with Firefox since the 52 releases on Windows and Linux (I have font.size.systemFontScale set to 125, because everything is way too small on HiDPI screens and using layout.css.devPixelsPerPx makes the FF UI and images on websites look absolutely atrocious), by removing the offending preference and resetting it.
4. Tried lowering the value of the offending preference but unless it was set to 110 or below, text would still get obscured on multiple sites. 110 is still far too small for my HiDPI screen.
5. Reverted to Firefox 56 stable.
Actual results:
Fonts appear to scale incorrectly in 57 when using the font.size.systemFontScale preference. They are often obscured, and far larger than the setting would enable in previous releases of Firefox.
Expected results:
Fonts should have scaled in the same way they did in previous releases of Firefox. Alternatively, mozilla could implement a preference that allows for a default zoom level on all sites. This would enable users like me to enjoy legible websites, and simply increase the font size in FF's interface via userchrome.css if necessary. NoSquint Plus and all similar extensions do not work as they used to when ported to webextensions. They all refresh the page after it loads with the desired zoom level, which makes browsing the web a profoundly obnoxious experience.
P.S. Attached is an image demonstrating the difference.
P.P.S. There is another bug pictured in the example image (hint: look at the context menus) which is related but separate.
Comment 1•7 years ago
|
||
Could you try running mozregression-gui[1] and getting specific patch which caused thus regression?
It will speed up things in fixing it.
Some information how to use mozregression-gui, is here [2]
[1] = https://github.com/mozilla/mozregression/releases
[2] = http://mozilla.github.io/mozregression/quickstart.html
Severity: normal → major
Status: UNCONFIRMED → NEW
Has Regression Range: --- → no
Has STR: --- → yes
status-firefox56:
--- → unaffected
status-firefox57:
--- → affected
status-firefox58:
--- → ?
status-firefox-esr52:
--- → unaffected
Ever confirmed: true
Flags: needinfo?(arbonarbot)
Keywords: regression,
regressionwindow-wanted
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Comment 2•7 years ago
|
||
Also, if you go to about:config, find the option "layout.css.servo.enabled", and set it to false (just double-clicking should toggle the value), then restart the browser, does that make any difference to the behavior you see?
I've installed the GUI for it but don't know when I'll be able to start testing builds (grad school responsibilities). Will try to get to it ASAP though.
Flags: needinfo?(arbonarbot)
On the latest beta now, and after toggling layout.css.servo.enabled to false, fonts appear to scale correctly again. Hope that helps!
Comment 5•7 years ago
|
||
(In reply to Arbo from comment #4)
> On the latest beta now, and after toggling layout.css.servo.enabled to
> false, fonts appear to scale correctly again. Hope that helps!
OK, thanks. That suggests the bug is related to font-size computation when Stylo is enabled (and the font.size.systemFontScale pref is adjusted): moving to the CSS Parsing & Computation component.
Component: Graphics: Text → CSS Parsing and Computation
Summary: Font Scaling Regression in FF57 Betas on Win7 (64-bit) → Stylo: Font Scaling Regression in FF57 Betas on Win7 (64-bit)
Comment 6•7 years ago
|
||
I can confirm this happens in Nightly on macOS when stylo is enabled and font.size.systemFontScale is set to a non-default value. It doesn't seem to affect all elements on the page, but some elements are having the scale applied twice.
E.g. on the slashdot.org home page, the horizontal "menu" across the top (Topics: | Devices | Build | Entertainment...) appears at 12px with default settings. When I set font.size.systemFontScale to 150, with stylo *disabled*, this text gets a computed size of 18px, which is exactly what I'd expect. But with stylo *enabled*, it jumps up to 27px, which is 12px * 150% * 150%.
I'm not seeing this "double scaling" in simple examples I try locally, though. I guess there must be something about the structure of the CSS slashdot is using, but I haven't identified what it is.
Comment 7•7 years ago
|
||
FWIW, this reminds me somewhat of bug 1391341, though I don't know if it's really a symptom of the same underlying issue.
Blocks: stylo-site-issues
Updated•7 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> I can confirm this happens in Nightly on macOS when stylo is enabled and
> font.size.systemFontScale is set to a non-default value. It doesn't seem to
> affect all elements on the page, but some elements are having the scale
> applied twice.
>
> E.g. on the slashdot.org home page, the horizontal "menu" across the top
> (Topics: | Devices | Build | Entertainment...) appears at 12px with default
> settings. When I set font.size.systemFontScale to 150, with stylo
> *disabled*, this text gets a computed size of 18px, which is exactly what
> I'd expect. But with stylo *enabled*, it jumps up to 27px, which is 12px *
> 150% * 150%.
>
> I'm not seeing this "double scaling" in simple examples I try locally,
> though. I guess there must be something about the structure of the CSS
> slashdot is using, but I haven't identified what it is.
What looks to be different is the usage of the rem unit. I bet we're incorrectly scaling them or what not.
Assignee | ||
Comment 10•7 years ago
|
||
Changing slashdot from:
body {
font: 13px/1.5 Arial,sans-serif;
}
to:
html, body {
font: 13px/1.5 Arial,sans-serif;
}
Makes it go back to normal. I suspect we're incorrectly scaling up system font size values?
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
I have no clue how to properly test it, since I haven't reduced a test-case... I suspect I could try though...
Assignee | ||
Comment 13•7 years ago
|
||
Here's a reduced test-case:
<!doctype html>
<html lang="en">
<style>
p { font-size: 0.8rem; }
</style>
<p>Which size am I?</p>
</html>
This is because of the weird stuff we do when lang changes.
Assignee | ||
Comment 14•7 years ago
|
||
Of course you don't even need rem units:
<!doctype html>
<html lang="en">
<p>Which size am I?</p>
</html>
Assignee | ||
Comment 15•7 years ago
|
||
Xidorn or Jonathan, do you know how can I write a reftest such as that the test is rendered with a pref, but not the reference?
In particular, I want to do something like:
pref(font.size.systemFontScale,200) test.html ref.html
Where test.html is:
<!doctype html>
<html lang="en">
<p>Which size am I?</p>
</html>
and ref.html is:
<!doctype html>
<html lang="en">
<p style="font-size: 2em">Which size am I?</p>
</html>
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(manishearth)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → emilio
Comment 16•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> Xidorn or Jonathan, do you know how can I write a reftest such as that the
> test is rendered with a pref, but not the reference?
>
> In particular, I want to do something like:
>
> pref(font.size.systemFontScale,200) test.html ref.html
>
> Where test.html is:
>
> <!doctype html>
> <html lang="en">
> <p>Which size am I?</p>
> </html>
>
> and ref.html is:
>
> <!doctype html>
> <html lang="en">
> <p style="font-size: 2em">Which size am I?</p>
> </html>
I believe you can annotate with test-pref(...) instead of just pref(...) in the manifest, and the pref will be applied only to the testcase and not the reference.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8923819 [details]
Bug 1412743: Test this.
https://reviewboard.mozilla.org/r/194966/#review200074
LGTM, assuming you've checked that it fails with current trunk code (prior to the fix here).
Attachment #8923819 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #19)
> Comment on attachment 8923819 [details]
> Bug 1412743: Test this.
>
> https://reviewboard.mozilla.org/r/194966/#review200074
>
> LGTM, assuming you've checked that it fails with current trunk code (prior
> to the fix here).
Yup, I had an unpatched release build handy, and it fails there :)
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8923771 [details]
Bug 1412743: Avoid double-applying text-zoom for keywords.
https://reviewboard.mozilla.org/r/194922/#review200096
Attachment #8923771 -
Flags: review?(manishearth) → review+
Updated•7 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee | ||
Comment 22•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8923771 -
Attachment is obsolete: true
Comment 24•7 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bc4844e42230
Test this. r=jfkthame
Comment 25•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 26•7 years ago
|
||
Please nominate this for Beta approval ASAP.
Flags: needinfo?(emilio)
Flags: in-testsuite+
Priority: -- → P1
Assignee | ||
Comment 27•7 years ago
|
||
Comment on attachment 8923819 [details]
Bug 1412743: Test this.
This request is for this patch and https://hg.mozilla.org/integration/autoland/rev/95e8712dbf3b, which is the actual fix.
Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: too big font rendering for users with font scaling or text-only zoom in some cases.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: Just avoids incorrectly applying zoom twice while computing the font size of an element if that element doesn't specify font-size itself and specifies lang="". Patch is pretty simple.
[String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8923819 -
Flags: approval-mozilla-beta?
Comment on attachment 8923819 [details]
Bug 1412743: Test this.
Stylo related, Beta57+
Attachment #8923819 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 30•7 years ago
|
||
Flags: needinfo?(emilio)
Comment 31•7 years ago
|
||
bugherder uplift |
Comment 32•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no
Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•