Skip to content
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

[cssom-1] Replace steps of set a CSS declaration with some constraints #2924

Merged
merged 1 commit into from Aug 17, 2018

Conversation

Projects
None yet
6 participants
@upsuper
Copy link
Member

commented Jul 13, 2018

(This depends on concepts being added to css-logical in #2922.)

This commit replaces the steps of "set a CSS declaration" with some constraints, so that user agents are allowed to sort the declarations in whatever way they want as far as those constraints hold.

It's not clear to me what is the right wording for this.

@upsuper upsuper requested review from emilio and therealglazou Jul 13, 2018

@upsuper upsuper added the cssom-1 label Jul 13, 2018

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2018

This would definitely need a working group resolution.

@emilio emilio requested a review from zcorpan Jul 14, 2018

at an index after all of those <a>CSS declaration</a>s.

<li>
The steps must return true if the serialization of <var>declarations</var> is changed as a result of

This comment has been minimized.

Copy link
@zcorpan

zcorpan Jul 14, 2018

Member

Word this as "If the serialization of declarations was changed as result of these steps, then return true. Otherwise, return false."

This comment has been minimized.

Copy link
@upsuper

upsuper Jul 15, 2018

Author Member

This wording "If ... then ... otherwise" sounds like part of steps, but this should be a constraint I think.

or removed from <var>declarations</var>.

<li>
If there are <a>CSS declaration</a>s in <var>declarations</var> whose

This comment has been minimized.

Copy link
@zcorpan

zcorpan Jul 14, 2018

Member

Move the s inside the <a>

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2018

To adopt this change, we need to remove two existing wpt and introduce a looser wpt. I'm considering doing this in Gecko bug 1473180. It's not clear to me whether I can change the corresponding wpt without a resolution from the working group.

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2018

@upsuper can you open an issue sketching the problem and the proposal and put the Agenda+ label?

I'll attend this week's call and I can try getting a resolution there.

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2018

I don't think we need a separate issue for that. I can put the problem and add Agenda+ on this PR directly.

So in #1898 we resolved (again) that seting property should always append the new declaration to the end of the declaration block, which was added to spec in #2516. However, this change has a performance impact, because browsers would be required to trigger mutation observer, and flush style. (To be honest, flushing style may be avoidable, but it could be complicated to fix given how things are working nowadays.)

Because of this, the change to Firefox has been put behind a pref and off by default, and the change to Chrome has been backed out.

In the end, in #1898 what we really want is that physical properties and flow-relative properties can override each other correctly. There are possible algorithms to realize that without sacrificing performance in usual cases, but such algorithms may be non-trivial and it could become tricky to spec and have everyone agree on one.

Thus, instead I propose that we just put down some basic requirements for this algorithm and allow implementations to explore. There would be observable behavior difference, but I don't think that would be too bad. If authors have been relying on the order, the original change itself would have been a breaking change.

@FremyCompany

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

👍

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

The Working Group just discussed Replace steps of set a CSS declaration with some constraints, and agreed to the following:

  • RESOLVED: havethe PR https://github.com/w3c/csswg-drafts/pull/2924 as the start of a set of constraints with gecko algorithm as an example in a note
The full IRC log of that discussion <dael> Topic: Replace steps of set a CSS declaration with some constraints
<dael> github: https://github.com//pull/2924
<dael> ??: Issue is that the latest resolution on how set property behaved: it always appends to end of declaration so it's sane with logical prop it's a nightmare of webcompat and perf for Gecko and Blink
<fantasai> dbaron, discussion at https://github.com//issues/1246
<dael> ??: WE turned it off in Gecko and backed out in block. xidorn had this proposal to let a se t of prop in a logicial group and in a UA dependent way that's in same logicial group it need to appear after so setProperty behaves correct
<rego> s/??/emilio
<dbaron> s/in block/in Blink/
<dael> emilio: I think frremy...what xidorn did in gecko which we haven't landed is that if you get to the case where a prop and there's another from the group that defers we append the new prop
<dael> emilio: xidorn proposes to define in terms of constraints which I'm okay, but prefer define properly. Onlyr eason not to do is proposal from frremy. We need to decide if we're fine resolving like this or if fine to say it's constaints UA can do what they want or define algo in spec
<dael> frremy: From what I recal my proposal was pretty in line with constaints. I'm fine with them as defined. Good to have UA experiment. If it's fine we can refine further. Fine to go with xidorn proposal for now. It makes a lot of sense.
<dael> emilio: Okay
<dael> astearns: I agree emilio it's good to have things properly defined once we have impl experience and can determine the constraints. Happy starting with the PR and adding
<dael> emilio: People fine with gecko algo as an example?
<dael> florian: SOunds okay
<dael> astearns: As a note?
<dael> emilio: Pretty much
<AmeliaBR> present +
<dael> astearns: Objections to having the PR https://github.com//pull/2924 as the start of a set of constraints with gecko algo as an exmplae in a note?
<dael> frremy: Sounds good
<dael> RESOLVED: havethe PR https://github.com//pull/2924 as the start of a set of constraints with gecko algorithm as an example in a note
<dael> astearns: Anything else on this?
@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

Updated the PR to include an example algorithm that we are using in Gecko. Related web-platform test would be landed (by sync bot) as soon as the Gecko change gets merged.

@emilio

emilio approved these changes Jul 19, 2018

Copy link
Collaborator

left a comment

Looks great, thanks!

Feel free to merge this, probably after #2922 lands as well.

Thanks very much again :)

@gsnedders

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

Because of this, the change to Firefox has been put behind a pref and off by default, and the change to Chrome has been backed out.

So what's the current behaviour? How interoperable are they? What would it take to converge behaviour in some other way to what we previously resolved in #1898?

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

Feel free to merge this, probably after #2922 lands as well.

I think we should have that landed first.

So what's the current behaviour? How interoperable are they?

Current behavior is to always update existing declaration with the same property name. I believe at least Blink and Gecko both do this, likely WebKit as well. Edge may have some other idea.

Blink (and probably WebKit as well) doesn't trigger mutation observer if the value and flag aren't changed, while Gecko did. That caused performance issue for us and thus we fixed that in 62 or 63.

What would it take to converge behaviour in some other way to what we previously resolved in #1898?

I don't think we are going to use the algorithm we previously resolved in #1898 because that has webcompat issue.

We may eventually converge to some algorithm, but it's probably not going to happen very soon, as a conformant algorithm without webcompat issue is going to be somewhat complicated, and getting everyone agree on every details could take quite a bit of time.

Also there are other stuff currently in my mind that we should probably add to this constraints which may make things more complicated...

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

BTW, the corresponding web-platform test is going to be landed in web-platform-tests/wpt#12065.

@upsuper upsuper merged commit 5f21336 into w3c:master Aug 17, 2018

1 check passed

ipr PR deemed acceptable.
Details
Issue: Should we add something like "Any observable side effect must not be made outside
<var>declarations</var>"? The current constraints sound like a hole for undefined behavior.

Note: The steps of <a>set a CSS declaration</a> are not defined in this level of CSSOM. The user agent may

This comment has been minimized.

Copy link
@FremyCompany

FremyCompany Aug 17, 2018

Contributor

nit: plural: user agents may use different algorithms as long as the contraints above hold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.