1919582 - Implement CSSStyleProperties and other recent CSSOM changes

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, task, P2)

Description

[Emilio Cobos Álvarez [:emilio]]

That is, the stuff in https://github.com/w3c/csswg-drafts/pull/9686 and co.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Implement CSSStyleProperties

Is this as simple as renaming CSS2Properties to CSSStyleProperties?

Comment 2

[Emilio Cobos Álvarez [:emilio]]

I think mostly, yueah

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1919582 - Rename CSS2Properties to CSSStyleProperties r=emilio

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Hi Andreas, we are renaming an existing interface CSS2Properties here, do we have a good reason that this would break addons? Are there many addons that specifically reference CSS2Properties?

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Similar question for devtools, although I think devtools complains if the browser version doesn't match for remote debugging? Would devtools get any problem by this renaming?

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #5)

Similar question for devtools, although I think devtools complains if the browser version doesn't match for remote debugging? Would devtools get any problem by this renaming?

we do try to maintain client backward compatibility for server on version N-2 https://firefox-source-docs.mozilla.org/devtools/backend/backward-compatibility.html
But looking at your patch, the change in DevTools is on the server, so we shouldn't have issue with backward compatibility here

Comment 7

[Andreas Wagner [:TheOne] [use NI]]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #4)

Hi Andreas, we are renaming an existing interface CSS2Properties here, do we have a good reason that this would break addons? Are there many addons that specifically reference CSS2Properties?

Hi Kagami, I am looking into this and will get back to you in the coming days.

Comment 8

[Andreas Wagner [:TheOne] [use NI]]

In total we found 279 add-ons using the string CSS2Properties, 8 of them with more than 1k users each. I forwarded the list of 8 to William Durand. He's going to have a look and will report back his findings here.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:saschanaz, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Waiting for William to respond.

Comment 11

[William Durand [:willdurand]]

Oops sorry about that. It's looking alright in the add-ons I've inspected, I think we can proceed without further changes. If we get reports in the Nightly cycle, then we might need to add an alias but probably not a must-have for now (given the data we have).

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Thank you! Let's try and see in that case.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/051df29ed179
https://hg.mozilla.org/integration/autoland/rev/d8ccd1164872
Rename CSS2Properties to CSSStyleProperties r=emilio,webcompat-reviewers,frontend-codestyle-reviewers,twisniewski,devtools-reviewers,nchevobbe

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/54098 for changes under testing/web-platform/tests

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6d971b48e2bc
https://hg.mozilla.org/integration/autoland/rev/b6256c2a366f
Revert "Bug 1919582 - Rename CSS2Properties to CSSStyleProperties r=emilio,webcompat-reviewers,frontend-codestyle-reviewers,twisniewski,devtools-reviewers,nchevobbe" for causing devtools failures in browser_dbg-preview-wrapped-lines.js.

Comment 16

[Serban Stanca [:SerbanS]]

Reverted this because it was causing devtools failures in browser_dbg-preview-wrapped-lines.js.

• Revert link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg-preview-wrapped-lines.js | The content on line 6 to wrap over 6 lines (including the inline previews) - Got 77, expected 92

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d06890365500
https://hg.mozilla.org/integration/autoland/rev/742e2447826a
Rename CSS2Properties to CSSStyleProperties r=emilio,webcompat-reviewers,frontend-codestyle-reviewers,twisniewski,devtools-reviewers,nchevobbe

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ef05f75c3f2f
https://hg.mozilla.org/integration/autoland/rev/65a215f0ac95
Revert "Bug 1919582 - Rename CSS2Properties to CSSStyleProperties r=emilio,webcompat-reviewers,frontend-codestyle-reviewers,twisniewski,devtools-reviewers,nchevobbe" for dt failure on browser_dbg-preview-wrapped-lines.js

Comment 19

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failure on browser_dbg-preview-wrapped-lines.js

Backout link: https://hg-edge.mozilla.org/integration/autoland/rev/65a215f0ac954d7030783d5ccca5cf595283bbf0
Log link: https://treeherder.mozilla.org/logviewer?job_id=520605925&repo=autoland&lineNumber=2938

Comment 20

[Kagami Rosylight [:saschanaz] (they/them)]

Wait, the test result differs between Linux and Windows? That's not expected at all from my patch 🤔

Comment 21

[Kagami Rosylight [:saschanaz] (they/them)]

Pushed try with dump(): https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=145761

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #20)

Wait, the test result differs between Linux and Windows? That's not expected at all from my patch 🤔

This might be caused by differences in font-size between platforms, making the lines to wrap earlier/later depending on the OS :/
We had a test that was giving us trouble in the past for the same reason, and what we did was to force the Debugger to use a specific font so it would have similar sizes everywhere:

https://searchfox.org/mozilla-central/rev/81221d27112f12a67cb86287bf2b3cd9f19373af/devtools/client/debugger/test/mochitest/browser_dbg-editor-horizontal-scroll.js#51-73

const font = new global.FontFace(
  "Ahem",
  "url(chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/examples/Ahem.ttf)"
);
const loadedFont = await font.load();
global.document.fonts.add(loadedFont);

is(global.devicePixelRatio, 1);
is(global.browsingContext.top.window.devicePixelRatio, 1);
global.browsingContext.top.overrideDPPX = 1;
is(global.browsingContext.fullZoom, 1);
is(global.browsingContext.textZoom, 1);

