-
Notifications
You must be signed in to change notification settings - Fork 747
[css-borders-4] Fixed and added definitions for box-shadow-offset: none #9096
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-borders-4] Fixed and added definitions for box-shadow-offset: none #9096
Conversation
css-borders-4/Overview.bs
Outdated
a list, each item consisting of four absolute lengths | ||
plus a computed color and optionally also a ''box-shadow-position/inset'' keyword | ||
Computed value: see individual properties | ||
Animation type: by computed value, |
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 should be "see individual properties".
Treating none
as 0 0
during interpolations should be in the propdef for box-shadow-offset
.
And below, omitted colors default to the ''currentcolor'' value
should be transparent
when the offset is none
. I think this will keep the existing behavior.
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 about now?
css-borders-4/Overview.bs
Outdated
(horizontal and vertical) from the element‘s box | ||
Animation type: by computed value, | ||
letting ''box-shadow-offset/none'' create a blank shadow | ||
(''transparent 0 0 0 0'') when interpolated with non-'none' 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.
I don't think that works, the computed value of a property can affect another, but if the computed value at the midpoint between none
and 10px 10px
is 5px 5px
, then 5px 5px
shouldn't by itself force box-shadow-color
to get a transparent
.
As I said, I think handling transparent
in the shorthand expansion will suffice to keep the old behavior.
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.
Ok, I've moved the handling of transparent
to the shorthand expansion now.
css-borders-4/Overview.bs
Outdated
Animation type: by computed value, | ||
letting ''box-shadow-offset/none'' create a blank shadow | ||
(''transparent 0 0 0 0'') when interpolated with non-'none' values | ||
and appending blank shadows with a corresponding |
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 think this should actually be handled in each longhand, i.e. the animation type of box-shadow-offset
should just add none
values, box-shadow-blur
add 0
, etc.
css-borders-4/Overview.bs
Outdated
No box shadow will be created. | ||
Any other box shadow properties specified for this box shadow have no | ||
effect. |
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.
Maybe make it clearer that this only refers to the current shadow? That is, if none
is mixed with other values, the others can still create shadows.
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 tried to clarify now that none
only affects the current shadow.
…action with `box-shadow-color`
While I was at it, I also removed "either the |
(horizontal and vertical) from the element‘s box | ||
Animation type: by computed value, | ||
treating ''box-shadow-offset/none'' as ''0 0'' | ||
when interpolated with non-'none' 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.
This needs to say that if the lists have different lengths, none
values are added to the short list until the have the same length.
Something analogous for the other longhands. Possibly consider a new animation type like "defaultable list" or something, since there already is a "repeatable list".
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.
css-borders-4/Overview.bs
Outdated
No box shadow will be created. | ||
Any other box shadow properties specified for that shadow in the list | ||
of box shadows have no effect. |
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.
Still seems confusing. Maybe something like
No box shadow will be created. | |
Any other box shadow properties specified for that shadow in the list | |
of box shadows have no effect. | |
The shadow will not be rendered. | |
The values of other box shadow properties corresponding to this shadow have no effect. |
Probably @fantasai can come up with something better.
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.
Note that I aligned that with the none
value for animation-name
.
And I believe it's clear enough that this means that other shadows in the list won't be affected by that.
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.
Ok, added your phrashing, though both seem equally good to me.
css-borders-4/Overview.bs
Outdated
If the offset is ''box-shadow-offset/none'', | ||
''box-shadow-color'' is treated as ''transparent''. |
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.
"Treated as" sounds like changing the used value of box-shadow-color
, which can be a possible approach, but the shorthand wouldn't be the correct place for it, and it would need more complication than that.
What I was thinking (maybe it's not the best behavior, but it's simple to specify for now, better approaches can be considered in a follow-up) is that the specified value of box-shadow-color
would be transparent
, so no "treating as" necessary.
Don't remove the default length either.
If the offset is ''box-shadow-offset/none'', | |
''box-shadow-color'' is treated as ''transparent''. | |
Omitted lengths are 0; | |
omitted colors default to the ''transparent'' value when the specified offset is ''none'' | |
and to ''currentcolor'' otherwise. |
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.
Pushed this change with a slight adjustment.
…pansion of `box-shadow` shorthand
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 guess this is fine as long as you address interpolation of lists with different lengths in a follow-up.
I also plan to file an issue to discuss other possible ways of affecting the color when interpolating from a none
offset to something else.
I've added the definition for the
none
value ofbox-shadow-offset
and corrected the computed value of that property to account for thenone
value now being part of the shadow list.The
box-shadow
shorthand was adjusted accordingly.Fixes #8567
Sebastian