Bug 218093

Summary: [css-logical] Implement logical border-radius
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: clopez, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jfernandez, joepeck, macpherson, menard, obrufau, rego, sam, simon.fraser, webkit-bug-importer, youennf, zsun
Priority: P2 Keywords: InRadar, WebExposed
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=1155270
Bug Depends on:    
Bug Blocks: 185977    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch none

Comment 1 Radar WebKit Bug Importer 2020-10-22 10:46:54 PDT
<rdar://problem/70579854>
Comment 2 zsun 2020-12-15 04:04:28 PST
Created attachment 416234 [details]
Patch
Comment 3 Javier Fernandez 2020-12-15 04:40:58 PST
Comment on attachment 416234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416234&action=review

> Source/WebCore/platform/text/WritingMode.h:193
> +            if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))

Isn't this equivalent to !isFlippedBlockWritingMode() (aka: vertical-rl) ?

> Source/WebCore/platform/text/WritingMode.h:210
> +        if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))

Ditto

> Source/WebCore/platform/text/WritingMode.h:221
> +        if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))

Ditto

> Source/WebCore/platform/text/WritingMode.h:226
> +            if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))

Ditto
Comment 4 Manuel Rego Casasnovas 2020-12-15 07:20:06 PST
Comment on attachment 416234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416234&action=review

You need to update some test expectations to include the new propoerties.

> Source/WebCore/ChangeLog:7
> +

Please add a longer description here, explaining which properties are being added as part of this patch.
Comment 5 Oriol Brufau 2020-12-15 07:47:18 PST
Comment on attachment 416234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416234&action=review

> Source/WebCore/platform/text/WritingMode.h:132
> +enum class LogicalRadiusCorner : uint8_t {

In the future there might be more properties referencing corners, other than border radius.
So it may be better to name the enums like LogicalBoxCorner and BoxCorner.
Seems I missed this in the Chromium review...
Comment 6 Simon Fraser (smfr) 2020-12-15 09:49:39 PST
Comment on attachment 416234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416234&action=review

> Source/WebCore/css/CSSProperty.cpp:175
> +    case CSSPropertyBorderStartStartRadius: {
> +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartStart, borderRadiusShorthand());
> +    }
> +    case CSSPropertyBorderStartEndRadius: {
> +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartEnd, borderRadiusShorthand());
> +    }
> +    case CSSPropertyBorderEndStartRadius: {
> +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndStart, borderRadiusShorthand());
> +    }
> +    case CSSPropertyBorderEndEndRadius: {
> +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndEnd, borderRadiusShorthand());
> +    }

No need for braces on these.

>> Source/WebCore/platform/text/WritingMode.h:132
>> +enum class LogicalRadiusCorner : uint8_t {
> 
> In the future there might be more properties referencing corners, other than border radius.
> So it may be better to name the enums like LogicalBoxCorner and BoxCorner.
> Seems I missed this in the Chromium review...

I concur. See BoxSide and LogicalBoxSide.

> Source/WebCore/platform/text/WritingMode.h:187
> +inline RadiusCorner mapLogicalRadiusCornerToPhysicalRadiusCorner(TextFlow textflow, WritingMode writingMode, LogicalRadiusCorner logicalRadiusCorner)

This can be called mapLogicalCornerToPhysicalCorner()
Comment 7 zsun 2020-12-16 03:45:01 PST
Created attachment 416325 [details]
Patch
Comment 8 zsun 2020-12-16 04:08:51 PST
(In reply to Javier Fernandez from comment #3)
> Comment on attachment 416234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416234&action=review
> 
> > Source/WebCore/platform/text/WritingMode.h:193
> > +            if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))
> 
> Isn't this equivalent to !isFlippedBlockWritingMode() (aka: vertical-rl) ?
> 
> > Source/WebCore/platform/text/WritingMode.h:210
> > +        if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))
> 
> Ditto
> 
> > Source/WebCore/platform/text/WritingMode.h:221
> > +        if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))
> 
> Ditto
> 
> > Source/WebCore/platform/text/WritingMode.h:226
> > +            if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))
> 
> Ditto

