csswg-drafts icon indicating copy to clipboard operation
csswg-drafts copied to clipboard

Published 20 hours ago • verified publisher icon w3c

[cssom] Serialization of a declaration block not idempotent in presence of logical properties.

Open emilio opened this issue 6 years ago • 9 comments

Testcase is:

<!doctype html>
<div id="test" style="margin-top: 10px; margin-block-start: 20px; margin-inline-start: 30px; margin-bottom: 40px; margin-left: 50px; margin-right: 60px"></div>
<pre>
<script>
  document.writeln(test.style.cssText);
</script>
</pre>

Which serializes to:

margin: 10px 60px 40px 50px; margin-block-start: 20px; margin-inline-start: 30px;

Which is not idempotent.

cc @FremyCompany

emilio avatar Oct 24 '18 01:10 emilio

cc @Loirooriol too :)

emilio avatar Oct 24 '18 01:10 emilio

I think this is easy to fix, and we may want to just not compress shorthands if we'd need to jump over a declaration with the same logical group in order to do that.

emilio avatar Oct 24 '18 01:10 emilio

To avoid overlap with #1282 (margin may end up resetting the logical longhands), we can consider this testcase:

$0.style.cssText = "margin-block-start: 10px; margin-bottom: 20px; margin-block-end: 30px";
$0.style.cssText;

It shouldn't serialize as margin-block: 10px 30px; margin-bottom: 20px.

I agree with your solution.

Loirooriol avatar Oct 24 '18 06:10 Loirooriol

I think the current use of the concept in setProperty allows 3 different border logical groups (border-width, border-style and border-color), but

$0.style.cssText = "border: 1px solid red; border-block-start-color: green; border-color: blue";
$0.style.cssText;

should not be "border: 1px solid blue; border-block-start-color: green;"

