[cssom-1] Ensure inline style and corresponding declaration block updates each other properly #2269

upsuper · 2018-02-03T09:35:39Z

This mostly follows how DOMTokenList works in the DOM spec, which has a similar situation that having a live object reflecting an attribute.

This should fix #1559.

upsuper · 2018-02-03T09:35:49Z

cc @bzbarsky

upsuper · 2018-02-03T11:48:23Z

Note that this cannot bikeshed correctly at this moment, because "attribute change steps" wasn't actually exported from the DOM spec until whatwg/dom#567 which was merged several hours ago.

upsuper · 2018-02-04T00:29:48Z

WebKit and Blink don't agree with the part of setProperty. They don't update the attribute when the setting doesn't change value. It seems tricky to me in the sense that I don't think we have a formal definition anywhere about how to compare two values of a property, do we?

Also, if we follow the resolution in #1898 to make setProperty append new value rather than mutate the value in-place, the behavior of WebKit and Blink may become more trickier to be right. Given these, I tend to keep the that part as-is.

upsuper · 2018-02-04T00:54:21Z

Also wpt test for this change: web-platform-tests/wpt#9376.

zcorpan · 2018-02-05T09:55:14Z

cssom-1/Overview.bs

+
+When a <a>CSS declaration block</a> object is created, then:
+<ol>
+ <li>Let <var>owner node</var> be the <a for="CSSStyleDeclaration">owner node</a>.


Need to also check if the readonly flag is set. Now this will try to get an attribute when doing getComputedStyle(elm) I think.

Oh, you're right. The algorithm for attribute change steps also need updates.

Updated to make this algorithm and attribute change steps check readonly flag.

zcorpan · 2018-02-05T10:08:56Z

They don't update the attribute when the setting doesn't change value. It seems tricky to me in the sense that I don't think we have a formal definition anywhere about how to compare two values of a property, do we?

We don't I think. But maybe figuring this out can be part of #1898 ?

upsuper · 2018-02-05T10:11:19Z

We don't I think. But maybe figuring this out can be part of #1898 ?

Yeah, sure, I guess setProperty part definitely needs further tweaking for that issue.

bzbarsky · 2018-02-05T16:01:47Z

cssom-1/Overview.bs

@@ -2034,6 +2034,25 @@ Note: The serialization of an empty CSS declaration block is the empty string.
 Note: The serialization of a non-empty CSS declaration block does not include any surrounding whitespace, i.e., no whitespace appears
 before the first property name and no whitespace appears after the final semicolon delimiter that follows the last property value.

+A <a>CSS declaration block</a> has these <a>attribute change steps</a> for its <a for="CSSStyleDeclaration">owner node</a>:
+<ol>
+ <li>If the <a for="CSSStyleDeclaration">readonly flag</a> is set, terminate these steps.


How can this even happen? The cases when there is an owner node are currently not readonly and there are no plans I am aware of to make them readonly...

There should probably be a note here explaining what this test is about, because it's pretty confusing.

getComputedStyle sets owner node per the spec. But maybe that is a spec bug?

https://drafts.csswg.org/cssom/#dom-window-getcomputedstyle

Oh, I missed that somehow. OK, then this is probably fine. That said, this is the first actual use of owner node that I can see. So if it simplifies things, we could probably avoid setting owner node in getComputedStyle instead of dealing with it here....

Yeah I think we can set it to null there.

We can discuss this in a separate PR. I don't think that needs to block this change.

Actually, I have a feeling that having the declaration block from getComputedStyle have the owner node set makes sense. Not sure how useful it is, though.

bzbarsky · 2018-02-05T16:06:24Z

cssom-1/Overview.bs

+ <li>If <var>owner node</var> is null, or the <a for="CSSStyleDeclaration">readonly flag</a> is set, terminate these steps.
+ <li>Let <var>value</var> be the result of <a lt="get an attribute by namespace and local name">getting an attribute</a>
+ given null, "<code>style</code>", and <var>owner node</var>.
+ <li>If <var>value</var> is not null, let the <a for="CSSStyleDeclaration">declarations</a> be the result of


s/let the/let/ ?

