1391341 - stylo: Font size inherits differently than in Gecko

Status: NEW

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Kevin Hsieh]

Attachment: 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.

Comment 1

[Kevin Hsieh]

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)

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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!

Comment 4

[Kevin Hsieh]

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).

Comment 5

[Daniel Holbert [:dholbert]]

(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.

Comment 6

[Jonathan Kew [:jfkthame]]

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.)

Comment 7

[Kevin Hsieh]

Note: Reftests in Bug 1367860 have been changed to avoid this bug.

Comment 8

[Cameron McCormack (:heycam)]

Is this bug 1375332, which Jeremy is in the middle of investigating?

Comment 9

[Jonathan Kew [:jfkthame]]

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.

Comment 10

[Jeremy Chen ([e-mail])]

(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. :(

Comment 11

[Jeremy Chen ([e-mail])]

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. :/

Comment 12

[Jonathan Kew [:jfkthame]]

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.)

Comment 13

[Jeremy Chen ([e-mail])]

(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. :)

Comment 14

[Manish Goregaokar [:manishearth]]

I suspect this to be a mismatch in fixup_none_generic or fixup_min_font_size, or perhaps in the system font computation.

Comment 15

[Manish Goregaokar [:manishearth]]

er, sorry, the base size stuff, not fixup_none_generic

Comment 16

[Jeremy Chen ([e-mail])]

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

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 18

[Manish Goregaokar [:manishearth]]

> 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.

Comment 19

[Manish Goregaokar [:manishearth]]

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).

Comment 20

[Manish Goregaokar [:manishearth]]

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.

Comment 21

[Manish Goregaokar [:manishearth]]

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.

Comment 22

[Manish Goregaokar [:manishearth]]

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.

Comment 23

[Jonathan Kew [:jfkthame]]

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

Comment 24

[Manish Goregaokar [:manishearth]]

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.

Comment 25

[Kevin Hsieh]

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?

Comment 26

[Manish Goregaokar [:manishearth]]

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.

Comment 27

[Jeremy Chen ([e-mail])]

(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 28

[Jeremy Chen ([e-mail])]

Attachment: Bug 1391341 - (wip) stylo: inherit the computed value instead of the specified keyword for font-size property.

According to the spec [1], we should just inherit the computed value from the parent
if the font-size property is unset or inherit. The current implementation in
Stylo keeps tracking the specified keyword during the inheritance, and recomputes
font-size in case of language changes. Althought this seems has a better interaction
with language specific user prefs, this would conflict with the spec as currently written.

To avoid the spec conflict and to match Gecko's behavior, let the font-size inheritance
blindly inherit the computed value from the parent element.

[1] https://www.w3.org/TR/css-fonts-3/#font-size-prop

Review commit: https://reviewboard.mozilla.org/r/173082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173082/

Comment 29

[Jeremy Chen ([e-mail])]

(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.

Comment 30

[Manish Goregaokar [: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)

Comment 31

[Manish Goregaokar [:manishearth]]

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.

Comment 32

[Manish Goregaokar [:manishearth]]

(The fix here seems to be a pure-gecko fix, so this might not be that high priority)

Comment 33

[Jeremy Chen ([e-mail])]

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.

Comment 34

[Bobby Holley (:bholley)]

Ok. Sounds like there's nothing urgent that needs to be done here.

Comment 35

[Kevin Hsieh]

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.)

Comment 36

[Jeremy Chen ([e-mail])]

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.

Comment 37

[Chris Mills [:cmills]]

Documented:

https://developer.mozilla.org/en-US/docs/Web/CSS/font-size#Quantum_CSS_notes
https://developer.mozilla.org/en-US/Firefox/Releases/57#Quantum_CSS_notes

Let me know if this description is OK. Thanks!

Comment 38

[Kevin Hsieh]

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)..."?

Comment 39

[Manish Goregaokar [:manishearth]]

That sounds good.

Comment 41

[Chris Peterson [:cpeterson]]

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?

Comment 42

[Jonathan Kew [:jfkthame]]

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.

Comment 43

[Kevin Hsieh]

Attachment: testcase.htm

Updated testcase based on Bug 1407690. Gecko behaves similarly to Chrome and Stylo behaves similarly to MSIE.

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

So, what should we do here?  What behavior is right?  What behavior should we do?  Match Chrome, MSIE?  What do edge/safari do?

Comment 45

[Kevin Hsieh]

It seems Chrome/Safari both behave like Gecko and MSIE/Edge both behave like Stylo.