Skip to content

[css-backgrounds-4] Added border-*-radius shorthands #7705

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

Merged
merged 5 commits into from
Feb 17, 2023

Conversation

SebastianZ
Copy link
Contributor

Defined the page-relative (physical) shorthand properties border-top-radius, border-right-radius, border-bottom-radius, and border-left-radius and the flow-relative (logical) shorthand properties border-block-start-radius, border-block-end-radius, border-inline-start-radius, border-inline-end-radius.

The page-relative shorthands were initially suggested ten years ago. And the flow-relative ones are a logical (note the double sense 😃) complement for them.

Note that I explicitly defined the two radii for the corners to be in physical order even for the logical properties. This was not explicitly mentioned in the definition of the longhands in CSS Logical Properties and Values 1 but implictily given by referring the value to <'border-top-left-radius'>. And this is also what user agents currently implement.
So I thought it would be better to clarify this.

@LeaVerou I assigned you as reviewer because it was your suggestion back then.

Sebastian

Defined the page-relative shorthand properties `border-top-radius`, `border-right-radius`, `border-bottom-radius`, and `border-left-radius` and the flow-relative shorthand properties `border-block-start-radius`, `border-block-end-radius`, `border-inline-start-radius`, `border-inline-end-radius`.
Copy link
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

LGTM, though I'd ask @fantasai for review as well.

@LeaVerou LeaVerou requested a review from fantasai September 7, 2022 10:42
Corner Sizing: the 'border-radius' and 'border-*-radius' shorthand properties</h3>

<h4 id="page-relative-corner-sizing">
Page-Relative (Physical) Corner Sizing:
Copy link
Member

Choose a reason for hiding this comment

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