It seems to me that there is a convention that we use "the" before associated properties, and no "the" before variables, so I add "the" here. Not sure if my understanding is correct.

Something like that, yeah, though probably not entirely consistent. But we should probably adopt Infra Standard conventions when we update things or write new material for CSSOM (like saying "then return" instead of "terminate these steps").

bzbarsky · 2018-02-05T16:07:39Z

cssom-1/Overview.bs

@@ -2079,6 +2098,8 @@ Setting the {{CSSStyleDeclaration/cssText}} attribute must run these steps:
 <li>Empty the <a for="CSSStyleDeclaration">declarations</a>.
 <li><a lt="Parse a CSS declaration block">Parse</a> the given value and, if the return value is not the empty list, insert the items in the list
 into the <a for="CSSStyleDeclaration">declarations</a>, in <a>specified order</a>.
+ <li>If the <a for="CSSStyleDeclaration">owner node</a> is not null, <a>set an attribute value</a> for <a for="CSSStyleDeclaration">owner node</a>


I really think we should prevent this attribute set from leading to a re-parse. It's a much simpler model than having it re-parse, because in the presence of CSP parsing from the string does NOT give the same results as the thing that was serialized to the string, in general. And since UAs are going to optimize out the reparsing for basic sanity reasons, this is likely to lead to divergence between what the spec theoretically says (but no one would ever do) and what UAs do.

There are also issues around base URIs: set vs serialize-and-reparse treat them differently. I did explicitly mention this in #1559 as a problem with the serialize-and-reparse approach.

Not reviewing the rest of the changes for now given that, because they will need to be modified anyway.

Do you have any suggestion for how can we avoid serialize-and-reparse? It seems to me if we want to avoid that, we would need to duplicate many definitions from the DOM spec just for getting rid of the "run attribute change steps" part.

Is there any existing algorithm in the DOM spec helps doing that? If not, I suggest we merge this change to get the spec closer to what browsers do currently, and file a new issue to address the serialize-and-reparse issue, which may need some help from the DOM spec side.

You could set a flag while you are setting the attribute, and not do any attribute change steps while that flag is set, right?

Hmmm, okay, fair. Thanks for the suggestion.

…ates each other properly. This mostly follows how `DOMTokenList` works in the DOM spec, which has a similar situation that having a live object reflecting an attribute. This should fix #1559.

…ze-and-reparse on style attribute.

upsuper · 2018-02-11T10:38:05Z

@zcorpan @bzbarsky ping. Are you two fine with the updated version? (The second commit is the change on top of what you have reviewed.)

zcorpan

I glossed over the serialize-and-reparse issue in my first review, sorry about that. I think the fix looks good. I added a test for it in web-platform-tests/wpt@dc41ada

upsuper · 2018-02-11T22:38:29Z

Thanks for the review and the new test :)

zcorpan · 2018-02-12T08:53:00Z

Browser bugs
Safari https://bugs.webkit.org/show_bug.cgi?id=182687
Edge https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/15908562/
Firefox - @upsuper ?
Chrome - passes the tests

upsuper · 2018-02-12T08:55:52Z

Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1197705

I removed the test of setting identical value triggering mutation record. That test would fail Chrome as well, but anyway... We can add that later after we address #1898.

bzbarsky · 2018-02-21T02:59:51Z

cssom-1/Overview.bs

+ from a string <var>value</var>.
+</ol>
+
+When a <a>CSS declaration block</a> object is created, then:


@upsuper Why is this timing correct? Especially since nothing specifies when these objects are actually created, last I checked...

As far as it is returned from Element.style, does it matter when the objects are actually created? Browser needs to create it at some point, but before script gets it somehow, whether it exists doesn't really matter, does it?

It matters because the base URI is different at different points in time, depending on what the node document of the element is. So depending on exactly when the creation/parsing happen you will get different behavior for relative URIs in property values....

Maybe that doesn't affect what CSSOM sees, just the actual styling behavior.

Hmm... okay. Sounds like something worth raising for css-style-attr I guess... The interaction between the two specs would be fun...

bzbarsky · 2018-02-21T03:00:19Z

cssom-1/Overview.bs

