Skip to content

[cssom] We may not want .setProperty() to change the order if it's setting an identical value to an existing declaration #2667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
upsuper opened this issue May 10, 2018 · 13 comments
Labels
cssom-1 Current Work

Comments

@upsuper
Copy link
Member

upsuper commented May 10, 2018

So in #1898, we resolved to have .setProperty() behave like appending rather than changing in-place. It causes problem that browsers become unable to optimize out setting identical values. When that happens in inline style, that can consequently trigger mutation event and observer.

From Gecko we have seen bug 1449584 and bug 1460295 related to this issue.

It seems Blink currently appends declaration when it's changing the value, and does nothing otherwise.

If we want to go that way, for the issue raised in #1898, it means although code like

elem.style.marginTop = "1px";
elem.style.marginBlockStart = "2px";
elem.style.marginTop = "0px";

would still work as expected, but something like

elem.style.marginTop = "1px";
elem.style.marginBlockStart = "2px";
elem.style.marginTop = "1px";

would not.

It's not clear to me whether that's something reasonable.

(Avoiding changing the declaration block when updating declaration with identical value wouldn't affect fixing #2515, though, which is good.)

@upsuper upsuper added the cssom-1 Current Work label May 10, 2018
@tabatkins
Copy link
Member

I think it's quite a problem if the second example doesn't set the top margin to 1px. The whole point of the order-changing is to make sure you can treat the logical and physicals as aliases of each other, and in this case, the current value "the top margin" is 2px, and you're changing it to 1px. The fact that a previous value was 1px is irrelevant to the author.

@upsuper
Copy link
Member Author

upsuper commented May 10, 2018

If we don't want to change the resolution in that case, I hope other browsers can follow the change as soon as possible, since that has started causing issue for us... If other browsers don't want to do this, Gecko may probably need to revert the behavior for webcompat reason at some point...

@FremyCompany
Copy link
Contributor

I think a middle ground can be found here. You are allowed not to reorder the properties if there are no properties defined after the one you just set that would override yourself.

elem.style.marginTop = "0px";
elem.style.marginBlockStart = "100px";
...
elem.style.marginTop = "0px"; // fires a style attribute update

but

elem.style.marginTop = "0px";
elem.style.paddingBlockStart = "100px";
...
elem.style.marginTop = "0px"; // doesn't fires a style attribute update

@emilio
Copy link
Collaborator

emilio commented May 10, 2018

That looks somewhat confusing from the developers' perspective, I think... But could work I guess. It could be confusing specially when setting a shorthand and only some properties get moved away...

@emilio
Copy link
Collaborator

emilio commented May 10, 2018

It's also a bit messy I think... You'd need to check all the potentially affected logical counter-parts...

@FremyCompany
Copy link
Contributor

Not really related but just to show that we need to change that serialize spec anyway:
https://jsfiddle.net/au00bj23/

I tried to run the steps defined in cssom in my head three times already, I still don't understand how browsers arrive at the result they currently arrive in.
https://www.w3.org/TR/cssom-1/#serialize-a-css-declaration-block

@upsuper
Copy link
Member Author

upsuper commented May 11, 2018

That is related to #2515, which needs a separate fix to the serialization algorithm. And requiring appending for setting different value is a prerequisite of that fix.

@FremyCompany
Copy link
Contributor

The problem isn't only with variable properties, nor only with the order in which things are defined in the array. An amazing example: https://wptest.center/#/w1gw11

So this is just a mess.

Here is how we currently do things in Edge and how I plan to potentially extend things in the future at the moment:

  • Properties are always serialized in the same order, regardless of their order in the array (margin always before padding, etc...)
  • All the logical properties we will add will be shadowed under a fully-encompassing shorthand (e.g. "margin"), and serialization of all the longhands associated to this shorthand (e.g. "margin-top" and "margin-block-start") will always happen at the location where that full shorthand would be serialized if it could be. Their relative order will be preserved (is "margin-top" before or after "margin-block-start") but not their order in the full sequence (is "padding-top" before "margin-top").

This way we should limit the style attribute changed notification to a minimum while preserving correctness. Whether our implementation actually stores the full order or not doesn't matter because the serialization step normalizes that order away where it doesn't matter.

@upsuper
Copy link
Member Author

upsuper commented May 11, 2018

Hmmm... I see what you mean. That's definitely another issue related to shorthand serialization. The algorithm is broken in that case as well :/

So you are proposing a different algorithm which doesn't preserve the order in general, but the order between physical properties and their corresponding logical properties? Hmm...

@upsuper
Copy link
Member Author

upsuper commented May 11, 2018

I think the right solution to that problem is that, both logical and physical properties should be subproperties of their corresponding shorthands. That is also the only feasible way to implement the logical keyword in the four-direction shorthands.

But then we are going to have problem with margin-{inline,block} etc. again :/

@FremyCompany
Copy link
Contributor

FremyCompany commented May 12, 2018

Just realized there was no test case for the actual issue described here, so I created one.
https://wptest.center/#/l6dif3

@FremyCompany
Copy link
Contributor

FremyCompany commented May 12, 2018

And, again, based on my experiments, it's totally doable to pass this test yet fire no attribute modification (didn't yet adapt the style serializer to serialize depending on the order of logical-able properties, so the "style" attribute and its internal representation do not round trip, but since this is not the only case I'd be tempted not to care about it until someone actually rewrite the algorithm to consider more cases)

@emilio
Copy link
Collaborator

emilio commented Mar 4, 2019

I believe this was fixed in #2924, please reopen otherwise.

@emilio emilio closed this as completed Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cssom-1 Current Work
Projects
None yet
Development

No branches or pull requests

4 participants