stylo: Font size inherits differently than in Gecko

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
P2
normal
8 months ago
a month ago

People

(Reporter: Kevin Hsieh, Unassigned, NeedInfo)

Tracking

(Blocks: 3 bugs, {dev-doc-complete, regression})

unspecified
dev-doc-complete, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fix-optional)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 months ago
Created attachment 8898397 [details]
testcase.htm

When stylo pref is on, lang="zh" results in a smaller font-size than other languages. As a result, reftests in Bug 1367860 fail Stylo vs. Gecko tests.

Steps to reproduce:

Open the attachment in Nightly with stylo pref on.

Actual results:

The two lines of text are in the same font but have different size.

Expected results:

The two lines of text are in the same font and have the same size. This is the case in Gecko, Blink, and WebKit.
(Reporter)

Comment 1

8 months ago
User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Nightly version: 57.0a1 (2017-08-17) (64-bit)
There's some platform (system-font?) dependence here, I believe -- the testcase gives "expected results" (text all the same size) for me, on Ubuntu 17.04 with Firefox Nightly 57.0a1 (2017-08-17) (with layout.css.servo.enabled=true).

Also: given that this is stylo-pref-dependent, something is likely going wrong in the style-system itself, before we get to actually doing text layout. --> Reclassifying as CSS
Component: Layout: Text → CSS Parsing and Computation
Yeah, same for me on Linux.

Manish, you use OSX and seems like the kind of stuff you're familiar with, could you take a look? Thanks!
Flags: needinfo?(manishearth)
(Reporter)

Comment 4

8 months ago
Based on my try run from Bug 1367860, the issue occurs on OS X but not Linux. It's hopefully not a system font issue (font-family is set explicitly in the testcase).
(In reply to Kevin Hsieh from comment #4)
> It's hopefully not a system font issue (font-family is set explicitly in the testcase).

Sorry -- when I referred to "system-font", I meant that this bug's testability probably depends on what specific fonts you have installed on your system (and which font is chosen to match the testcase's requested font-family).  For example -- on my system, the devtools font pane says the text renders with "NimbusSanL-Regu", which is apparently the closest thing I have to Helvetica.
I'm pretty certain the smaller font size here is coming from langGroup-dependent font prefs, see https://dxr.mozilla.org/mozilla-central/rev/04bee69b3274bd8d5cf52d54a0a5cc14dbe8693a/modules/libpref/init/all.js#4095-4097.

Not sure why the stylo vs gecko behavior difference, though, or indeed which behavior is the "expected" one. (The behavior described in bug 1367860 may also be playing a role of some kind here.)
(Reporter)

Comment 7

8 months ago
Note: Reftests in Bug 1367860 have been changed to avoid this bug.
Is this bug 1375332, which Jeremy is in the middle of investigating?
Maybe. From a brief look, it doesn't sound identical -- this one is about the lang-specific default (initial) value of font-size, and how it inherits across a lang change, while that is about the minimum (which may clamp a css-specified size). They feel similar enough that they might turn out to have a shared underlying cause, but I'm not sure at this point.
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> Maybe. From a brief look, it doesn't sound identical -- this one is about
> the lang-specific default (initial) value of font-size, and how it inherits
> across a lang change, while that is about the minimum (which may clamp a
> css-specified size). They feel similar enough that they might turn out to
> have a shared underlying cause, but I'm not sure at this point.

I just did a quick verification, and it turns out that my patches for bug 1375332 doesn't solve the issue here. :(
I can take a look since I'm around (due to bug 1375332) and I use OSX as well. The bad part might be that this issue can't be reproduced on Linux, so I can't use RR to debug it. :/
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
I suspect it's only Mac-specific because of the platform-specific default font size prefs:
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/modules/libpref/init/all.js#4157

