Bug 229756 – [css-flexbox] Add support for left & right css-align-3 positional alignment properties

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-flexbox] Add support for left & right css-align-3 positional alignment properties

[Sergio Villar Senin]

Created attachment 437009 [details]
Patch

[Javier Fernandez]

Comment on attachment 437009 [details]
Patch

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

r=me

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1533
> +

nit: unnecessary empty line.

[Manuel Rego Casasnovas]

Comment on attachment 437009 [details]
Patch

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

r=me, I left some inline comments.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1502
> +static LayoutUnit initialJustifyContentOffset(const RenderStyle& style, LayoutUnit availableFreeSpace, unsigned numberOfChildren, bool isReversed)

Do we need to pass style? Can't we just use style() on this method?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1511
> +    if (justifyContent == ContentPosition::Left || justifyContent == ContentPosition::Right) {
> +        if (style.isColumnFlexDirection()) {
> +            if (style.isHorizontalWritingMode()) {
> +                // If the propertyâs axis is not parallel with either left<->right axis, this value behaves as start. Currently,

Nit: weird character here.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1512
> +                // the only case where the propertyâs axis is not parallel with either left<->right axis is in a column flexbox.

Nit: and here too.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1523
> +                // https: //www.w3.org/TR/css-align-3/#valdef-justify-content-left
> +                justifyContent = ContentPosition::Start;
> +            } else if ((justifyContent == ContentPosition::Left && style.isFlippedBlocksWritingMode()) || (justifyContent == ContentPosition::Right && style.isFlippedLinesWritingMode()))
> +                justifyContent = ContentPosition::End;
> +            else
> +                justifyContent = ContentPosition::Start;
> +        } else if ((justifyContent == ContentPosition::Left && !style.isLeftToRightDirection()) || (justifyContent == ContentPosition::Right && style.isLeftToRightDirection()))
> +            justifyContent = ContentPosition::End;
> +        else
> +            justifyContent = ContentPosition::Start;
> +    }

I don't have a strong opinion, but I have an alternative code for this, not sure if that simplifies things or not, but at least in my head it makes it easier to understand it.
The basic idea is that "left" is usually "start" and "right" is usually "end", unless some specific conditions happen.

Could we write something like this (of course adding comments as needed)?
  if (justifyContent == ContentPosition::Left) {
    justifyContent = ContentPosition::Start;

    if (style.isColumnFlexDirection())
      if (!style.isHorizontalWritingMode() && style.isFlippedBlocksWritingMode())
        justifyContent = ContentPosition::End;
    else if (!style.isLeftToRightDirection()))
      justifyContent = ContentPosition::End;
  }
  if (justifyContent == ContentPosition::Right) {
    justifyContent = ContentPosition::End;

    if (style.isColumnFlexDirection())
      if (style.isHorizontalWritingMode() || !style.isFlippedLinesWritingMode())
        justifyContent = ContentPosition::Start;
    else if (!style.isLeftToRightDirection()))
      justifyContent = ContentPosition::Start;
  }

Do you think it's easier or hader to follow?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1861
> +    ContentDistribution distribution = style().resolvedJustifyContentDistribution(contentAlignmentNormalBehavior());

We end up calling this twice, first in initialJustifyContentOffset() and later here.

I believe I won't change the method signature and keep passing the ContentPosition and ContentDitribution to it.

[Sergio Villar Senin]

Comment on attachment 437009 [details]
Patch

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

Many thanks for the useful reviews. I'll address the comments before landing.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1502
>> +static LayoutUnit initialJustifyContentOffset(const RenderStyle& style, LayoutUnit availableFreeSpace, unsigned numberOfChildren, bool isReversed)
> 
> Do we need to pass style? Can't we just use style() on this method?

It's a static function not a method :)

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1511
>> +                // If the propertyâs axis is not parallel with either left<->right axis, this value behaves as start. Currently,
> 
> Nit: weird character here.

hmm, very likely copy-paste issues from spec text.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1523
>> +    }
> 
> I don't have a strong opinion, but I have an alternative code for this, not sure if that simplifies things or not, but at least in my head it makes it easier to understand it.
> The basic idea is that "left" is usually "start" and "right" is usually "end", unless some specific conditions happen.
> 
> Could we write something like this (of course adding comments as needed)?
>   if (justifyContent == ContentPosition::Left) {
>     justifyContent = ContentPosition::Start;
> 
>     if (style.isColumnFlexDirection())
>       if (!style.isHorizontalWritingMode() && style.isFlippedBlocksWritingMode())
>         justifyContent = ContentPosition::End;
>     else if (!style.isLeftToRightDirection()))
>       justifyContent = ContentPosition::End;
>   }
>   if (justifyContent == ContentPosition::Right) {
>     justifyContent = ContentPosition::End;
> 
>     if (style.isColumnFlexDirection())
>       if (style.isHorizontalWritingMode() || !style.isFlippedLinesWritingMode())
>         justifyContent = ContentPosition::Start;
>     else if (!style.isLeftToRightDirection()))
>       justifyContent = ContentPosition::Start;
>   }
> 
> Do you think it's easier or hader to follow?

Yeah much easier indeed, I just tried to avoid the duplication of the structure but for the sake of maintenance I think it's better to follow your approach.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1533
>> +
> 
> nit: unnecessary empty line.

ok.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:1861
>> +    ContentDistribution distribution = style().resolvedJustifyContentDistribution(contentAlignmentNormalBehavior());
> 
> We end up calling this twice, first in initialJustifyContentOffset() and later here.
> 
> I believe I won't change the method signature and keep passing the ContentPosition and ContentDitribution to it.

That's true for this case but not for staticMainAxisPositionForPositionedChild() case for example.

The thing is that you need the style() anyway, so provided that the resolvedJustifyContentPosition() are really cheap (they're just getters and logical operations) I think we could keep them as is even though we're indeed calling them twice.

[Sergio Villar Senin]

Committed r282078 (241378@main): <https://commits.webkit.org/241378@main>

[Radar WebKit Bug Importer]

<rdar://problem/82815793>