Yes, it might be worthy introducing isFlippedBlockWritingMode() in WritingMode.h. As we discussed, I probably will leave this to a separate patch.
Comment 9 zsun 2020-12-16 04:09:28 PST
(In reply to Manuel Rego Casasnovas from comment #4)
> Comment on attachment 416234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416234&action=review
> 
> You need to update some test expectations to include the new propoerties.
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> Please add a longer description here, explaining which properties are being
> added as part of this patch.

Done
Comment 10 zsun 2020-12-16 04:10:08 PST
(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 416234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416234&action=review
> 
> > Source/WebCore/css/CSSProperty.cpp:175
> > +    case CSSPropertyBorderStartStartRadius: {
> > +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartStart, borderRadiusShorthand());
> > +    }
> > +    case CSSPropertyBorderStartEndRadius: {
> > +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartEnd, borderRadiusShorthand());
> > +    }
> > +    case CSSPropertyBorderEndStartRadius: {
> > +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndStart, borderRadiusShorthand());
> > +    }
> > +    case CSSPropertyBorderEndEndRadius: {
> > +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndEnd, borderRadiusShorthand());
> > +    }
> 
> No need for braces on these.
> 
> >> Source/WebCore/platform/text/WritingMode.h:132
> >> +enum class LogicalRadiusCorner : uint8_t {
> > 
> > In the future there might be more properties referencing corners, other than border radius.
> > So it may be better to name the enums like LogicalBoxCorner and BoxCorner.
> > Seems I missed this in the Chromium review...
> 
> I concur. See BoxSide and LogicalBoxSide.
> 
> > Source/WebCore/platform/text/WritingMode.h:187
> > +inline RadiusCorner mapLogicalRadiusCornerToPhysicalRadiusCorner(TextFlow textflow, WritingMode writingMode, LogicalRadiusCorner logicalRadiusCorner)
> 
> This can be called mapLogicalCornerToPhysicalCorner()

Code updated. Thank you!
Comment 11 zsun 2020-12-16 04:10:32 PST
(In reply to Oriol Brufau from comment #5)
> Comment on attachment 416234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416234&action=review
> 
> > Source/WebCore/platform/text/WritingMode.h:132
> > +enum class LogicalRadiusCorner : uint8_t {
> 
> In the future there might be more properties referencing corners, other than
> border radius.
> So it may be better to name the enums like LogicalBoxCorner and BoxCorner.
> Seems I missed this in the Chromium review...

Updated. Thank you!
Comment 12 zsun 2020-12-16 04:15:05 PST
Created attachment 416327 [details]
Patch
Comment 13 zsun 2020-12-16 04:50:33 PST
(In reply to zsun from comment #10)
> (In reply to Simon Fraser (smfr) from comment #6)
> > Comment on attachment 416234 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=416234&action=review
> > 
> > > Source/WebCore/css/CSSProperty.cpp:175
> > > +    case CSSPropertyBorderStartStartRadius: {
> > > +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartStart, borderRadiusShorthand());
> > > +    }
> > > +    case CSSPropertyBorderStartEndRadius: {
> > > +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartEnd, borderRadiusShorthand());
> > > +    }
> > > +    case CSSPropertyBorderEndStartRadius: {
> > > +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndStart, borderRadiusShorthand());
> > > +    }
> > > +    case CSSPropertyBorderEndEndRadius: {
> > > +        return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndEnd, borderRadiusShorthand());
> > > +    }
> > 
> > No need for braces on these.
> > 
> > >> Source/WebCore/platform/text/WritingMode.h:132
> > >> +enum class LogicalRadiusCorner : uint8_t {
> > > 
> > > In the future there might be more properties referencing corners, other than border radius.
> > > So it may be better to name the enums like LogicalBoxCorner and BoxCorner.
> > > Seems I missed this in the Chromium review...
I will try to update the Chromium changes.
> > 
> > I concur. See BoxSide and LogicalBoxSide.
> > 
> > > Source/WebCore/platform/text/WritingMode.h:187
> > > +inline RadiusCorner mapLogicalRadiusCornerToPhysicalRadiusCorner(TextFlow textflow, WritingMode writingMode, LogicalRadiusCorner logicalRadiusCorner)
> > 
> > This can be called mapLogicalCornerToPhysicalCorner()
> 
> Code updated. Thank you!
Comment 14 zsun 2020-12-16 05:31:39 PST
Created attachment 416330 [details]
Patch
Comment 15 zsun 2020-12-16 06:33:57 PST
Created attachment 416335 [details]
Patch
Comment 16 Simon Fraser (smfr) 2020-12-16 09:16:26 PST
Comment on attachment 416335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416335&action=review

> Source/WebCore/css/CSSProperties.json:1516
> +        "border-corner-radius": {

I don't see this property in https://www.w3.org/TR/css-logical/#border-radius-shorthands anywhere.
Comment 17 zsun 2020-12-16 11:27:42 PST
(In reply to Simon Fraser (smfr) from comment #16)
> Comment on attachment 416335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416335&action=review
> 
> > Source/WebCore/css/CSSProperties.json:1516
> > +        "border-corner-radius": {
> 
> I don't see this property in
> https://www.w3.org/TR/css-logical/#border-radius-shorthands anywhere.

You are right. It should be "border-radius" but "border-radius" is already used in this file to refer to physical radius corners. 

Any suggestion on naming this would be appreciated.
Comment 18 Oriol Brufau 2020-12-16 13:34:37 PST
Comment on attachment 416335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416335&action=review

>>> Source/WebCore/css/CSSProperties.json:1516
>>> +        "border-corner-radius": {
>> 
>> I don't see this property in https://www.w3.org/TR/css-logical/#border-radius-shorthands anywhere.
> 
> You are right. It should be "border-radius" but "border-radius" is already used in this file to refer to physical radius corners. 
> 
> Any suggestion on naming this would be appreciated.

Ziran, you seem to think that you need a shorthand for the logical border radius properties. Why?
Isn't it possible to just remove this "border-corner-radius"?
Your patch for Chromium didn't have it.
Comment 19 zsun 2020-12-17 02:21:49 PST
Created attachment 416404 [details]
Patch
Comment 20 zsun 2020-12-17 02:24:23 PST
(In reply to Oriol Brufau from comment #18)
> Comment on attachment 416335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416335&action=review
> 
> >>> Source/WebCore/css/CSSProperties.json:1516
> >>> +        "border-corner-radius": {
> >> 
> >> I don't see this property in https://www.w3.org/TR/css-logical/#border-radius-shorthands anywhere.
> > 
> > You are right. It should be "border-radius" but "border-radius" is already used in this file to refer to physical radius corners. 
> > 
> > Any suggestion on naming this would be appreciated.
> 
> Ziran, you seem to think that you need a shorthand for the logical border
> radius properties. Why?
> Isn't it possible to just remove this "border-corner-radius"?
> Your patch for Chromium didn't have it.

Probably shouldn't be. Patch updated with it removed. Thank you!
Comment 21 zsun 2020-12-17 03:17:51 PST
Created attachment 416409 [details]
Patch
Comment 22 zsun 2020-12-17 03:21:22 PST
(In reply to zsun from comment #8)
> (In reply to Javier Fernandez from comment #3)
> > Comment on attachment 416234 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=416234&action=review
> > 
> > > Source/WebCore/platform/text/WritingMode.h:193
> > > +            if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))
> > 
> > Isn't this equivalent to !isFlippedBlockWritingMode() (aka: vertical-rl) ?
> > 
> > > Source/WebCore/platform/text/WritingMode.h:210
> > > +        if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))
> > 
> > Ditto
> > 
> > > Source/WebCore/platform/text/WritingMode.h:221
> > > +        if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))
> > 
> > Ditto
> > 
> > > Source/WebCore/platform/text/WritingMode.h:226
> > > +            if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode))
> > 
> > Ditto
> 
> Yes, it might be worthy introducing isFlippedBlockWritingMode() in
> WritingMode.h. As we discussed, I probably will leave this to a separate
> patch.

isFlippedWritingMode(WritingMode writingMode) has been define 
in WritingMode.h. I guess probably it's enough without introducing isFlippedBlockWritingMode(). 

Patch has been updated. Thank you!
Comment 23 Oriol Brufau 2020-12-17 09:10:24 PST
Comment on attachment 416409 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416409&action=review

> Source/WebCore/platform/text/WritingMode.h:194
> +            return BoxCorner::BottomLeft;                

There are some trailing spaces here

> Source/WebCore/platform/text/WritingMode.h:206
> +        return BoxCorner::TopRight; 

Ditto

> Source/WebCore/platform/text/WritingMode.h:216
> +            return BoxCorner::BottomRight;                

Ditto

> Source/WebCore/platform/text/WritingMode.h:227
> +            return BoxCorner::TopRight;                

Ditto
Comment 24 zsun 2020-12-17 09:50:11 PST
Created attachment 416432 [details]
Patch
Comment 25 zsun 2020-12-17 09:53:42 PST
(In reply to Oriol Brufau from comment #23)
> Comment on attachment 416409 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416409&action=review
> 
> > Source/WebCore/platform/text/WritingMode.h:194
> > +            return BoxCorner::BottomLeft;                
> 
> There are some trailing spaces here
> 
> > Source/WebCore/platform/text/WritingMode.h:206
> > +        return BoxCorner::TopRight; 
> 
> Ditto
> 
> > Source/WebCore/platform/text/WritingMode.h:216
> > +            return BoxCorner::BottomRight;                
> 
> Ditto
> 
> > Source/WebCore/platform/text/WritingMode.h:227
> > +            return BoxCorner::TopRight;                
> 
> Ditto

Updated. Thank you!
Comment 26 Oriol Brufau 2020-12-17 09:58:07 PST
Comment on attachment 416432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416432&action=review

> Source/WebCore/platform/text/WritingMode.h:181
> +inline BoxCorner mapLogicalCornerToPhysicalCorner(TextFlow textflow, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner)

Webkit has the non-standard '-webkit-writing-mode: horizontal-bt'. The mapping doesn't seem correct for that value.
For example, with horizontal-bt ltr, the block-start side is the bottom, and the inline-start side is the left.
But the start-start corner resolves to top-right instead of bottom-left.
Comment 27 Simon Fraser (smfr) 2020-12-17 10:58:00 PST
Comment on attachment 416432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416432&action=review

> Source/WebCore/platform/text/WritingMode.h:126
> +enum class LogicalBoxCorner : uint8_t {

Maybe a comment here to describe the order (block/inline or inline/block?)
Comment 28 Simon Fraser (smfr) 2020-12-17 11:01:19 PST
Comment on attachment 416432 [details]
Patch

r- based on Oriol's comment. Please be sure to add a test for -webkit-writing-mode: horizontal-bt
Comment 29 zsun 2020-12-18 07:03:44 PST
Created attachment 416517 [details]
Patch
Comment 30 EWS Watchlist 2020-12-18 07:05:15 PST
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 31 zsun 2020-12-18 07:15:40 PST
Created attachment 416519 [details]
Patch
Comment 32 zsun 2020-12-18 07:17:49 PST
(In reply to Simon Fraser (smfr) from comment #27)
> Comment on attachment 416432 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416432&action=review
> 
> > Source/WebCore/platform/text/WritingMode.h:126
> > +enum class LogicalBoxCorner : uint8_t {
> 
> Maybe a comment here to describe the order (block/inline or inline/block?)

Updated.
Comment 33 zsun 2020-12-18 07:21:28 PST
(In reply to Oriol Brufau from comment #26)
> Comment on attachment 416432 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416432&action=review
> 
> > Source/WebCore/platform/text/WritingMode.h:181
> > +inline BoxCorner mapLogicalCornerToPhysicalCorner(TextFlow textflow, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner)
> 
> Webkit has the non-standard '-webkit-writing-mode: horizontal-bt'. The
> mapping doesn't seem correct for that value.
> For example, with horizontal-bt ltr, the block-start side is the bottom, and
> the inline-start side is the left.
> But the start-start corner resolves to top-right instead of bottom-left.

Updated. It has modifies the imported WPT tests. I'm not sure if I should port it back.
Comment 34 zsun 2020-12-18 10:25:30 PST
The changes on tests related to horizontal-bt will be moved to a different patch. The following bug has been raised to address it:

https://bugs.webkit.org/show_bug.cgi?id=220015
Comment 35 Oriol Brufau 2020-12-20 07:14:23 PST
Comment on attachment 416519 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416519&action=review

> Source/WebCore/platform/text/WritingMode.h:183
> +inline BoxCorner mapLogicalCornerToPhysicalCorner(TextFlow textflow, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner)

I guess this function should be marked constexpr, like the others.

And it seems strange to pass both TextFlow and WritingMode as arguments.
I would pass the TextFlow only, though you may need a new isFlippedLinesTextFlow function.

> Source/WebCore/platform/text/WritingMode.h:185
> +    BoxCorner physicalCorner = static_cast<BoxCorner>(logicalBoxCorner);

physicalCorner doesn't seem necessary.

> Source/WebCore/platform/text/WritingMode.h:186
> +    switch (logicalBoxCorner) {

This switch seems very long. I think you can simplify the cases as such:

    switch (logicalBoxCorner) {
    case LogicalBoxCorner::StartStart:
        if (isFlippedTextFlow(textflow)) {
            if (isReversedTextFlow(textflow))
                return BoxCorner::BottomRight;
        } else if (!isReversedTextFlow(textflow)) {
           return BoxCorner::TopLeft;
        }
        if (isFlippedLinesTextFlow(textflow))
            return BoxCorner::BottomLeft;
        return BoxCorner::TopRight;
    case LogicalBoxCorner::StartEnd:
        if (isFlippedTextFlow(textflow)) {
            if (!isReversedTextFlow(textflow))
                return BoxCorner::BottomRight;
        } else if (isReversedTextFlow(textflow)) {
            return BoxCorner::TopLeft;
        }
        if (isFlippedLinesTextFlow(textflow))
            return BoxCorner::BottomLeft;
        return BoxCorner::TopRight;
    case LogicalBoxCorner::EndStart:
        if (isFlippedTextFlow(textflow)) {
            if (!isReversedTextFlow(textflow))
                return BoxCorner::TopLeft;
        } else if (isReversedTextFlow(textflow)) {
            return BoxCorner::BottomRight;
        }
        if (isFlippedLinesTextFlow(textflow))
            return BoxCorner::TopRight;
        return BoxCorner::BottomLeft;
    case LogicalBoxCorner::EndEnd:
        if (isFlippedTextFlow(textflow)) {
            if (isReversedTextFlow(textflow))
                return BoxCorner::TopLeft;
        } else if (!isReversedTextFlow(textflow)) {
           return BoxCorner::BottomRight;
        }
        if (isFlippedLinesTextFlow(textflow))
            return BoxCorner::TopRight;
        return BoxCorner::BottomLeft;
    }


Or even remove the switch and use

    bool isBlockStart = logicalBoxCorner == LogicalBoxCorner::StartStart || logicalBoxCorner == LogicalBoxCorner::StartEnd;
    bool isInlineStart = logicalBoxCorner == LogicalBoxCorner::StartStart || logicalBoxCorner == LogicalBoxCorner::EndStart;
    if (isBlockStart == isFlippedTextFlow(textflow)) {
        if (isInlineStart == isReversedTextFlow(textflow))
            return BoxCorner::BottomRight;
    } else if (isInlineStart != isReversedTextFlow(textflow)) {
       return BoxCorner::TopLeft;
    }
    if (isBlockStart == isFlippedLinesTextFlow(textflow))
        return BoxCorner::BottomLeft;
    return BoxCorner::TopRight;
Comment 36 zsun 2020-12-22 09:11:46 PST
Created attachment 416669 [details]
Patch
Comment 37 zsun 2020-12-22 09:12:25 PST
(In reply to Oriol Brufau from comment #35)
> Comment on attachment 416519 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416519&action=review
> 
> > Source/WebCore/platform/text/WritingMode.h:183
> > +inline BoxCorner mapLogicalCornerToPhysicalCorner(TextFlow textflow, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner)
> 
> I guess this function should be marked constexpr, like the others.
> 
> And it seems strange to pass both TextFlow and WritingMode as arguments.
> I would pass the TextFlow only, though you may need a new
> isFlippedLinesTextFlow function.
> 
> > Source/WebCore/platform/text/WritingMode.h:185
> > +    BoxCorner physicalCorner = static_cast<BoxCorner>(logicalBoxCorner);
> 
> physicalCorner doesn't seem necessary.
> 
> > Source/WebCore/platform/text/WritingMode.h:186
> > +    switch (logicalBoxCorner) {
> 
> This switch seems very long. I think you can simplify the cases as such:
> 
>     switch (logicalBoxCorner) {
>     case LogicalBoxCorner::StartStart:
>         if (isFlippedTextFlow(textflow)) {
>             if (isReversedTextFlow(textflow))
>                 return BoxCorner::BottomRight;
>         } else if (!isReversedTextFlow(textflow)) {
>            return BoxCorner::TopLeft;
>         }
>         if (isFlippedLinesTextFlow(textflow))
>             return BoxCorner::BottomLeft;
>         return BoxCorner::TopRight;
>     case LogicalBoxCorner::StartEnd:
>         if (isFlippedTextFlow(textflow)) {
>             if (!isReversedTextFlow(textflow))
>                 return BoxCorner::BottomRight;
>         } else if (isReversedTextFlow(textflow)) {
>             return BoxCorner::TopLeft;
>         }
>         if (isFlippedLinesTextFlow(textflow))
>             return BoxCorner::BottomLeft;
>         return BoxCorner::TopRight;
>     case LogicalBoxCorner::EndStart:
>         if (isFlippedTextFlow(textflow)) {
>             if (!isReversedTextFlow(textflow))
>                 return BoxCorner::TopLeft;
>         } else if (isReversedTextFlow(textflow)) {
>             return BoxCorner::BottomRight;
>         }
>         if (isFlippedLinesTextFlow(textflow))
>             return BoxCorner::TopRight;
>         return BoxCorner::BottomLeft;
>     case LogicalBoxCorner::EndEnd:
>         if (isFlippedTextFlow(textflow)) {
>             if (isReversedTextFlow(textflow))
>                 return BoxCorner::TopLeft;
>         } else if (!isReversedTextFlow(textflow)) {
>            return BoxCorner::BottomRight;
>         }
>         if (isFlippedLinesTextFlow(textflow))
>             return BoxCorner::TopRight;
>         return BoxCorner::BottomLeft;
>     }
> 
> 
> Or even remove the switch and use
> 
>     bool isBlockStart = logicalBoxCorner == LogicalBoxCorner::StartStart ||
> logicalBoxCorner == LogicalBoxCorner::StartEnd;
>     bool isInlineStart = logicalBoxCorner == LogicalBoxCorner::StartStart ||
> logicalBoxCorner == LogicalBoxCorner::EndStart;
>     if (isBlockStart == isFlippedTextFlow(textflow)) {
>         if (isInlineStart == isReversedTextFlow(textflow))
>             return BoxCorner::BottomRight;
>     } else if (isInlineStart != isReversedTextFlow(textflow)) {
>        return BoxCorner::TopLeft;
>     }
>     if (isBlockStart == isFlippedLinesTextFlow(textflow))
>         return BoxCorner::BottomLeft;
>     return BoxCorner::TopRight;

Code updated. Thank you!
Comment 38 Sam Weinig 2020-12-25 18:51:23 PST
Comment on attachment 416669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416669&action=review

Thanks for working on this!

> Source/WebCore/ChangeLog:37
> +        (WebCore::mapLogicalCornerToPhysicalCorner):
>  2020-12-22  Simon Fraser  <simon.fraser@apple.com>

Please leave a newline at the end here.

> Source/WebCore/ChangeLog:70
> -        (This saves a loop on the inline level boxes while constructing the lines at InlineContentBuilder::createDisplayLines) 
> +        (This saves a loop on the inline level boxes while constructing the lines at InlineContentBuilder::createDisplayLines)

Hey, this was probably just your editor going wild, but please leave the whitespace in the rest of the ChangeLog (or really anything you're not actively changing)  alone. It isn't necessary to change.
Comment 39 zsun 2021-01-05 07:07:52 PST
Created attachment 417001 [details]
Patch
Comment 40 zsun 2021-01-05 07:09:48 PST
(In reply to Sam Weinig from comment #38)
> Comment on attachment 416669 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416669&action=review
> 
> Thanks for working on this!
> 
> > Source/WebCore/ChangeLog:37
> > +        (WebCore::mapLogicalCornerToPhysicalCorner):
> >  2020-12-22  Simon Fraser  <simon.fraser@apple.com>
> 
> Please leave a newline at the end here.
> 
> > Source/WebCore/ChangeLog:70
> > -        (This saves a loop on the inline level boxes while constructing the lines at InlineContentBuilder::createDisplayLines) 
> > +        (This saves a loop on the inline level boxes while constructing the lines at InlineContentBuilder::createDisplayLines)
> 
> Hey, this was probably just your editor going wild, but please leave the
> whitespace in the rest of the ChangeLog (or really anything you're not
> actively changing)  alone. It isn't necessary to change.

Thanks very much for the comments! The files have been updated.
Comment 41 zsun 2021-01-05 08:23:33 PST
Created attachment 417005 [details]
Patch
Comment 42 Oriol Brufau 2021-01-07 13:39:09 PST
Comment on attachment 417005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417005&action=review

> Source/WebCore/platform/text/WritingMode.h:90
> +    return isFlippedTextFlow(textflow) != isVerticalTextFlow(textflow);

You could make isFlippedLinesWritingMode call this function to avoid duplicating the logic:

    constexpr inline bool isFlippedLinesWritingMode(WritingMode writingMode)
    {
        return isFlippedLinesTextFlow(makeTextFlow(writingMode, TextDirection::LTR));
    }

> Source/WebCore/platform/text/WritingMode.h:190
> +    BoxCorner physicalCorner = static_cast<BoxCorner>(logicalBoxCorner);

physicalCorner doesn't seem needed.

> Source/WebCore/platform/text/WritingMode.h:229
> +    return physicalCorner;

So I guess you are returning physicalCorner because some compiler was failing to notice that all cases are covered?
Then I would add an ASSERT_NOT_REACHED() and just return some random corner like BoxCorner::TopLeft.
I don't think static_cast<BoxCorner>(logicalBoxCorner) makes much sense.

Or just replace the entire switch with

    bool isBlockStart = logicalBoxCorner == LogicalBoxCorner::StartStart || logicalBoxCorner == LogicalBoxCorner::StartEnd;
    bool isInlineStart = logicalBoxCorner == LogicalBoxCorner::StartStart || logicalBoxCorner == LogicalBoxCorner::EndStart;
    if (isBlockStart == isFlippedTextFlow(textflow)) {
        if (isInlineStart == isReversedTextFlow(textflow))
            return BoxCorner::BottomRight;
    } else if (isInlineStart != isReversedTextFlow(textflow)) {
       return BoxCorner::TopLeft;
    }
    if (isBlockStart == isFlippedLinesTextFlow(textflow))
        return BoxCorner::BottomLeft;
    return BoxCorner::TopRight;
Comment 43 zsun 2021-01-08 06:18:31 PST
Created attachment 417266 [details]
Patch
Comment 44 zsun 2021-01-08 06:20:24 PST
(In reply to Oriol Brufau from comment #42)
> Comment on attachment 417005 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417005&action=review
> 
> > Source/WebCore/platform/text/WritingMode.h:90
> > +    return isFlippedTextFlow(textflow) != isVerticalTextFlow(textflow);
> 
> You could make isFlippedLinesWritingMode call this function to avoid
> duplicating the logic:
> 
>     constexpr inline bool isFlippedLinesWritingMode(WritingMode writingMode)
>     {
>         return isFlippedLinesTextFlow(makeTextFlow(writingMode,
> TextDirection::LTR));
>     }
> 
> > Source/WebCore/platform/text/WritingMode.h:190
> > +    BoxCorner physicalCorner = static_cast<BoxCorner>(logicalBoxCorner);
> 
> physicalCorner doesn't seem needed.
> 
> > Source/WebCore/platform/text/WritingMode.h:229
> > +    return physicalCorner;
> 
> So I guess you are returning physicalCorner because some compiler was
> failing to notice that all cases are covered?
> Then I would add an ASSERT_NOT_REACHED() and just return some random corner
> like BoxCorner::TopLeft.
> I don't think static_cast<BoxCorner>(logicalBoxCorner) makes much sense.
> 
> Or just replace the entire switch with
> 
>     bool isBlockStart = logicalBoxCorner == LogicalBoxCorner::StartStart ||
> logicalBoxCorner == LogicalBoxCorner::StartEnd;
>     bool isInlineStart = logicalBoxCorner == LogicalBoxCorner::StartStart ||
> logicalBoxCorner == LogicalBoxCorner::EndStart;
>     if (isBlockStart == isFlippedTextFlow(textflow)) {
>         if (isInlineStart == isReversedTextFlow(textflow))
>             return BoxCorner::BottomRight;
>     } else if (isInlineStart != isReversedTextFlow(textflow)) {
>        return BoxCorner::TopLeft;
>     }
>     if (isBlockStart == isFlippedLinesTextFlow(textflow))
>         return BoxCorner::BottomLeft;
>     return BoxCorner::TopRight;

Updated. Thank you!
Comment 45 Oriol Brufau 2021-01-11 10:15:15 PST
Informal r+, but I'm not a reviewer.
Comment 46 Simon Fraser (smfr) 2021-01-11 10:28:42 PST
Comment on attachment 417266 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417266&action=review

> Source/WebCore/css/CSSProperty.cpp:63
> +static CSSPropertyID resolveToPhysicalProperty(TextDirection direction, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner, const StylePropertyShorthand& shorthand)
> +{
> +    return shorthand.properties()[static_cast<size_t>(mapLogicalCornerToPhysicalCorner(makeTextFlow(writingMode, direction), logicalBoxCorner))];
> +}

This code is unsafe. If it's called with some random StylePropertyShorthand (not border-radius) then it can run off the end of the properties() array.

Since you only call this for borderRadiusShorthand(), make it explicitly about border-radius.
Comment 47 Oriol Brufau 2021-01-11 11:14:02 PST
Comment on attachment 417266 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417266&action=review

>> Source/WebCore/css/CSSProperty.cpp:63
>> +}
> 
> This code is unsafe. If it's called with some random StylePropertyShorthand (not border-radius) then it can run off the end of the properties() array.
> 
> Since you only call this for borderRadiusShorthand(), make it explicitly about border-radius.

Good point, though the existing resolveToPhysicalProperty are also affected, aren't they?
Maybe check the length() of the shorthand in a release assert for the functions that take a shorthand,
and for the third function replace the `const CSSPropertyID* properties` parameter with `const CSSPropertyID (&properties)[2]`?
That's basically what Chromium does.
Comment 48 zsun 2021-01-13 07:55:47 PST
Created attachment 417531 [details]
Patch
Comment 49 EWS 2021-01-13 11:25:42 PST
Committed r271447: <https://trac.webkit.org/changeset/271447>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417531 [details].