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

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

Closed
emilio opened this issue Oct 23, 2017 · 18 comments

Comments

Projects
None yet
9 participants
@emilio
Copy link
Collaborator

commented Oct 23, 2017

Or if it should, order of the expansion of all should be specified.

From https://drafts.csswg.org/css-cascade-3/#all-shorthand:

The all property is a shorthand that resets all CSS properties except direction and unicode-bidi. It only accepts the CSS-wide keywords.

This includes logical properties, and this is a problem because that means that it produces unexpected results when used from CSSOM (where the position a property declaration appears on matters).

In particular, we came across https://bugzilla.mozilla.org/show_bug.cgi?id=1410028, which happens because of the internal order the properties are reset.

In particular, it's not the same if all expands to:

padding-inline-start: initial;
padding-left: initial;

that if it expands the other way around.

i.e., this two pieces of code should be equivalent:

<!doctype html>
<script>
let setStyle = (el, props) => {
  for (prop in props)
    el.style.setProperty(prop, props[prop]);
};

let logical = document.createElement('div');
let physical = document.createElement('div');

setStyle(physical, {
  all: 'initial',
  'background-color': 'blue',
  display: 'block',
  width: '100px',
  height: '100px',
  padding: '10px'
});

setStyle(logical, {
  all: 'initial',
  'background-color': 'blue',
  display: 'block',
  width: '100px',
  height: '100px',
  'padding-inline-start': '10px',
  'padding-inline-end': '10px',
  'padding-block-start': '10px',
  'padding-block-end': '10px'
});

document.documentElement.appendChild(physical);
document.documentElement.appendChild(logical);
</script>

But they aren't in current browsers.

@upsuper

This comment has been minimized.

Copy link
Member

commented Oct 23, 2017

The problem of making all not expand to logical properties is that it in some cases it would fail to reset all properties. For example, if someone has

elem.style.paddingLeft = '10px';
elem.style.paddingInlineStart = '10px';
elem.style.all = 'initial';

people should be expecting that there is no padding anymore. However, if all doesn't reset logical properties, we would still have padding-inline-start left there.

I guess the solution should be having the order defined in the way that, logical properties go before physical ones. This doesn't fix the issue that the two pieces of code having different behavior, but it at least provides a behavior compatible with pre-logical world, and can consistently reset all properties with all.

@emilio

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 23, 2017

I guess the solution should be having the order defined in the way that, logical properties go before physical ones. This doesn't fix the issue that the two pieces of code having different behavior, but it at least provides a behavior compatible with pre-logical world, and can consistently reset all properties with all.

