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

Serialize property display #461

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Serialize property display #461

wants to merge 4 commits into from

Conversation

jm318
Copy link
Contributor

@jm318 jm318 commented Aug 22, 2017

Add description for serializing 'display' property.

@jm318
Copy link
Contributor Author

jm318 commented Aug 22, 2017

@shans PTAL. Thanks!

@jm318
Copy link
Contributor Author

jm318 commented Aug 22, 2017

@shans PTAL. Thanks!

@shans
Copy link
Contributor

shans commented Aug 22, 2017

LGTM. @tabatkins ?

@shans shans requested a review from tabatkins August 22, 2017 06:21
: 'display'
::
1. Let |values| initially be the empty [=list=].
2. If 'display-outside' or 'display-inside' is specified:
Copy link
Member

Choose a reason for hiding this comment

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

These properties don't exist. You probably mean the <<display-inside>> and <<display-outside>> values. ^_^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for pointing out! :)

::
1. Let |values| initially be the empty [=list=].
2. If 'display-outside' or 'display-inside' is specified:
1. Append value of 'display-outside' to |values|.
Copy link
Member

Choose a reason for hiding this comment

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

The value is an abstract CSS value; need to serialize it to produce a JS value. (Same applies to all the steps.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

2. If 'display-outside' or 'display-inside' is specified:
1. Append value of 'display-outside' to |values|.
2. Append value of 'display-inside' to |values|.
3. Goto step 7.
Copy link
Member

Choose a reason for hiding this comment

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

This means you won't hit the list-item step at all if you specified something like display: block list-item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I understand this is what the spec says according to "|", i.e. these are mutually exclusive?

2. Append value of 'display-inside' to |values|.
3. Goto step 7.
3. If 'display-listitem' is specified:
1. Append ''list-item'' to |values|.
Copy link
Member

Choose a reason for hiding this comment

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

Append a string, "list-item", not a CSS value ''list-item''.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

1. Append its value to |values|.
2. Goto step 7.
7. If |values| is:
1. ''block flow'', return 'block'.
Copy link
Member

Choose a reason for hiding this comment

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

|values| is a list of strings, not a sequence of CSS values. Needs to be «"block", "flow"», per the Infra Standard.

Copy link
Member

Choose a reason for hiding this comment

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

And 'block' isn't a valid property, and should be a string instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for pointing out!

11. ''inline ruby'', return ''ruby''.
12. ''block table'', return ''table''.
13. ''inline table'', return ''inline-table''.
14. [=list/empty=], return "none".
Copy link
Member

Choose a reason for hiding this comment

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

How does it end up empty, and how does emptiness equate to display: none? none is handled by the <<display-box>> clause in step 5, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be empty if property is simply "display:", but I guess it's illegal anyway. So I removed the case for empty-list.

@jm318
Copy link
Contributor Author

jm318 commented Aug 23, 2017

tabatkins@ I've addressed your comments, PTAL, thanks!

Base automatically changed from master to main February 2, 2021 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants