379809 - [FIX]Uncomment the "9px fantasy" value of "font" in property_database.js

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I had to comment it out because it was failing line-height roundtripping on Mac:

*** 22164 ERROR FAIL | serialize(line-height)+parse should be idempotent for 'font: 9px fantasy' | got "10.8px", expected "13.8833px"

(from test_value_storage.html).

We should figure out what's going on here and uncomment it.

Comment 1

[Robert Sayre]

I can reproduce this failure, for the record.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> dbaron: this is failing in the part of the test where we set the subprops one at a time
> dbaron: and after we set it, check its computed value
> dbaron: but the computed line-height depends on the specified values of various other parts of the font shorthand
> dbaron: in particular on the font-family
> dbaron: which we set _after_ we check the value
> dbaron: I don't see it, because "fantasy" and "serif" are the same font on my devel machine
> dbaron: but on the mac, they're not
> dbaron: and have a 3px difference in line-height at 9px font-size
<dbaron> it shouldn't if it's not normal...
> well, "normal" is the one that's failing
> "font: 9px fantasy"

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Summary of changes:

1)  Split apart the setting of subproperties and their computation in the two
    places where this was relevant.
2)  Added "5px green none" to the various border-* shorthand tests (I had
    switched the subprop order for border-right to test that this worked
    correctly too, and figured I'd leave it in).
3)  Add the "thin" and "green" values that border-bottom already had to the
    other three sides.

Thoughts?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 263974 [details] [diff] [review]
Proposed fix

I can understand the need to do this when setting subproperties to step1vals, but not when setting them to step1comps.  So could you leave the second loop that you're changing the way it was -- or maybe even do the removeProperty inside the loop?

r=dbaron with that

Comment 5

[Boris Zbarsky [:bzbarsky]]

Checked in, with that change.  I moved removeProperty to the beginning of that loop.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I had to add the exception for "thin" to the other border sides in test_value_computation.html.

And moving removeProperty made us fail the test for parse+compute+serialize(border-bottom-width) and so forth, because while the computed border-bottom-width is a length, if that's the _only_ value set (that is, the border-style is its initial "none"), the computed border-bottom-width is 0.  Same for outline-width, and some other properties.

So I do think that my initial patch for that part of the file was correct, for what it's worth.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Alternatively, we could use the dependency list instead of the other computed values.

Comment 8

[Boris Zbarsky [:bzbarsky]]

dependency list?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

the prerequisites entry in the property table.

But I think it's ok to just land what you had originally; that should be fine.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> the prerequisites entry in the property table.

Ah, ok.  Didn't realize that existed (not all props have it, and it's not documented at the top).  I agree that we could use it here, with some work... but I think it's easier to do the other thing; I've landed it.