Well, the problem you described is a non-issue in a pre-logical world too, fwiw, but I agree that whatever path we take here is going to make some behavior inconsistent (whether it's my example, or yours).

@heycam

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2017

I guess this is not just an issue with all, but for any shorthand that sets logical properties. For example:

elem.style.writingMode = "horizontal-tb";
elem.style.marginTop = "1px";
elem.style.marginBlockStart = "2px";
elem.style.margin = "0px";  // top margin is still 2px :(
@emilio

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 23, 2017

You mean for any shorthand that sets physical properties with logical counterparts, right?

And yeah, not even that, but just something like:

elem.style.writingMode = "horizontal-tb";
elem.style.marginTop = "1px";
elem.style.marginBlockStart = "2px";
elem.style.marginTop = "0px";  // top margin is still 2px :(

Given that (which is basically the problem Xidorn described before) is a more general problem with logical props themselves, I think we should make all not reset them, and keep the order unspecified. But not sure what other people think.

It may be worth trying to come up with a solution for that problem in a more general way, but it seems hard / impossible to me, because you don't know what physical property is actually represented by that property until computed value time.

@Loirooriol

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2017

Note that, if you need to rely on the order of an iteration, you should not use for-in loops:

The mechanics and order of enumerating the properties is not specified

Instead, you should use something like for (prop of Reflect.ownKeys(props)), because the order of OrdinaryOwnPropertyKeys is explicitly defined.

Other than this, I think the result would always be as expected if you used removeProperty just before setProperty. Would it be backwards-compatible if CSSOM automatically did this?

@adomasven

This comment has been minimized.

Copy link

commented Oct 24, 2017

Food for thought: what happens in this scenario?

elem.style.marginTop = "1px";
elem.style.marginBlockStart = "2px";
elem.style.writingMode = "horizontal-tb"; // is top margin 1px or 2px now?
@upsuper

This comment has been minimized.

Copy link
Member

commented Oct 24, 2017

Food for thought: what happens in this scenario?

If it starts with an empty style declaration block, then it is deterministic from the spec that, the top margin should be 2px, because margin-top comes before margin-block-start and thus it's overridden by the latter. It is just like

elem {
  margin-top: 1px;
  margin-block-start: 2px;
  writing-mode: horizontal-tb;
}

I think this is clear in the spec, and it is not the problem here.

@upsuper

This comment has been minimized.

Copy link
Member

commented Oct 24, 2017

Other than this, I think the result would always be as expected if you used removeProperty just before setProperty. Would it be backwards-compatible if CSSOM automatically did this?

Maybe... I somehow think the current behavior makes more sense than always appending the changed declaration... except when it interacts with shorthands and logical properties...

Probably what we can do is:

  • define the expand order for shorthands
  • have CSSOM remove all longhands including their corresponding logical properties from the declaration block when it is setting a shorthand

With the first, we would have interoperable deterministic result, and with the second, the result should be less surprising when people try to set shorthand when some longhands have been there.

@Loirooriol

This comment has been minimized.

Copy link
Collaborator

commented Oct 24, 2017

Probably what we can do is:

  • define the expand order for shorthands
  • have CSSOM remove all longhands including their corresponding logical properties from the declaration block when it is setting a shorthand

But this wouldn't fix the following, would it?

elem.style.marginTop = "1px";
elem.style.marginBlockStart = "2px";
elem.style.marginTop = "0px";
getComputedStyle(elem).marginTop; // "2px" :(

A basic cascading rule is that the last declaration wins when there is a tie. So I really think that by default CSSOM should move the declaration to the end when you assign a value (but a method that preserves the order could be added). If this is not backwards-compatible, please add a note in the spec warning about this misbehavior, and recommend to use removeProperty before setting a property.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 25, 2017

Bug 1410028: Add reftest. r=me
Not in WPT because w3c/csswg-drafts#1898 is not
resolved yet.

MozReview-Commit-ID: HRM7AcMcJVd

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 26, 2017

Bug 1410028 - Add reftest. r=me, a=ritu
Not in WPT because w3c/csswg-drafts#1898 is not
resolved yet.

MozReview-Commit-ID: HRM7AcMcJVd

--HG--
extra : source : d0896f475d15c65a5fe4afcf86c0e42713100952
extra : amend_source : 0be9c9b9aa7672e5e53020e28cff10d2c47e0fa6

xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this issue Oct 26, 2017

Bug 1410028 - Add reftest. r=me, a=ritu
Not in WPT because w3c/csswg-drafts#1898 is not
resolved yet.

MozReview-Commit-ID: HRM7AcMcJVd
@tabatkins

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

I've retagged this as just a CSSOM issue, and marked it as a f2f issue, because it's kinda complicated. Retag the other specs if you disagree and think that there is something they should do.


As far as I can tell, the problem is:

  • el.style is backed by a "list of declarations"; when you set el.style, if this declaration was previously set, it updates it in-place; if not, it appends a new declaration to the end.
  • This is then serialized to the style attribute in the "list of declarations" order.
  • thus, setting el.style.fooTop="one"; el.style.fooBlockStart="two"; el.style.fooTop="three"; will serialize to style="foo-top: three; foo-block-start: two;", and the logical value will end up winning, per the Cascade rules for how to resolve coinciding logical/physical declarations. (Same applies in reverse if we set logical/physical/logical.)
  • so this appears to be a CSSOM issue with it not respecting order-of-setting in a way that matches with the order-in-ruleset that Cascade depends on.

Right?

So Xidorn and Oriol have suggestions:

  1. Xidorn suggests that when you set a property with phys/log counterparts, unset the corresponding other property from the el.style data structure.
  2. Oriol suggests always appending (and removing the old declaration from the list), so that serialization preserves the order the properties were set in.

(1) seems hard to do - you need to know the final computed value of writing-mode, direction, and maybe text-orientation to figure out the corresponding property, and those might be set by a completely different stylesheet.

(2) seems to cleanly fit in with the Cascade machinery, tho there might be compat concerns.

@Loirooriol

This comment has been minimized.

Copy link
Collaborator

commented Oct 27, 2017

this is then serialized to the style attribute in a predefined order (the order you set the properties in doesn't matter)

The order does matter. That's because set a CSS declaration value uses a "list of declarations", and lists are ordered. The algorithm also says "append", which implies there is an order. (Firefox and Chrome are compliant, but it seems Edge does not respect this insertion order).

The problem is that the algorithm only appends the new declaration if there was no previous declaration of the same property. Otherwise, it only updates the value of the old one, without moving it to the end.

Therefore, if you first set properties using all and then set a property to another value, you don't know whether the new value will win the cascade or not, because the order in which all inserted declarations into the list does not seem to be defined anywhere.

Additionally, the resulting declarations of the following steps will be

elem.style.marginTop = "1px";        // { margin-top: 1px }
elem.style.marginBlockStart = "2px"; // { margin-top: 1px; margin-block-start: 2px }
elem.style.marginTop = "0px";        // { margin-top: 0px; margin-block-start: 2px }

so (assuming writing-mode: horizontal-tb) the top margin will end up being 2px instead of 0px. Instead, I think it should be

elem.style.marginTop = "1px";        // { margin-top: 1px }
elem.style.marginBlockStart = "2px"; // { margin-top: 1px; margin-block-start: 2px }
elem.style.marginTop = "0px";        // { margin-block-start: 2px; margin-top: 0px }
@tabatkins

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

Ah, thanks for the clarification! Doesn't change the conclusion much, but tweaks the details a bit.

@tabatkins

This comment has been minimized.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

The Working Group just discussed logical/physical property cascade and the .style API, and agreed to the following resolutions:

  • RESOLVED
  • RESOLVED: Make it so that setters to the .style api are appended rather than put in place
The full IRC log of that discussion <gregwhitworth> Topic: logical/physical property cascade and the .style API
<astearns> github: https://github.com//issues/1898
<gregwhitworth> TabAtkins: someone pointed out that there are problems with the way logical properties are defined when trying to use the DOM APIs
<gregwhitworth> TabAtkins: they get resolved in the normal order and then go through the cascade and we deal with the physical and logical based on ordering and writing mode
<gregwhitworth> TabAtkins: when you set a property it gets appended to the list of properties in the style attribute
<gregwhitworth> TabAtkins: and if you set one that's already there it just replaces it
<gregwhitworth> TabAtkins: so this can cause it to be out of sync with what would happen in the cascade
<gregwhitworth> TabAtkins: the proposal is to amend the CSSOM that if something is there it is always appended
<gregwhitworth> TabAtkins: related to this
<gregwhitworth> TabAtkins: we had an issue with !important and we resolved to append to make this work
<gregwhitworth> dbaron: I'm happy to make that change
<gregwhitworth> dbaron: we had a long discussion and resolved the opposite way
<gregwhitworth> dbaron: a setter should append
<gregwhitworth> dbaron: at the time implementations were inconsistent due to optimizations
<gregwhitworth> dbaron: for existing properties in the fast path they would replace and others they would append
<gregwhitworth> dbaron: one question, would having to do that mess up optimizations?
<gregwhitworth> TabAtkins: when we discussed it internally we were fine making the change
<gregwhitworth> dbaron: I found the minutes from 2013 but I'm looking for the other one
<gregwhitworth> astearns: so I'm clear, the suggestion to append or remove and then append?
<gregwhitworth> TabAtkins: those are equivelant
<gregwhitworth> dbaron: I think it may turn out that it's not equivelant in the future
<gregwhitworth> TabAtkins: we should figure out which one we want, it will need to have a note to determine this
<gregwhitworth> dholbert: you could inspect the style attr and determine that way
<gregwhitworth> TabAtkins: the current model limits to only one
<gregwhitworth> astearns: I'm hearing two engines happy to change
<gregwhitworth> astearns: any compat concerns?
<gregwhitworth> dbaron: possibly
<gregwhitworth> astearns: the compat issue would be limited to logical props shorthands?
<gregwhitworth> TabAtkins: no
<gregwhitworth> fantasai: it would only affect people looking at .style expecting a particular order
<gregwhitworth> fantasai: which doesn't make much sense, so it's probably not very common
<gregwhitworth> florian: I doubt it's common
<gregwhitworth> Rossen: tools and editors might be doing this
<gregwhitworth> TabAtkins: I'd be surprised if the editor was reading the style and writing to .style
<gregwhitworth> Rossen: they could
<gregwhitworth> astearns: The proposed resolution is to change CSSOM to append values via the .style API and add a note
<gregwhitworth> TabAtkins: yeah, and try to figure that out in the future
<dbaron> Things I found were https://lists.w3.org/Archives/Public/www-style/2013Sep/0469.html and https://lists.w3.org/Archives/Public/www-style/2013Oct/0007.html but I think there was further followup after the latter
<gregwhitworth> astearns: any other opinions? objections?
<gregwhitworth> RESOLVED
<gregwhitworth> RESOLVED: Make it so that setters to the .style api are appended rather than put in place
@Loirooriol

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2017

the suggestion to append or remove and then append

If old declarations are not removed and the element continuously receives new styles (e.g. simulating an animation using JS instead of CSS Animations), then the list of declarations may become huge. This will waste memory for no reason, won't it?

@emilio

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 7, 2017

I'm assuming that you'd remove other entries in the declaration block for the same property, which behavior-wise is identical to what happens during parsing (and remove + append).

At least Firefox and Servo rely on this invariant for a single declaration block.

Browser bugs:

@emilio

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 9, 2017

I just realized that there's a gotcha, and it's that you can't optimize out redundant setters anymore, unless it's the last position on the declaration block.

For example, all browsers I know of implement an optimization to avoid doing stuff in the case the property set had the same value. This is no longer applicable unless the property is at the end of the block, which isn't great... :(

@zcorpan

This comment has been minimized.

Copy link
Member

commented Feb 11, 2018

For the record: From #2269 (comment) which I think is basically the same issue as @emilio's comment above

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?

@emilio emilio closed this in #2516 Apr 11, 2018

emilio added a commit that referenced this issue Apr 11, 2018

fergald added a commit to fergald/csswg-drafts that referenced this issue May 7, 2018

@emilio emilio changed the title [css-logical-1] [css-cascade-3] The all longhand probably shouldn't set logical properties. [css-logical-1] [css-cascade-3] The all shorthand probably shouldn't set logical properties. May 15, 2018

aarongable pushed a commit to chromium/chromium that referenced this issue May 19, 2018

[cssom] Make CSSOM APIs always append to the declaration block.
...instead of replacing if the property is already set.

This matches the spec[1] which was changed as a resolution from[2].

Intent thread: https://groups.google.com/a/chromium.org/d/msg/blink-dev/lzBoa1EAQpI/strfpNczAwAJ

[1]: https://drafts.csswg.org/cssom/#set-a-css-declaration
[2]: w3c/csswg-drafts#1898

Bug: 782407
Change-Id: I4b72c0207a4cf734fc2e415bd62c7863103ec309
Reviewed-on: https://chromium-review.googlesource.com/1054867
Commit-Queue: Emilio Cobos Álvarez <emilio@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560180}
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.