stylo: support Fennec's "Honor system font sizes" setting

RESOLVED FIXED in Firefox 58

Status

()

defect
P4
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: hiro)

Tracking

57 Branch
mozilla58
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 fixed)

Details

Attachments

(1 attachment)

The Gecko implementation was bug 1328868.

/**
 * This setting corresponds to a global text zoom setting affecting
 * all content that is not already subject to font size inflation.
 * It is interpreted as a percentage value that is applied on top
 * of the document's current text zoom setting.
 *
 * The resulting total zoom factor (text zoom * system font scale)
 * will be limited by zoom.minPercent and maxPercent.
 */
pref("font.size.systemFontScale", 100);
Priority: -- → P4
Assignee: nobody → manishearth
It looks like we already support this; Stylo already uses mEffectiveTextZoom, and always has.

Is there evidence stating that we don't support this?
Makoto, do you have more info on what's not working here?
Flags: needinfo?(m_kato)
failed log is here.

https://treeherder.mozilla.org/logviewer.html#?job_id=134365073&repo=try&lineNumber=2774

10:39:33 INFO - 188 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | reftest comparison: != fontScaleOn fontScaleOff
10:39:43 INFO - 191 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | reftest comparison: == fontScaleOn cssZoom
10:39:43 INFO - 195 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | reftest comparison: == fontScaleOn textZoom
10:40:15 INFO - 215 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | reftest comparison: != noZoom fontScaleNoFontInflation 


To build Fennec/sytlo, it requires bug 1397764.
Flags: needinfo?(m_kato)
I'm in Montreal for the week and the hashmap stuff will take priority so I probably won't be able to get to this for a while (and i don't yet have an android build). Unassigning for now. I can answer questions about the zoom stuff though.

As far as I can tell this should work; because both we and Gecko use mEffectiveTextZoom. Perhaps we don't send the correct change hint?
Assignee: manishearth → nobody
I did setup android build. I can take a look into this.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
It seems to me that the android's system font size stuff (or just the failure test case) relies on MediaFeatureValuesChanged call in nsPresContext::UpdateEffectiveTextZoom().  In stylo the call is skipped basically, if I tweaked that MediaFeatureValuesChanged is called for stylo, the failure test passed normally.

I am not yet sure whether we should fix stylo or android side, but probably we should fix android implementation (or the test case).
In the test case, test_settings_fontinflation.html, when nsPresContext::UpdateEffectiveTextZoom() is called, stylist.device.default_values has been already created with not-yet-zoomed value even if PresShell::DidInitialized() is false.  So in servo case we should regenerate the default values.  And to do that, I think calling MediaFeatureValuesChanged() is fine.
Comment on attachment 8919136 [details]
Bug 1404545 - Force to call MediaFeatureValuesChanged for stylo in UpdateEffectiveTextZoom.

https://reviewboard.mozilla.org/r/190050/#review195274
Attachment #8919136 - Flags: review?(cam) → review+
Thank you for the review!
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ccba28267ff
Force to call MediaFeatureValuesChanged for stylo in UpdateEffectiveTextZoom. r=heycam
https://hg.mozilla.org/mozilla-central/rev/1ccba28267ff
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.