// /!\ Change the Codemirror font to use a fixed font across all OSes
// and always have the same number of characters displayed.
// Note that this devtools mono makes the "o" characters almost invisible.
editor.codeMirror.contentDOM.style.fontFamily = "Ahem";
editor.codeMirror.contentDOM.style.fontSize = "10px";
editor.codeMirror.contentDOM.style.lineHeight = "15px";
editor.codeMirror.contentDOM.style.fontWeight = "normal";
editor.codeMirror.contentDOM.style.fontStyle = "normal";
editor.codeMirror.contentDOM.style.fontStretch = "normal";
is(global.getComputedStyle(editor.codeMirror.contentDOM).fontFamily, "Ahem");

maybe this could help you here

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/82b6c106ae7a
https://hg.mozilla.org/integration/autoland/rev/f3f81262b106
Rename CSS2Properties to CSSStyleProperties r=emilio,webcompat-reviewers,frontend-codestyle-reviewers,twisniewski,devtools-reviewers,nchevobbe

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/54577 for changes under testing/web-platform/tests

Comment 26

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f3f81262b106

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by nairnandu

Comment 28

[Kagami Rosylight [:saschanaz] (they/them)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Name change of an existing web interface object
[Affects Firefox for Android]: Yes
[Suggested wording]: Gecko-specific CSS2Properties is now renamed to CSSStyleProperties, to align with the latest web standard and for better interop with other browser engines.
[Links (documentation, blog post, etc)]:

Comment 29

[Hamish Willee]

FF144 MDN work for this can be tracked in https://github.com/mdn/content/issues/41133

I can see that this changes CSS2Properties to CSSStyleProperties, but that isn't particularly useful information since neither of these are documented on MDN, and it isn't clear what CSS2Properties did before and CSSStyleProperties does now. Can you advise?

What I can see in the spec is that CSSStyleProperties is

[Exposed=Window]
interface CSSStyleProperties : CSSStyleDeclaration {
  [CEReactions] attribute [LegacyNullToEmptyString] CSSOMString cssFloat;
};
partial interface CSSStyleProperties {
  [CEReactions] attribute [LegacyNullToEmptyString] CSSOMString _camel_cased_attribute;
};
partial interface CSSStyleProperties {
  [CEReactions] attribute [LegacyNullToEmptyString] CSSOMString _webkit_cased_attribute;
};

What I take from that is that this is a CSSStyleDeclaration with the property cssFloat.

1. What is cssFloat for?

2. For every CSS property defined on the object such as box-shadow there will be a camel case property boxShadow and a webkit cased property box_shadow. Is that right - anything else I'm missing on the object?

---

The object is returned in a number of places

interface mixin ElementCSSInlineStyle {
  [SameObject, PutForwards=cssText] readonly attribute CSSStyleProperties style;
};

AND https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle

3. FF does not appear to use CSSStyleProperties for the style or the getComputedStyle so:

   • What is it used for?
   • What is the point of CSSStyleProperties - why not just use CSSStyleDeclaration?

4. In other words, I'm trying to work out what the point is of all this and document the delta against what the spec says. Any advice you can give me would be much appreciated!

Comment 30

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Hamish Willee from comment #29)

I can see that this changes CSS2Properties to CSSStyleProperties, but that isn't particularly useful information since neither of these are documented on MDN, and it isn't clear what CSS2Properties did before and CSSStyleProperties does now. Can you advise?

Probably it would be something like:

            {
              "version_added": "144"
            },
            {
              "alternative_name": "CSS2Properties",
              "version_added": "(i dunno, probably firefox 1, I only confirmed firefox 2 had it)"
            },

I see https://github.com/mdn/browser-compat-data/pull/27902 added it but without alternative_name.

1. What is cssFloat for?

It's for backward compatibility, back in the day float was a keyword and you couldn't just do document.body.style.float.

2. For every CSS property defined on the object such as box-shadow there will be a camel case property boxShadow and a webkit cased property box_shadow. Is that right - anything else I'm missing on the object?

webkit prefix one would exist only for certain properties, and the hyphen case "box-shadow" is also a valid property on the object.

The object is returned in a number of places

interface mixin ElementCSSInlineStyle {
  [SameObject, PutForwards=cssText] readonly attribute CSSStyleProperties style;
};

AND https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle

3. FF does not appear to use CSSStyleProperties for the style or the getComputedStyle so:
   • What is it used for?
   • What is the point of CSSStyleProperties - why not just use CSSStyleDeclaration?

Err, I just forgot to update them in Gecko after https://github.com/w3c/csswg-drafts/pull/12477

4. In other words, I'm trying to work out what the point is of all this and document the delta against what the spec says. Any advice you can give me would be much appreciated!

The details are in https://github.com/w3c/csswg-drafts/issues/5649, tl;dr is that we don't want to expose every existing properties in CSSPageRule.style and want to expose only relevant properties (for CSSPageRule the page related ones). And thus CSSStyleDeclaration as a superclass and then each subclass (CSSStyleProperties in general, or CSSPageDescriptors for CSSPageRule) where only the relevant properties are exposed.

Does it help? 👀

Comment 31

[Kagami Rosylight [:saschanaz] (they/them)]

a webkit cased property box_shadow

That's not a thing but webkitBoxShadow and -webkit-box-shadow are valid properties on the object.

Comment 32

[Hamish Willee]

Thanks! That makes sense and is very helpful.

I have created the interface document in https://github.com/mdn/content/pull/41254 - do you think you could sanity check it? Note that I haven't yet done the work in HTMLElement.style and other docs yet to refer to this new interface. That comes next - along with further updates in the compatibility docs.

Comment 33

[Donal Meehan [:dmeehan]]

Added to the Fx144 release notes