I would expect "page-relative" to mean inside (binding) versus outside edges of a printed page (i.e., inside would be the left side for a recto page in an ltr book, etc. We have had proposals for such logical properties, but I think they've been removed from current drafts.

I think its probably best to just use "Physical" and not "Page-Relative".

Also, in both this h4 title and the next, I think you should mention these are specifically side shorthands... since there are longhand properties for these things elsewhere in the specification.

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 would expect "page-relative" to mean inside (binding) versus outside edges of a printed page (i.e., inside would be the left side for a recto page in an ltr book, etc. We have had proposals for such logical properties, but I think they've been removed from current drafts.

I think its probably best to just use "Physical" and not "Page-Relative".

Note that I took "Page-Relative" from CSS Box 3/4, where it refers to the physical properties to distinguish them from their logical counterparts. Though you're right, "physical" is less ambiguous and also much more common throughout the specs. So I replaced "Page-Relative" by "Physical".

We may change that terminology in CSS Box as well to bring them in line with the rest of the specs.

Also, in both this h4 title and the next, I think you should mention these are specifically side shorthands... since there are longhand properties for these things elsewhere in the specification.

Good point. I've added it.

I also realized that the second h4 actually included the wrong property names. 😊 So I've also fixed that now.

…ned that new `border-*-radius` properties are _side_ shorthands
Comment on lines 292 to 294
<p>These properties correspond to the 'border-top-radius',
'border-right-radius', 'border-bottom-radius', and 'border-left-radius'
shorthand properties.
Copy link
Contributor

Choose a reason for hiding this comment

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

This implies that with writing-mode: horizontal-tb; direction: rtl,

border-block-start-radius: 1px 2px;

should become

border-top-radius: 1px 2px;

i.e.

border-top-left-radius: 1px;
border-top-right-radius: 2px;

Instead, I think that

border-block-start-radius: 1px 2px;

should expand to

border-start-start-radius: 1px;
border-start-end-radius: 2px;

which then will map to

border-top-right-radius: 1px;
border-top-left-radius: 2px;

This is because shorthands must be expanded into longhands at parse time, while logical-to-physical mapping happens at computed-value time. So we can't have logical shorthands mapping into physical shorthands. Shorthands just expand, it's the longhands that can have logical/physical pairings.

The two values for the radii are given in the order
top-left, top-right for 'border-top-radius',
top-right, bottom-right for 'border-right-radius'
bottom-left, bottom-right for 'border-bottom-radius',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this, would it be better to follow the same order as in border-radius?

So border-radius: 1px 2px 3px 4px would be equivalent to

border-top-radius: 1px 2px;
border-bottom-radius: 3px 4px;

instead of

border-top-radius: 1px 2px;
border-bottom-radius: 4px 3px;

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 believe it makes more sense to keep them from top to bottom and left to right, especially in regard of their logical counterparts. E.g. in horizontal writing modes you probably would not want the values switch their order depending on whether they are left-to-right or right-to-left.

Though I created an issue in the spec. so we can discuss this separately.

…o longhands

Obviously, shorthands must be expanded into longhands at parse time, while logical-to-physical mapping happens at computed-value time. So we can't have logical shorthands mapping into physical shorthands.
Comment on lines 292 to 299
<p>These properties correspond to the 'border-start-start-radius',
'border-start-end-radius', 'border-end-start-radius',
and 'border-end-end-radius' longhand properties.
The start/end refers to the axis given by the first part of the name
(i.e. 'border-start-<i>inline</i>-radius' for 'border-block-start-radius`,
'border-end-<i>inline</i>-radius' for 'border-block-end-radius',
'border-<i>block</i>-start-radius' for 'border-inline-start-radius', and
'border-<i>block</i>-end-radius' for 'border-inline-end-radius')
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 understand what the paragraph is trying to say.
Also, border-start-inline-radius and border-end-inline-radius don't exist, do you mean border-inline-start-radius and border-inline-end-radius?

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 basically copied the wording from https://drafts.csswg.org/css-logical-1/#border-radius-properties. There, the italic written parts are meant to clarify the logical direction and stand for start and end.

The sentence is meant to say that border-block-start-radius is the shorthand for border-start-start-radius and border-end-start-radius, border-block-end-radius the shorthand for border-end-start-radius and border-end-end-radius, border-inline-start-radius the shorthand for border-start-start-radius and border-end-start-radius, and border-inline-end-radius the shorthand for border-start-end-radius and border-end-end-radius.
Though that is obviously even longer and quite complicated.
So maybe there's an easier way to express that?

(Also, the order of the longhands is meant to follow the ones of the physical shorthands, so it is always start first then end.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The start/end refers to the axis given by the first part of the name

To give an example, the inline-end in border-inline-end-radius means the end radii in the inline axis, so border-start-end-radius and border-end-end-radius.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it confusing because the wording from https://drafts.csswg.org/css-logical-1/#border-radius-properties explains the mapping of logical longhands into physical longhands, but here it's trying to explain how some shorthands expand into longhands.

I would rather copy the wording from the physical shorthands

The physical 'border-*-radius' shorthands set the two physical 'border-*-*-radius'
longhand properties of the related physical side. [...]
The two values for the radii are given in the order
top-left, top-right for 'border-top-radius',
top-right, bottom-right for 'border-right-radius'
bottom-left, bottom-right for 'border-bottom-radius',
and top-left, bottom-left for 'border-left-radius'.

The logical 'border-*-radius' shorthands set the two logical 'border-*-*-radius'
longhand properties of the related logical side. [...]
The two values for the radii are given in the order
start-start, start-end for 'border-block-start-radius',
end-start, end-end for 'border-block-end-radius'
start-start, end-start for 'border-inline-start-radius',
and start-end, end-end for 'border-inline-end-radius'.

Or maybe merge the sections and define all the shorthands together.

The 'border-*-radius' shorthands set the two 'border-*-*-radius'
longhand properties of the related side. [...]
The two values for the radii are given in the order
top-left, top-right for 'border-top-radius',
top-right, bottom-right for 'border-right-radius'
bottom-left, bottom-right for 'border-bottom-radius',
top-left, bottom-left for 'border-left-radius',
start-start, start-end for 'border-block-start-radius',
end-start, end-end for 'border-block-end-radius'
start-start, end-start for 'border-inline-start-radius',
and start-end, end-end for 'border-inline-end-radius'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. I've merged the two sections now.

@SebastianZ SebastianZ requested a review from Loirooriol October 9, 2022 18:41
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'm not sure if the expansion order is the best, or if these shorthands will actually be useful in practice. But at least the definition makes sense now

@SebastianZ
Copy link
Contributor Author

Thanks for your approval, @Loirooriol!

I'm not sure if the expansion order is the best, or if these shorthands will actually be useful in practice.

I have already added an issue to the spec. and will create a GitHub one once the PR is merged.
There is a long-standing demand for those shorthands from the community and a lot of use cases, so I am confident that they will be use useful.

@dbaron It would be fantanstic if you could also give this another review, so we can finally land this.

Sebastian

@SebastianZ
Copy link
Contributor Author

@fantasai Would you mind reviewing this change, please?

Sebastian

@SebastianZ
Copy link
Contributor Author

@fantasai A friendly reminder to review this PR.

Sebastian

@SebastianZ
Copy link
Contributor Author

SebastianZ commented Feb 17, 2023

As there were previously already two approvals for this PR and there was no further feedback within four months, I go ahead and merge it. We can discuss further adjustments in separate issues.

Sebastian

@SebastianZ SebastianZ merged commit 72189dc into w3c:main Feb 17, 2023
@cdoublev
Copy link
Collaborator

cdoublev commented Feb 20, 2023

there was no further feedback within four months [...]. We can discuss further adjustments in separate issues.

Sorry, I only became aware of this PR by updating w3c/webref.

I do not personally need border-block-radius and border-inline-radius (edit: or border-start-radius and border-end-radius? there must be a good reason I do no know for repeating start|end in logical sub-properties, instead of using block|inline) but I feel like they are missing since border-block and border-inline exist.

I did not create a separate issue as it seems to be more about fixing a minor oversight than discussing about an adjustement.

@Loirooriol
Copy link
Contributor

@cdoublev border-start-start-radius refers to the block-start inline-start corner. border-block-radius and border-inline-radius don't exist just like we don't have border-horizontal-radius nor border-vertical-radius, and it's not clear what that would mean, since all corners have an horizontal and a vertical component.

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.

5 participants