If you set similar prefs on Linux, you may find that the same behavior occurs there. (I haven't tested this, though.)
(In reply to Jonathan Kew (:jfkthame) from comment #12)
> I suspect it's only Mac-specific because of the platform-specific default
> font size prefs:
> http://searchfox.org/mozilla-central/rev/
> 48ea452803907f2575d81021e8678634e8067fc2/modules/libpref/init/all.js#4157
> 
> If you set similar prefs on Linux, you may find that the same behavior
> occurs there. (I haven't tested this, though.)

You're right!! Just set the prefs to be agreed with those on Mac, then it is reproducible on Linux as well. Thank you. :)
I suspect this to be a mismatch in fixup_none_generic or fixup_min_font_size, or perhaps in the system font computation.
Flags: needinfo?(manishearth)
er, sorry, the base size stuff, not fixup_none_generic
Hmmm, font-size is an inherited property, so there might be something wrong in Stylo's cascade_inherit_font_size().

The testcase should be rendered like:

```
<!DOCTYPE html>
<html>
<body style="font-family: Helvetica">
  <div lang="en">
    These two lines should be exactly the same.
  </div>
  <div lang="zh" style="font-size: inherit;">
    These two lines should be exactly the same.
  </div>
</body>
</html>
```

But in Stylo, it is rendered like:

```
<!DOCTYPE html>
<html>
<body style="font-family: Helvetica">
  <div lang="en">
    These two lines should be exactly the same.
  </div>
  <div lang="zh" style="font-size: initial;">
    These two lines should be exactly the same.
  </div>
</body>
</html>
```


[1] https://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/servo/components/style/properties/longhand/font.mako.rs#1022
Note also this piece of code:

  https://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/servo/components/style/properties/properties.mako.rs#3383

Sincerely, the font-size stuff is over the roof in complexity. I hope this fix can decrease it instead of increase it.
> These two lines should be exactly the same.

I'm not sure if that's true, in case of an explicitly inherited keyword size we still have to recalculate the size.
Huh, interesting. This is not the case in Gecko.

Which ... doesn't make sense, given how Gecko works. Gecko's algorithm is to restyle keyword font sizes as if the lang and generic have always been the current lang and generic. In which case font-size: inherit will copy over the base size for zh, not the base size for the default lang.


I really want to get rid of the complexity here too, but we can't do that without getting rid of Gecko's complexity, which is IMO worse and also holistic (so we can't just rip out bits of it we don't like, all of it has to go at once).
So the fix here would be to make cascade_inherit_font_size different from the currently nonexistant cascade_unset_font_size. unset will do what inherit does now, and inherit will blind copy. Pass down unset in https://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/servo/components/style/properties/properties.mako.rs#3390-3391.

This should make us match Gecko. I'm unsure *why* gecko is behaving the way it does, however, and I do feel that servo's behavior is more correct here.
dbaron says that font-size:inherit should always do the same thing as not specifying it at all. However, I checked again, and this *does* work in gecko


<!DOCTYPE html>
<html>
<body style="font-family: Helvetica">
  <div lang="en">
    These two lines should be exactly the same.
  </div>
  <div lang="zh" style="font-size: inherit;">
    These two lines should be exactly the same.
  </div>
  <div lang="zh">
    These two lines should be exactly the same.
  </div>
</body>
</html>


The problem is that lang=zh is not resetting the base size (since this entire document is on implied font-size:medium) in Gecko, whereas servo does. Servo seems to have the correct behavior here.
Yeah, it seems like Gecko is incorrect here. Gecko should be recalculating base sizes whenever the generic or the lang changes, because if you have tweaked font size prefs (I do), lang=zh should use a new font size.
If I'm understanding CSS Fonts[1] properly, I think gecko's (rather than servo's) behavior is correct here.

I'm basing this on two things from the spec:

  (a) "Computed value: 	absolute length"

which means the computed value of font-size on the <body> in comment 21, for example, will be an absolute length (16px, assuming default prefs) and not the initial value 'medium'. When this is combined with the statement that

  (b) "Child elements inherit the computed ‘font-size’ value" (see the end of §3.5)

I conclude that the <div> children must inherit the value 16px, rather than inheriting 'medium' and re-evaluating it in the light of the prefs for the changed lang.

(Offhand, I think I might actually _prefer_ servo's behavior, but it looks like that would conflict with the spec as currently written.)

[1] https://www.w3.org/TR/css-fonts-3/#font-size-prop
This behavior is completely unspecced but is kinda necessary for things to work.

I wrote about some of this at https://manishearth.github.io/blog/2017/08/10/font-size-an-unexpectedly-complex-css-property/


Basically, "font-size:medium" maps to different things depending on the generic font family and the language. This dependence *inherits* -- the default font-size on a document is medium, so you need this behavior to get used everywhere.

It's especially important for one to be able to set the default font size for a given language in the prefs (something both chrome and firefox let you do, and is a pretty useful feature).

Gecko solves this by using SetGenericFont; a function which recomputes font-size as if the lang/generic had always been the current lang/generic. Servo solves this by doing some extra bookkeeping when font-size is derived from a keyword.

Servo's behavior here is copying Gecko's, we just managed to somehow be more correct at it.
(Reporter)

Comment 25

8 months ago
This may be the font-size analogue to the font-family inheritance issue in Bug 1367860. In that bug, both Gecko and Stylo add the lang-specific default generic font-family to the computed font list immediately when it should be managed separately (so that it can be overwritten in the case of a lang change, while still inheriting any specified font-family). This renders the lang-specific font-family pref useless unless the author forces re-computation by specifying any font-family--even one containing only invalid fonts--when or after specifying lang.

In this case, if we compute an absolute length so early, then there seems to be less of a point to having language-specific font-size prefs. Although, why is the default font-size for Chinese set to be smaller than the default font-size for Latin?
Kind of. But in this case this behavior is intentional; font-size is weird.

No idea why it's smaller, I've actually set mine to be larger because Chinese glyphs are hard to read for me.
(In reply to Jonathan Kew (:jfkthame) from comment #23)
> If I'm understanding CSS Fonts[1] properly, I think gecko's (rather than
> servo's) behavior is correct here.
> 
> I'm basing this on two things from the spec:
> 
>   (a) "Computed value: 	absolute length"
> 
> which means the computed value of font-size on the <body> in comment 21, for
> example, will be an absolute length (16px, assuming default prefs) and not
> the initial value 'medium'. When this is combined with the statement that
> 
>   (b) "Child elements inherit the computed ‘font-size’ value" (see the end
> of §3.5)
> 
> I conclude that the <div> children must inherit the value 16px, rather than
> inheriting 'medium' and re-evaluating it in the light of the prefs for the
> changed lang.
> 
> (Offhand, I think I might actually _prefer_ servo's behavior, but it looks
> like that would conflict with the spec as currently written.)
> 
> [1] https://www.w3.org/TR/css-fonts-3/#font-size-prop

Hmmm, after Manish's explanation, I guess I prefer servo's behavior as well. Probably we should explicitly specify the interaction with the font-size and the language prefs in the spec?
Comment hidden (mozreview-request)
(In reply to Manish Goregaokar [:manishearth] from comment #20)
> So the fix here would be to make cascade_inherit_font_size different from
> the currently nonexistant cascade_unset_font_size. unset will do what
> inherit does now, and inherit will blind copy. Pass down unset in
> https://searchfox.org/mozilla-central/rev/
> 5696c3e525fc8222674eed6a562f5fcbe804c4c7/servo/components/style/properties/
> properties.mako.rs#3390-3391.
> 
> This should make us match Gecko. I'm unsure *why* gecko is behaving the way
> it does, however, and I do feel that servo's behavior is more correct here.

The current WIP doesn't seen right, there're a bunch of failures on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6557c23592862769556e5771c4fdd4c920f94f9c&selectedJob=126335307.

Hi Manish, I wondern what's the approach you mentioned above. Are you saying that we should cascade inherit and unset differently in Stylo? Could you enlighten me a bit more? I'm canceling the review due to the failures, and changing the request to NI.
Flags: needinfo?(manishearth)
Yes, I intend to work on adding it to the spec at some point.

(That blog post documents everything, feel free to take a shot at specifying it if you want to)
The approach I mentioned above was for making Stylo match Gecko. Not something I want to do, but it's something we *can* do to make the tests pass.

We should figure out why Gecko isn't retriggering SetGenericFont on lang changes and fix that.
Flags: needinfo?(manishearth)
(The fix here seems to be a pure-gecko fix, so this might not be that high priority)
FWIW, Chrome renders the testcase in comment 0 (and comment 21) the same as Firefox does, all texts are in the same size, whereas a smaller size for lang="zh" texts in Stylo.

Since we're not fixing this shortly (according to comment 32), mark this a behavior change for Stylo then.
Blocks: 1365771
Ok. Sounds like there's nothing urgent that needs to be done here.
Priority: -- → P4
(Reporter)

Comment 35

8 months ago
We can consider adjusting the default font size pref for zh to match other languages--I don't see why it should be smaller specifically on Mac, esp. when other browsers don't do this. (This would also "resolve" the issue from the testcase's perspective.)
(Reporter)

Updated

8 months ago
Blocks: 1396411
(Reporter)

Updated

8 months ago
Summary: stylo: Decreased font size when lang="zh" → stylo: Font size inherits differently than in Gecko
(Reporter)

Updated

8 months ago
See Also: → bug 1396731
Attachment #8901654 - Attachment description: Bug 1391341 - stylo: inherit the computed value instead of the specified keyword for font-size property. → Bug 1391341 - (wip) stylo: inherit the computed value instead of the specified keyword for font-size property.
Attachment #8901654 - Attachment is obsolete: true
Since bug 1396731 is about landing, it would be great to mention here that the testcase in comment 0 is reproducible only if you reset "font.size.variable.zh" pref to some number less than the current default value, 16. Here's an example:

```
pref("font.size.variable.zh-CN", 15);
pref("font.size.variable.zh-HK", 15);
pref("font.size.variable.zh-TW", 15);
```


Bug 1396731 is not resolving this issue, so we should get back to this someday. 

Un-assign myself due to its priority, and obsolete the current wip.

For anyone who'd like to take it over, comment 23 and comment 24 are helpful and worth reading.
Assignee: jeremychen → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

8 months ago
No longer blocks: 1396411
(Reporter)

Comment 38

7 months ago
I'm not sure if "inherited font sizes end up being smaller" is quite accurate. Perhaps something like "...for some language settings, inherited font-sizes unexpectedly override corresponding defaults from the browser preferences (see bug 1391341)..."?
Flags: needinfo?(manishearth)
That sounds good.
Flags: needinfo?(manishearth)

Updated

7 months ago
status-firefox57: --- → wontfix
status-firefox58: --- → fix-optional
Duplicate of this bug: 1407690
Blocks: 1375906
See Also: → bug 1412743
Jonathan or Jeremy: this difference between Gecko's and Stylo's font-size inheritance has been relnoted (comment 37). Can we close this bug? Or are there code changes we want to land for Stylo?
Flags: needinfo?(jfkthame)
Flags: needinfo?(jeremychen)
I think the fact that there's a difference is still an issue. Either we should fix Stylo to give the same result as Gecko, or we should clarify that Gecko's behavior is a bug and Stylo is correct. In that case, this could be regarded as a Gecko bug rather than a Stylo one (and we might decide to WONTFIX it, given that Stylo is the future), but this is something that needs a conscious decision to be made (and possibly a spec issue raised, per comment 23).

In any case, I don't think this is resolved; we just swept it under the carpet for now by tweaking the tests that it affected.
Flags: needinfo?(jfkthame)
Flags: needinfo?(jeremychen)
status-firefox59: --- → affected
Keywords: regression
Priority: P4 → P3
status-firefox58: fix-optional → affected
Priority: P3 → P2
(Reporter)

Comment 43

5 months ago
Created attachment 8931515 [details]
testcase.htm

Updated testcase based on Bug 1407690. Gecko behaves similarly to Chrome and Stylo behaves similarly to MSIE.
Attachment #8898397 - Attachment is obsolete: true
So, what should we do here?  What behavior is right?  What behavior should we do?  Match Chrome, MSIE?  What do edge/safari do?
Flags: needinfo?(jfkthame)
(Reporter)

Comment 45

2 months ago
It seems Chrome/Safari both behave like Gecko and MSIE/Edge both behave like Stylo.
status-firefox58: affected → wontfix
status-firefox59: affected → wontfix
status-firefox60: --- → fix-optional
You need to log in before you can comment on or make changes to this bug.