So either all border longhands should be in the same logical group (I would like some clarification in #3033), or instead of checking what I initially imagined, i.e.

  • there is no triplet of properties A, B, C such that
    • index(A) < index(B) < index(C)
    • A, B, C belong to the same logical group, but B has different mapping logic than A and C
    • A and C belong to current longhands

it should be

  • there is no pair of properties A, B such that
    • index(property) < index(A) < index(B)
    • A, B belong to the same logical group but have different mapping logic
    • B belongs to current longhands

where property and current longhands are the ones from the algorithm in https://drafts.csswg.org/cssom/#serialize-a-css-declaration-block

Loirooriol avatar Oct 25 '18 08:10 Loirooriol

There's also the case of the property containing both the logical and the physical longhands (i.e., all)... That one's fun, since it's not defined in which order we expand the properties even... I guess since all only allows a CSS wide keyword, this doesn't really matter in practice, but...

emilio avatar Oct 26 '18 02:10 emilio

This has been fixed by https://github.com/w3c/csswg-drafts/commit/dc37a3febec22610ac67b83b7c280a2d5b9cb129, isn't it?

cdoublev avatar Oct 19 '22 14:10 cdoublev

Firefox handles margin-block-start: 10px; margin-bottom: 20px; margin-block-end: 30px well now. But border: 1px solid red; border-block-start-color: green; border-color: blue is still wrong. Either Firefox didn't implement the spec well, or the spec needs more refinement.

Edit: the spec seems fine

Loirooriol avatar Oct 19 '22 15:10 Loirooriol

I have not implemented the step added by the commit I linked above. Could you please tell me why margin-block-start: 10px; margin-bottom: 20px; margin-block-end: 30px does not serialize to margin-bottom: 20px; margin-block: 10px 30px?

EDIT: hmm, I guess a round-trip would change the declaration order (margin-block-start would move after margin-bottom), therefore it should stay as specified in the initial input.

I would expect the second example to serialize to border-width: 1px; border-style: solid; border-block-start-color: green; border-color: blue? Is it correct?

cdoublev avatar Oct 19 '22 18:10 cdoublev

Don't forget border-image-*. I think it should be:

  1. In some undefined order:
    • border-style: solid
    • border-width: 1px
    • border-image: none 100% / 1 / 0 stretch (or equivalent)
  2. border-block-start-color: green
  3. border-color: blue

Loirooriol avatar Oct 19 '22 22:10 Loirooriol

For the following case:

style.borderTopWidth = '1px'
style.borderBlockStartWidth = '2px'
style.borderTopStyle = 'solid'
style.borderTopColor = 'green'

I would expect style.cssText to be border-top: 1px solid green; border-block-start-width: 2px; but the current definition of step 6 of serialize a CSS declaration block seems to prevent serializing with border-top.

If so, I think it could be modified with:

- If there’s any declaration in `declaration block` in between the first and the last longhand in `current longhands` which belongs to the same logical property group, but has a different mapping logic as any of the longhands in `current longhands`, and is not in `current longhands`, continue with the steps labeled `shorthand loop`.
+ If there’s any declaration in `declaration block` in between some declarations in `current longhands` which belongs to the same logical property group, but has a different mapping logic as any of the longhands of these declarations, and is not in `current longhands`, continue with the steps labeled `shorthand loop`.

cdoublev avatar Oct 23 '22 07:10 cdoublev

I think the spec is saying the same as your wording, so it shouldn't be preventing the serialization with border-top. Basically, being between some longhands that match a certain condition is equivalent to being between the first and the last longhands that match the condition.

But I agree the spec is not particularly clear, I had to read it a few times to understand it. Your wording is not particularly clear to me either, what does "these declarations" refer to? This looks better to me:

If there’s any declaration in declaration block in between the first and the last longhand in current longhands which is not in current longhands, and belongs to the same logical property group but has a different mapping logic as any of the longhands in current longhands, continue with the steps labeled shorthand loop.

Note that now it's clear that the "which" refers to "any declaration" and not to "first/last longhand in current longhands". And no comma after "same logical property group", otherwise it's not clear that "as any of the longhands in current longhands" refers to both "same" and "different".

Loirooriol avatar Oct 23 '22 20:10 Loirooriol

Basically, being between some longhands that match a certain condition is equivalent to being between the first and the last longhands that match the condition.

In my example, the declaration for border-block-start-width is between the first and last declaration of current longhands, but it is not between declarations whose longhands are in the same logical property group. There should be three declarations whose longhands are in the same logical property group: the declaration not in current longhands, and the "wrapping" declarations in current longhands.

To make it clear:

style.borderTopWidth = '1px'
style.borderBlockStartWidth = '2px'
// Some border-*-width is missing here 
// so that the above declaration is in between two declarations 
// whose longhands are in the same logical property group
style.borderTopStyle = 'solid'
style.borderTopColor = 'green'
style.cssText; // border-top: 1px solid green; border-block-start-width: 2px;

style.borderTopWidth = '1px'
style.borderBlockStartWidth = '2px'
style.borderRightWidth = '1px'
style.borderBottomWidth = '1px'
style.borderLeftWidth = '1px'
style.cssText; // border-top-width: 1px; border-block-start-width: 2px; border-right-width: 1px; border-bottom-width: 1px; border-left-width: 1px;

Note that Firefox serializes style.cssText to border-top-width: 1px; border-block-start-width: 2px; border-top-style: solid; border-top-color: green; in the first example. It may be right because the order of declarations would be preserved with a round-trip. I do not know if this should be a requirement for shorthand serialization.

I agree with your rewording. I also considered splitting the sentence (step) by listing conditions but ended up only rewording it to make my point. I was also not comfortable with a declaration in between longhands in current longhands because a declaration is compared to longhands, and current longhands contains declarations, not longhands.

But I am sure someone can come up with the ideal and correct wording.

cdoublev avatar Oct 24 '22 12:10 cdoublev

but it is not between declarations whose longhands are in the same logical property group

Precisely, the spec says "belongs to the same logical property group, but has a different mapping logic as any of the longhands in current longhands". So border-block-start-width doesn't have to be between 2 longhands in the same property group, just having border-top-width is enough.

Loirooriol avatar Oct 24 '22 15:10 Loirooriol

OK, sorry, I'm contradicting myself, that's what happens when I try to post quickly. So yeah, I guess that the spec is preventing the serialization with border-top even though it would be fine.

But note that requiring it to be between 2 declaration in the same logical group would be wrong, see https://github.com/w3c/csswg-drafts/issues/3244#issuecomment-432960155

Loirooriol avatar Oct 24 '22 15:10 Loirooriol

But note that requiring it to be between 2 declaration in the same logical group would be wrong, see #3244 (comment)

border should be skipped because the declaration for border-block-start-color is stored before other border-*-color declarations, right?

I guess it only applies to shorthands (there is only border) whose longhands belong to more than one logicial property group. In this case, the current wording is fine, yes.

cdoublev avatar Oct 24 '22 18:10 cdoublev

border should be skipped because the declaration for border-block-start-color is stored before other border-*-color declarations, right?

Exactly, we can't serialize border before border-block-start-color since some physical longhands of border in the border-color-* logical property group appear after border-block-start-color.

I guess it only applies to shorthands (there is only border) whose longhands belong to more than one logical property group

Not necessarily more than one logical property group. It suffices to have some longhands in one group (without covering the group completely), and some longhands not in the group (either another group or no group). But yeah I think border is the only case for now.

The spec could say:

If there’s any declaration in declaration block such that:

  • It's not in current longhands
  • It appears after the first longhand in current longhands
  • It appears before some longhand in current longhands that belongs to the same logical property group but has a different mapping logic

Loirooriol avatar Oct 25 '22 14:10 Loirooriol

I wonder when a declaration that would not be in current longhand and belong to the same logical property group than some longhands in current longhand, would not have a different mapping logic. This condition can be removed, isn't it?

I guess it could be further improved by serializing the interleaved declaration, in order to serialize border: 1px solid red; border-block-start-color: green; border-color: blue to border-block-start-color: green; border: 1px solid blue;. But this would prevent serializing eg. border: 1px solid red; border-block-start-color: green; border-block-end-color: green; border-color: blue to border-block-color: green; border: 1px solid blue;... unless you look for all interleaved declarations: fun stuff!

Another example for the observation I made in the previous paragraph is the output for style.cssText after declaring all to initial and removing/updating any longhand declaration: since physical longhands must now be canonically ordered after the corresponding logical longhands, border cannot be used because its reset-only sub-properties are declared before the logical border-* properties, as well as corners because corner-shape is declared before and border-radius-* are declared after the logical border-radius-* properties. Ordering all border and corners sub-properties after the logical properties would be a workaround for these specific cases though.

cdoublev avatar Oct 26 '22 07:10 cdoublev

I managed to implement the improvement mentioned in my previous comment. Basically it boils down to (if some condition is satisfied) resuming Declaration loop with the interfering declaration from step 6 (the one that is not in current longhands) before the current declaration (the first declaration of current longhands).

6. If there’s any declaration interfering in declaration block such that... :

  • It's not in current longhands
  • It appears after the first longhand in current longhands
  • It appears before some declaration in current longhands whose name belongs to the same logical property group but has a different mapping logic than the name of interfering

... then if all declarations in current longhands appearing before interfering in declaration block, are not in the same logical property group or have the same mapping logic than the name of interfering, resume the steps labeled declaration loop with interfering, otherwise resume the steps labeled shorthand loop.

Similarly as the obvervation I made in a previous comment, I think either are not in the same logical property group or have the same mapping logic is required.

Test cases to recap:

(Below, move declarations would be the result from a round trip)

// Currently resolved: move declarations backward
style.cssText = 'border-top-width: 1px; border-block-start-width: 1px; border-top-style: solid; border-top-color: green'
expect(style.cssText).toBe('border-top: 1px solid green; border-block-start-width: 1px;')

// Improvement: move declarations forward
style.cssText = 'border-top-width: 1px; border-block-start-style: none; border-top-style: solid; border-top-color: green'
expect(style.cssText).toBe('border-block-start-style: none; border-top: 1px solid green;')

// Guarded: skip shorthand when declarations cannot be moved backward/forward
let input = 'border-top-width: 1px; border-block-start-width: 1px; border-block-start-style: none; border-top-style: solid; border-top-color: green'
expect(style.cssText).toBe(input)

// More complex cases with moving declarations forward
style.cssText = 'border: 1px solid red; border-block-start-color: orange; border-color: green'
expect(style.cssText).toBe('border-block-start-color: orange; border: 1px solid green;')
style.cssText = 'border-top-width: 1px; border-block-start-width: 1px; border-block-end-width: 1px; border-top-style: solid; border-top-color: green'
expect(style.cssText).toBe('border-top: 1px solid green; border-block-width: 1px;')
style.cssText = 'border-block-width: 1px; border-top-width: 2px; border-top-style: none; border-block-style: solid; border-block-color: green;'
expect(style.cssText).toBe('border-top-style: none; border-block: 1px solid green; border-top-width: 2px;')

Let me know what you think.

cdoublev avatar Oct 31 '22 04:10 cdoublev

resume the steps labeled declaration loop with interfering

Can't just jump from declaration to interfering, since there could be unrelated declarations in between:

border-top-width: 1px; color: red; border-block-start-style: none; border-top-style: solid; border-top-color: green

Should not skip color: red. Also, there is no guarantee that shorthand will be serializable by the time that we reach the declaration in current longhands that appears after interfering and has the same logical group with opposite logic.

My concern with trying too hard to find the best serialization when there are logical properties is that it can make it much harder to also improve things like #2515. Optimizing for 2 different things is tricky.

Loirooriol avatar Oct 31 '22 18:10 Loirooriol

Can't just jump from declaration to interfering, since there could be unrelated declarations in between

Sorry, I did not mean to copy/paste code, but I meant declarations.splice(declarations.indexOf(currentLonghands[0]), 0, ...declarations.splice(declarations.indexOf(interfering), 1)) in JS code (were declarations is a shallow copy of the block declarations).

You also "jump" declarations when you look forward, no?

My concern with trying too hard to find the best serialization when there are logical properties is that it can make it much harder to also improve things like https://github.com/w3c/csswg-drafts/issues/2515.

Yeah, I know about this issue but I have not given much thought to it. The problem with find the best serialization seems to draw the line where it becomes too much when crossed.

cdoublev avatar Oct 31 '22 19:10 cdoublev

Well then the iteration order needs to be precisely defined. Note that when handling interfering then it may be potentially serializable via some shorthand, which may have its own interfering, and so on. Also note that serializing interfering first seems a bad idea if shorthand ends up not being serializable.

Loirooriol avatar Nov 02 '22 05:11 Loirooriol

Damn, you are right. It fails with the following entries (assuming the order is guaranteed).

    const input = {
        'border-block-start-width': '1px',
        'border-top-style': 'none',
        'border-block-end-style': 'solid',
        'border-right-style': 'none',
        'border-bottom-style': 'none',
        'border-left-style': 'none',
        'border-block-start-style': 'solid',
        'border-block-start-color': 'green',
    }
    const expected = {
        'border-top-style': 'none',
        'border-block-end-style': 'solid',
        'border-right-style': 'none',
        'border-bottom-style': 'none',
        'border-left-style': 'none',
        'border-block-start': '1px solid green',
    }

I revert to emilio's algorithm (which fixes this issue), ie. abort searching a shorthand when encoutering the first condition that might cause the serialization to not be idempotent, and I think this issue can be closed.

Thanks for the follow up!

cdoublev avatar Nov 02 '22 06:11 cdoublev