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)

57 Branch
defect

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)

Attached image FF5657CompareBugFont.png (deleted) —
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.
Component: Untriaged → Graphics: Text
Product: Firefox → Core
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
Ever confirmed: true
Flags: needinfo?(arbonarbot)
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
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!
(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)
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.
FWIW, this reminds me somewhat of bug 1391341, though I don't know if it's really a symptom of the same underlying issue.
ni? manish for stylo font-size issue :)
Flags: needinfo?(manishearth)
(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.
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?
I have no clue how to properly test it, since I haven't reduced a test-case... I suspect I could try though...
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.
Of course you don't even need rem units: <!doctype html> <html lang="en"> <p>Which size am I?</p> </html>
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>
Flags: needinfo?(manishearth)
Assignee: nobody → emilio
(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 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+
(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 :)
Attachment #8923771 - Flags: review?(manishearth) → review+
OS: Windows 7 → All
Hardware: x86_64 → All
Attachment #8923771 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Please nominate this for Beta approval ASAP.
Flags: needinfo?(emilio)
Flags: in-testsuite+
Priority: -- → P1
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+
Needs a rebased patch for Beta.
Flags: needinfo?(emilio)
Attached patch beta.patch (deleted) — Splinter Review
Flags: needinfo?(emilio)
(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.

Attachment

General

Creator:
Created:
Updated:
Size: