-
Notifications
You must be signed in to change notification settings - Fork 141
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
base: main
Are you sure you want to change the base?
Conversation
@shans PTAL. Thanks! |
@shans PTAL. Thanks! |
LGTM. @tabatkins ? |
css-typed-om/Overview.bs
Outdated
: 'display' | ||
:: | ||
1. Let |values| initially be the empty [=list=]. | ||
2. If 'display-outside' or 'display-inside' is specified: |
There was a problem hiding this comment.
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. ^_^
There was a problem hiding this comment.
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! :)
css-typed-om/Overview.bs
Outdated
:: | ||
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|. |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
css-typed-om/Overview.bs
Outdated
2. Append value of 'display-inside' to |values|. | ||
3. Goto step 7. | ||
3. If 'display-listitem' is specified: | ||
1. Append ''list-item'' to |values|. |
There was a problem hiding this comment.
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''
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
css-typed-om/Overview.bs
Outdated
1. Append its value to |values|. | ||
2. Goto step 7. | ||
7. If |values| is: | ||
1. ''block flow'', return 'block'. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
css-typed-om/Overview.bs
Outdated
11. ''inline ruby'', return ''ruby''. | ||
12. ''block table'', return ''table''. | ||
13. ''inline table'', return ''inline-table''. | ||
14. [=list/empty=], return "none". |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
tabatkins@ I've addressed your comments, PTAL, thanks! |
Add description for serializing 'display' property.