Bug 218093 – [css-logical] Implement logical border-radius

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

https://drafts.csswg.org/css-logical/#border-radius-properties

[Radar WebKit Bug Importer]

<rdar://problem/70579854>

Created attachment 416234 [details]
Patch

[Javier Fernandez]

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

[Manuel Rego Casasnovas]

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.

[Oriol Brufau]

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

[Simon Fraser (smfr)]

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()

Created attachment 416325 [details]
Patch

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

(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

(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!

(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!

Created attachment 416327 [details]
Patch

(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!

Created attachment 416330 [details]
Patch

Created attachment 416335 [details]
Patch

[Simon Fraser (smfr)]

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.

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

[Oriol Brufau]

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.

Created attachment 416404 [details]
Patch

(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!

Created attachment 416409 [details]
Patch

(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!

[Oriol Brufau]

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

Created attachment 416432 [details]
Patch

(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!

[Oriol Brufau]

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.

[Simon Fraser (smfr)]

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?)

[Simon Fraser (smfr)]

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

Created attachment 416517 [details]
Patch

[EWS Watchlist]

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

Created attachment 416519 [details]
Patch

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

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

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

[Oriol Brufau]

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;

Created attachment 416669 [details]
Patch

(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!

[Sam Weinig]

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  <[e-mail]>

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.

Created attachment 417001 [details]
Patch

(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  <[e-mail]>
> 
> 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.

Created attachment 417005 [details]
Patch

[Oriol Brufau]

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;

Created attachment 417266 [details]
Patch

(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!

[Oriol Brufau]

Informal r+, but I'm not a reviewer.

[Simon Fraser (smfr)]

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.

[Oriol Brufau]

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.

Created attachment 417531 [details]
Patch

[EWS]

Committed r271447: <https://trac.webkit.org/changeset/271447>

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