8000 [css-borders-4] Fixed and added definitions for box-shadow-offset: none by SebastianZ · Pull Request #9096 · w3c/csswg-drafts · GitHub
[go: up one dir, main page]

Skip to content

Conversation

SebastianZ
Copy link
Contributor

I've added the definition for the none value of box-shadow-offset and corrected the computed value of that property to account for the none value now being part of the shadow list.
The box-shadow shorthand was adjusted accordingly.

Fixes #8567

Sebastian

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,
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about now?

@SebastianZ SebastianZ requested a review from Loirooriol July 26, 2023 11:00
(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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

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
Copy link
Contributor

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.

Comment on lines 639 to 641
No box shadow will be created.
Any other box shadow properties specified for this box shadow have no
effect.
Copy link
Contributor

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.

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 tried to clarify now that none only affects the current shadow.

@SebastianZ
Copy link
Contributor Author

While I was at it, I also removed "either the none value, which indicates no shadows, or a comma-separated list of shadows, " from the box-shadow shorthand as it isn't correct anymore.

@SebastianZ SebastianZ requested a review from Loirooriol July 27, 2023 20:59
@SebastianZ SebastianZ changed the title [css-backgrounds-4] Fixed and added definitions for box-shadow-offset: none [css-borders-4] Fixed and added definitions for box-shadow-offset: none Jul 27, 2023
(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.
Copy link
Contributor

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".

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 meant to add the everything related to the coordinating list property group in #8613.

I was also thinking about the animation type, as "repeatable list" doesn't fit here. @fantasai Any ideas?

Comment on lines 636 to 638
No box shadow will be created.
Any other box shadow properties specified for that shadow in the list
of box shadows have no effect.
Copy link
Contributor

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

Suggested change
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.

Copy link
Contributor Author

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.

Copy link
Contributor< 8000 /span> Author

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.

Comment on lines 765 to 766
If the offset is ''box-shadow-offset/none'',
''box-shadow-color'' is treated as ''transparent''.
Copy link
Contributor

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.

Suggested change
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.

Copy link
Contributor Author

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.

@SebastianZ SebastianZ requested a review from Loirooriol August 1, 2023 20:52
Copy link
Contributor
@Loirooriol Loirooriol left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[css-backgrounds-4] Allow repeating none in box-shadow-offset
2 participants
0