Skip to content

[cssom] Make CSSStyleRule serialization aware of nesting.#6117

Merged
tabatkins merged 1 commit into
w3c:mainfrom
tabatkins:cssom-nesting
May 10, 2021
Merged

[cssom] Make CSSStyleRule serialization aware of nesting.#6117
tabatkins merged 1 commit into
w3c:mainfrom
tabatkins:cssom-nesting

Conversation

@tabatkins

Copy link
Copy Markdown
Member

@emilio review please?

@tabatkins tabatkins added the cssom-1 Current Work label Mar 18, 2021
@tabatkins tabatkins requested a review from emilio March 18, 2021 23:53
Comment thread cssom-1/Overview.bs
<li>If |decls| is not null, prepend it to |rules|.
<li>For each |rule| in |rules|:
<ol>
<li>Append a newline followed by two spaces to |s|.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit weird that we're doing this fake indentation thing. So for nested declarations this would look like:

Source:

a {
  b {
    c {
      color: red;
    }
  }
}

cssText:

a {
  b {
  c { color: red; } }
}

Which is a bit off. I think I'd rather avoid newlines completely, but not particularly opposed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely agree that this is busted, I'm just copying the existing indentation treatment for the other rules in the spec; @media already shows off that exact same awful behavior, for instance.

I didn't want to try and fix the indentation for just this rule and leave the rest busted, so I stuck with the current behavior and assume that all of them can be fixed up at once in a separate commit.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, thanks.

@tabatkins tabatkins merged commit 4471096 into w3c:main May 10, 2021
@tabatkins tabatkins deleted the cssom-nesting branch May 10, 2021 22:16
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

Successfully merging this pull request may close these issues.

2 participants