1290786 - Merge CSS2Properties into CSSStyleDeclaration

Status: NEW

(Core :: DOM: CSS Object Model, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+10]

In the CSSOM spec, there is no such thing called CSS2Properties, and the attributes are in interface CSSStyleDeclaration. Also it seems none of Blink, WebKit, and Edge has this interface available. I think we should remove it as well.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

bz, do you think this would cause any compat issue?

Comment 2

[Boris Zbarsky [:bzbarsky]]

What's the story on FontFaceRule?  Does it still claim to return a "CSSStyleDeclaration" from .style in the CSS spec?  Because the thing it returns should NOT have all the CSS2Properties stuff on it.  This is the reason we kept the separate CSS2Properties interface, pending the working group resolving the FontFaceRule mess.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

It looks like in other engines, CSSFontFaceRule.style returns a CSSStyleDeclaration which includes all the properties. I know that doesn't make sense, but other browsers agree on this behavior, so I suppose it's safe to switch.

I think the correct solution would be adding the descriptors into CSSFontFaceRule directly like what we've done for CSSCounterStyleRule, and leave .style obsolete there with using interface CSSStyleDeclaration with all properties.

Does it sound reasonable for us to merge those two, then?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> CSSFontFaceRule.style returns a CSSStyleDeclaration which includes all the properties

With getters and setters that do what?  What does setting .style.fontFamily do?  What about .style.display?  .style.borderColor?

As of late last year, what the other UAs did was not actually interoperable with each other and not really working very well at all.  See https://lists.w3.org/Archives/Public/www-style/2015Dec/0144.html (and its whole thread) and bug 1058408.

> I know that doesn't make sense

Then why do it?  It's going to be a bunch of code bloat, and for what purpose?

Really, the right thing to do here is to kick CSSWG to actually decide something about this API and then talk to the other UAs about actually implementing the result.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Boris Zbarsky [:bz] from comment #4)
> As of late last year, what the other UAs did was not actually interoperable
> with each other and not really working very well at all.  See
> https://lists.w3.org/Archives/Public/www-style/2015Dec/0144.html (and its
> whole thread) and bug 1058408.

Thanks for the information. I'll have a look at bug 1058408.

> > I know that doesn't make sense
> 
> Then why do it?  It's going to be a bunch of code bloat, and for what
> purpose?

Because I want to create a subclass of CSSStyleDeclaration (actually CSS2Properties currently) for Stylo, but the definition of nsDOMCSSDeclaration doesn't make much sense for Stylo, because I don't think we want to implement css::Declaration there, or need CSS parsing environment for it.

Actually it already doesn't make sense for computed values. Although nsComputedDOMStyle currently inherits from nsDOMCSSDeclaration, it just overrides the methods with its own impl and use stub aborting impl for abstract methods nsDOMCSSDeclaration requires, to stop compilers from complaining.

With merging CSS2Properties into CSSStyleDeclaration, nsComputedDOMStyle (as well as the future Stylo CSS declaration class) would be able to inherit nsICSSDeclaration directly.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Actually it already doesn't make sense for computed values

All the IDL getters make sense from an API standpoint, though as you note they have a different implementation.  The setters do not, because computed style a readonly thing, I agree; it would have made some sense, if this were green-field API design, to have a writable interface inheriting from a readonly one.  Maybe.  The case of the font face thing is quite different, though, because for most property names neither the getter nor the setter makes sense from an API standpoint.  And on the other hand it wants to expose things that are not CSS property names at all (e.g. unicodeRange).

I agree that the way we implement all this in practice is a bit weird, and largely that way for historical reasons.  It would probably be cleaner to have a CSS2Properties class (name bikesheddable) that inherits from nsICSSDeclaration and has all the Get*/Set* inline methods that forward to Get/SetPropertyValue and nothing else declared on it.  Then nsDOMCSSDeclaration and nsComputedDOMStyle would both inherit from this class and implement the pure virtuals from nsICSSDeclaration, and the Stylo thing would do the same.

I'm fine with us making such a change.  I'm also fine with Stylo basically using the nsComputedDOMStyle approach for now.

What I'm not fine with is making web-facing changes in the direction of nonsense due to our internal implementation decisions, especially when its not that hard to not make such web-facing changes...

Comment 7

[Rick Byers]

Note that in an attempt to measure API overlap across browsers (part of the chromium web platform predictability project), these CSS2Properties properties are showing up as a large number (880) of Gecko-specific APIs.  I know measuring interop by counting APIs is fundamentally flawed, but it's producing some useful results for us.  And hanging all the CSS properties here could, of course, result in site compat bugs.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Sure.  I'd be happy to have UAs align on something sane here.  See comment 6.

Comment 9

[chris]

By the way, the FontFaceRule interface was updated in the CSS Fonts 4 spec some time ago:
https://drafts.csswg.org/css-fonts-4/#om-fontface

Comment 10

[Boris Zbarsky [:bzbarsky]]

Updated how? It's the same broken definition it's been all along.

Comment 11

[chris]

No, it was a very different interface proposed by John Daggett, for quite a while; but no-one was implementing it including Mozilla so the CSS WG reverted it to what people seem to be implementing.

Comment 12

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

Gecko doesn't actually implement that fully, and hasn't since the spec rolled CSS2Properties into CSSStyleDeclaration, since that requires that fontFaceRule.style.width (etc.) exist and do something. We've never implemented the CSS2Properties part for @font-face rules; see this testcase.