Implement unitless length quirk

[upsuper]

Spec: https://quirks.spec.whatwg.org/#the-unitless-length-quirk

In quirks mode, certain length properties should accept unitless number as length.

(Part of #11709)

[absoludity]

I just took a look at this, but currently the parse() function for (all?) properties does not have any reference to SharedStyleContext.QuirksMode. I'm not sure if the ParserContex should be updated to include that? (Or ParserContextExtraData - though that looks like it's only used for feature=gecko, but parsing with quirksmode would be needed with or without that, I assume).

Let me know if there's a direction I can head there, or whether I should look for something else :) Thanks @upsuper

[upsuper]

I think we need to add a field about quirks mode to ParserContext, so that parsing functions have access to that. I think that's the right direction. I don't have clear idea about what is the best way to write the parsing code, though.

I don't think it would be an easy one, but I guess it would probably be fine for you :) Thanks for taking look at this.

[absoludity]

Hi @upsuper . I took a look at this last night and put together a small proof-of-concept for background-position-x:

absoludity@5c3beb6

As per the comment there, what I'm not yet sure about is who or what will trigger the ParserContext::QuirksMode to change, and when that happens. I'll take a closer look over the weekend, but let me know if you've any pointers about that.

Thanks.

[upsuper]

I think all constructors of ParserContext should have a new parameter quirks_mode, and that parameter should be recursively propagated into all the callers, until at some level the function is responsible to provide the quirks mode (which is a per-document thing).

For example:

• ParserContext::new_with_extra_data is called by
• style::properties::parse_style_attribute, which is called by
• <script::dom::element::Element as VirtualMethods>::attribute_mutated

and in the method of Element, you can get quirks mode from doc.quirks_mode(), so this chain ends here.

[absoludity]

@upsuper: How can that particular example propagation path (up to attribute_mutated) be tested? Adding the quirks_mode through there is easy enough, but neither the unit tests or wpt tests (unitless-length.html) seem to have any test which is affected by that. I'll look for another propagation path to something for which I can test (currently checking new_with_extra_data -> CSSRule::parse -> ...), as with limited scope of the codebase, I'm not keen to propose PRs without tests.

[upsuper]

I guess you can start with adding a branch to the parse_longhand macro to make it accept an optional quirks mode parameter, and then add some assertions to parsing tests of corresponding properties.

I think wpt test unitless-length.html depends on path via Stylesheet::update_from_str. But since it uses elem.textContent to update the style text, whether it works may depend on other features, so I wouldn't bet too much on that...

[upsuper]

For testing attribute_mutated, the best way is probably having a quirks mode document which use elem.setAttribute('style', sometestcode);. Not sure where is the best place to put it... Probably new reftests in wpt or wpt/mozilla.

@jdm do you have any suggestion?

[absoludity]

Yeah, unit tests of the properties themselves are straight forward (already in the branch for background-position-x, though that's a good idea for updating parse_longhand to accept quirks mode - I'll update to that), but I'd feel safer if I could test the integration also (where I'm pushing the quirks_mode arg up to attribute_mutated in this case). Thanks either way - I'll keep experimenting.

[jdm]

Until we can modify css-tests, integration tests belong in tests/wpt/mozilla/tests.

[bholley]

@absoludity, any updates here?

[absoludity]

@bholley No sorry - been super busy both with work and outside work lately, so I've not done anything since the two commits on my test-quirks branch https://github.com/absoludity/servo/tree/test-quirks

If you're in a position to give this attention now, great - I'm happy to join in again when I get time (hopefully about 1 week from now my evenings should be a little more free). My main aim with the initial branch was to find a way to ensure the changes result in a (new) failing test then passing. I was looking at adding tests/wpt/mozilla/tests as above, based on the wpt quirks test for unitless-length.html, which would do this.

[bholley]

I'm just going through all the issues that we need to ship Servo's CSS engine in Firefox, and this is one of the ones we're hoping to knock out in the next couple of weeks. If you can get back to this in a week or so that sounds awesome! Really appreciate your help. :-)

[absoludity]

@bholley, @upsuper Sorry - I've not had spare time to do servo work and due to changes in circumstances I'll need to focus on other things for the next months in any spare time that I have. Hopefully someone else can pick this up.

[bholley]

Ok - thanks for letting us know! We'll try to find a new owner for this.

[stshine]

@nox should this be closed now?

[nox]

Yes.