-
Notifications
You must be signed in to change notification settings - Fork 711
[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
Comments
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. |
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... |
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.
but
|
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... |
It's also a bit messy I think... You'd need to check all the potentially affected logical counter-parts... |
Not really related but just to show that we need to change that serialize spec anyway: 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. |
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. |
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:
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. |
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... |
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 But then we are going to have problem with |
Just realized there was no test case for the actual issue described here, so I created one. |
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) |
I believe this was fixed in #2924, please reopen otherwise. |
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
would still work as expected, but something like
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.)
The text was updated successfully, but these errors were encountered: