For , "0" should be parsed as number rather than length

[upsuper]

The plain zero can be parsed by both length and number, and there is no difference between either way in general. But for serialization, zero length would become 0px while zero number would stay 0, and the latter looks better as it doesn't changes, and you can always specify 0px to get the zero length.

This specific behavior is currently undefined in the spec (see w3c/csswg-drafts#489), but parsing plain zero as number matches all major browsers.

[upsuper]

The relevant code is https://github.com/servo/servo/blob/master/components/style/values/specified/length.rs#L1142-L1148

[highfive]

Please make a comment here if you intend to work on this issue. Thank you!

[atheed]

May I claim this?
Full disclosure: I'm fairly new to Servo and Rust.

[jdm]

@atheed Please do! Ask questions here if anything is unclear.

[atheed]

I might be a little confused, but I'm not sure if the parse_non_negative in https://github.com/servo/servo/blob/master/components/style/values/specified/length.rs#L1142-L1148 is ever being called. For what CSS properties is that function supposed to be called? I've tried various lengths and numbers for various properties (font-size, padding, etc.), but none of them activate that ^ function.

[jdm]

It turns out that https://dxr.mozilla.org/servo/source/components/style/properties/longhand/border.mako.rs#263 is the only caller, and the border-image-outset property is only supported when using this code as part of Firefox (not the normal Servo binary), as evidenced by the product="gecko". However, we can write a unit test that exercises this code in tests/unit/style/parsing (run it with ./mach test-unit -p style).

[atheed]

Ah, I see -- thanks for the heads up!

[upsuper]

Ah, actually I was thinking about line-height when I submitted this issue. It seems line-height uses a different path for parsing. The relevant code is https://github.com/servo/servo/blob/master/components/style/properties/longhand/inherited_text.mako.rs#L51

[atheed]

Ah, I see. So, this would be the relevant code for line-height https://github.com/servo/servo/blob/master/components/style/values/specified/length.rs#L897-L898, yes?
That said, it also does look like border-image-outset suffers from the same issue as line-height (i.e. the parsing of 0 as 0px instead of simply 0), so I can try to fix it for both.

[upsuper]

Oh, I checked my list, and I was actually filing this for border-image-outset, but the same issue exists in line-height. I think the code for line-height is very specific in its own parsing code I mentioned above. That code tries to parse it as length first, and would happily accept 0. That code should be changed to try number first.

line-height's syntax is normal | | | , and I guess we don't want to add a new type LengthOrPercentageOrNumber for it... So I guess it should be enough to just change the parsing code of line-height.

[atheed]

Thanks.
I had a quick follow-up question: I've fixed the issue for both line-height and border-image-outset, and while I was writing unit tests for the new "0" behaviour for the latter (in https://github.com/servo/servo/blob/master/tests/unit/style/parsing/border.rs), I noticed that there don't appear to be any parsing unit tests for the former. As such, I was wondering where I should write the parsing unit tests for line-height for the new "0" behaviour -- should I write them in a new file?

[jdm]

The existing inherited_text.rs should be fine for that.