+ <li>Let <var>owner node</var> be <var>declaration block</var>'s <a for="CSSStyleDeclaration">owner node</a>.
+ <li>If <var>owner node</var> is null, terminate these steps.
+ <li>Set <var>declaration block</var>'s <a for="CSSStyleDeclaration">updating flag</a>.
+ <li><a>Set an attribute value</a> for <var>owner node</var> using "<code>style</code>" and the result of


This doesn't handle mutation events right, but I guess neither does anything else....

upsuper assigned zcorpan Feb 3, 2018

upsuper mentioned this pull request Feb 4, 2018

[cssom] Add test for synchronization between css declaration block and style attribute web-platform-tests/wpt#9376

Merged

upsuper mentioned this pull request Feb 4, 2018

[cssom-1] Record mutation for changes to CSSStyleDeclaration. #2268

Closed

zcorpan reviewed Feb 5, 2018

View reviewed changes

zcorpan approved these changes Feb 5, 2018

View reviewed changes

bzbarsky requested changes Feb 5, 2018

View reviewed changes

upsuper added 2 commits February 6, 2018 23:19

[cssom-1] Ensure inline style and corresponding declaration block upd…

71f88df

…ates each other properly. This mostly follows how `DOMTokenList` works in the DOM spec, which has a similar situation that having a live object reflecting an attribute. This should fix #1559.

[cssom-1] Add updating flag to CSS declaration block to avoid seriali…

03cef6e

…ze-and-reparse on style attribute.

zcorpan mentioned this pull request Feb 11, 2018

[css-logical-1] [css-cascade-3] The all shorthand probably shouldn't set logical properties. #1898

Closed

zcorpan approved these changes Feb 11, 2018

View reviewed changes

upsuper merged commit 79619b8 into w3c:master Feb 11, 2018

upsuper deleted the cssom-style-attr branch February 11, 2018 22:38

upsuper mentioned this pull request Feb 11, 2018

[cssom-1] Remove or replace "terminate ..." #2299

Merged

bzbarsky changed the title ~~[cssom-1] Ensure inline style and corresponding declaration block ates each other properly~~ [cssom-1] Ensure inline style and corresponding declaration block updates each other properly Feb 12, 2018

bzbarsky reviewed Feb 21, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cssom-1] Ensure inline style and corresponding declaration block updates each other properly #2269

[cssom-1] Ensure inline style and corresponding declaration block updates each other properly #2269

upsuper commented Feb 3, 2018

upsuper commented Feb 3, 2018

upsuper commented Feb 3, 2018

upsuper commented Feb 4, 2018

upsuper commented Feb 4, 2018

zcorpan Feb 5, 2018

upsuper Feb 5, 2018

upsuper Feb 5, 2018

zcorpan commented Feb 5, 2018

upsuper commented Feb 5, 2018

bzbarsky Feb 5, 2018

zcorpan Feb 5, 2018

bzbarsky Feb 5, 2018

zcorpan Feb 5, 2018

upsuper Feb 5, 2018

bzbarsky Feb 5, 2018

upsuper Feb 5, 2018

zcorpan Feb 11, 2018

bzbarsky Feb 5, 2018

upsuper Feb 5, 2018

bzbarsky Feb 6, 2018

upsuper Feb 6, 2018

upsuper commented Feb 11, 2018

zcorpan left a comment

upsuper commented Feb 11, 2018

zcorpan commented Feb 12, 2018

upsuper commented Feb 12, 2018

bzbarsky Feb 21, 2018

upsuper Feb 21, 2018

bzbarsky Feb 21, 2018

upsuper Feb 21, 2018

bzbarsky Feb 21, 2018

[cssom-1] Ensure inline style and corresponding declaration block updates each other properly #2269

[cssom-1] Ensure inline style and corresponding declaration block updates each other properly #2269

Conversation

upsuper commented Feb 3, 2018

upsuper commented Feb 3, 2018

upsuper commented Feb 3, 2018

upsuper commented Feb 4, 2018

upsuper commented Feb 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcorpan commented Feb 5, 2018

upsuper commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upsuper commented Feb 11, 2018

zcorpan left a comment

Choose a reason for hiding this comment

upsuper commented Feb 11, 2018

zcorpan commented Feb 12, 2018

upsuper commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment