change RenderFlexibleBox to act on logical coordinates

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

change RenderFlexibleBox to act on logical coordinates

[Ojan Vafai]

Created attachment 107413 [details]
Patch

[Tony Chang]

Comment on attachment 107413 [details]
Patch

LGTM.  May want to wait a day or two to see if Hyatt has any suggestions.

[Dave Hyatt]

Comment on attachment 107413 [details]
Patch

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

In general I have two high level concerns:

(1) Make sure your function names don't just drop "Horizontal" and not replace it with anything. You should have "InlineAxis" or "InlineDirection" in those names.

(2) You need to make use of more helpers to deal with flex item children that have orthogonal writing modes.

(3) It looks like RTL behavior isn't right. Remember that converting left to start and right to end is not the same thing for RTL. start is on the right for RTL. You need to decide if you're writing the code to go from "line left" to "line right" in which case you use logicalLeft and logicalRight, or if you really are trying to write the code to go from the start edge to the end edge, i.e., to be directionally abstract.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:102
> +    layoutFlexibleBlock(relayoutChildren);

Maybe you want to call this layoutInlineDirection or something? The old code used the term "Horizontal" and if you convert to logical the equivalent term would be "inline direction."

> Source/WebCore/rendering/RenderFlexibleBox.cpp:116
> +static LayoutUnit preferredFlexItemLogicalContentWidth(RenderBox* child)

preferredLogicalContentWidthForFlexItem perhaps? This is a mouthful heh.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:119
> +    if (child->style()->logicalWidth().isAuto())
> +        return child->maxPreferredLogicalWidth() - child->borderStart() - child->borderEnd() - child->scrollbarLogicalHeight() - child->paddingStart() - child->paddingEnd();

I don't completely understand this. Remember the child's writing mode can be different from the parent's, and so what the child thinks are its start and end may be along the opposite axis to the one you're laying out children along. In general if you find yourself directly accessing child border and padding values like this, you're probably doing something wrong. (I say probably, since there may be a few cases where it's ok.)

If you look at RenderBlock there are helper functions to get the margins for a child along the correct axis, e.g., marginLogicalTopForChild, to handle the case where the child's writing mode may be different. You may need something like that here.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:123
> -void RenderFlexibleBox::layoutHorizontalBlock(bool relayoutChildren)
> +void RenderFlexibleBox::layoutFlexibleBlock(bool relayoutChildren)

As stated above, I think you need to work "inline direction" into this function name, since that is what it's doing.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:135
> -    while (!runFreeSpaceAllocationAlgorithmHorizontal(availableFreeSpace, totalPositiveFlexibility, totalNegativeFlexibility, inflexibleItems, childSizes)) {
> +    while (!runFreeSpaceAllocationAlgorithm(availableFreeSpace, totalPositiveFlexibility, totalNegativeFlexibility, inflexibleItems, childSizes)) {

Same here. Seems like you need "inline direction" in here.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:172
> +        preferredLogicalWidth += child->style()->marginStart().calcMinValue(flexboxAvailableLogicalWidth);
> +        preferredLogicalWidth += child->style()->marginEnd().calcMinValue(flexboxAvailableLogicalWidth);
> +        preferredLogicalWidth += child->style()->paddingStart().calcMinValue(flexboxAvailableLogicalWidth);
> +        preferredLogicalWidth += child->style()->paddingEnd().calcMinValue(flexboxAvailableLogicalWidth);

Seems like you should be using the helper functions here in case the child has an orthogonal writing mode.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:174
> -        if (child->style()->marginLeft().isAuto())
> +        if (child->style()->marginStart().isAuto())

Same.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:176
> -        if (child->style()->marginRight().isAuto())
> +        if (child->style()->marginEnd().isAuto())

Same.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:179
> +        preferredLogicalWidth += child->borderStart() + child->borderEnd();

Same.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:183
> +        totalPositiveFlexibility += logicalPositiveFlexForChild(child);
> +        totalNegativeFlexibility += logicalNegativeFlexForChild(child);

Helpers. Yay! This should be correct.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:188
> -bool RenderFlexibleBox::runFreeSpaceAllocationAlgorithmHorizontal(LayoutUnit& availableFreeSpace, float& totalPositiveFlexibility, float& totalNegativeFlexibility, InflexibleFlexItemSize& inflexibleItems, WTF::Vector<LayoutUnit>& childSizes)
> +bool RenderFlexibleBox::runFreeSpaceAllocationAlgorithm(LayoutUnit& availableFreeSpace, float& totalPositiveFlexibility, float& totalNegativeFlexibility, InflexibleFlexItemSize& inflexibleItems, WTF::Vector<LayoutUnit>& childSizes)

Include "inline direction " or "inline axis" in the name.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:203
> +                Length childLogicalMaxWidth = child->style()->logicalMaxWidth();

Need a helper here. The child might be orthogonal, in which case getting its logical width isn't correct.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:215
> +                Length childLogicalMinWidth = child->style()->logicalMinWidth();

Same here.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:245
> -    LayoutUnit xOffset = borderLeft() + paddingLeft();
> +    LayoutUnit logicalXOffset = borderStart() + paddingStart();

This seems suspicious for RTL that you're operating from the right edge, but I didn't look closely enough to tell for sure. We have stuff like logicalLeftOffsetForContent() to help you get the left edge and move from there.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:271
> +        child->setLogicalLocation(IntPoint(logicalXOffset, logicalYOffset));

This seems wrong for RTL. Logical location is just flipping x and y for horizontal vs. vertical. It doesn't deal with directions.

[Ojan Vafai]

Created attachment 107793 [details]
Patch

[Ojan Vafai]

I think this addresses all your comments. I also added a test for various writing-modes/directions. They all seem to do the right thing except the vertical-rl and horizontal-bt ones don't pass due to https://bugs.webkit.org/show_bug.cgi?id=68304.

I added the following helper methods to RenderFlexibleBox. I wasn't sure if you'd rather I add some of these to RenderBlock. It's not clear to me how generically useful they are.

LayoutUnit logicalBorderWidthForChild(RenderBox* child);
LayoutUnit logicalPaddingWidthForChild(RenderBox* child);
LayoutUnit logicalScrollbarHeightForChild(RenderBox* child);
Length marginStartStyleForChild(RenderBox* child);
Length marginEndStyleForChild(RenderBox* child);
void setLogicalOverrideSize(RenderBox* child, LayoutUnit childPreferredSize);

[Ojan Vafai]

(In reply to comment #3)
> (From update of attachment 107413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107413&action=review
> (3) It looks like RTL behavior isn't right. Remember that converting left to start and right to end is not the same thing for RTL. start is on the right for RTL. You need to decide if you're writing the code to go from "line left" to "line right" in which case you use logicalLeft and logicalRight, or if you really are trying to write the code to go from the start edge to the end edge, i.e., to be directionally abstract.

I went with startEdge and then convert it to logicalLeft right before the setLogicalLocationForChild call.

[Ojan Vafai]

Created attachment 107899 [details]
Patch

[Dave Hyatt]

Comment on attachment 107899 [details]
Patch

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

Switch all the queries of writing mode to use isHorizontalWritingMode() instead of style()->isHorizontalWritingMode().

The one other observation I have is make your helper functions do as much work as possible to avoid switching on writing mode too much. For example if you only ever look at border and padding together, then combine those into a single helper rather than calling two functions, one for border and one for padding.

Things mostly look correct to me except for the orthogonal case mentioned below.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:155
> +        return child->maxPreferredLogicalWidth() - logicalBorderWidthForChild(child) - logicalScrollbarHeightForChild(child) - logicalPaddingWidthForChild(child);

You can't use maxPreferredLogicalWidth if your writing mode is orthogonal to your parent's. I'm not sure what you should be using, since I can't remember what the writing-mode spec said. It's either 0 or the height of the viewport or something. I can't recall, but should be in the spec.

[Ojan Vafai]

(In reply to comment #8)
> (From update of attachment 107899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107899&action=review
> The one other observation I have is make your helper functions do as much work as possible to avoid switching on writing mode too much. For example if you only ever look at border and padding together, then combine those into a single helper rather than calling two functions, one for border and one for padding.

There is a case where we get borderWidth, but not paddingWidth. Although, there's no case where we get paddingWidth without borderWidth, so I could have borderWidth/borderAndPaddingWidth instead of borderWidth/paddingWidth.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:155
> > +        return child->maxPreferredLogicalWidth() - logicalBorderWidthForChild(child) - logicalScrollbarHeightForChild(child) - logicalPaddingWidthForChild(child);
> 
> You can't use maxPreferredLogicalWidth if your writing mode is orthogonal to your parent's. I'm not sure what you should be using, since I can't remember what the writing-mode spec said. It's either 0 or the height of the viewport or something. I can't recall, but should be in the spec.

Looks like shrink-to-fit is the correct behavior: http://dev.w3.org/csswg/css3-writing-modes/#orthogonal-auto

[Ojan Vafai]

Created attachment 108063 [details]
Patch

[Dave Hyatt]

Comment on attachment 108063 [details]
Patch

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

r=me

> Source/WebCore/rendering/RenderBlock.h:214
> +    void setLogicalLocationForChild(RenderBox* child, const LayoutPoint&);
>      void setLogicalLeftForChild(RenderBox* child, LayoutUnit logicalLeft, ApplyLayoutDeltaMode = DoNotApplyLayoutDelta);
>      void setLogicalTopForChild(RenderBox* child, LayoutUnit logicalTop, ApplyLayoutDeltaMode = DoNotApplyLayoutDelta);

Ultimately you'll probably want to support layout deltas for flexible box children. You should file a followup bug about this.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:324
> +        setLogicalLocationForChild(child, IntPoint(logicalLeft, logicalTop));

You should probably have a placeChild method like the old flexible box did that handles repainting. Right now you're moving without repainting, which will result in repaint issues if the child moves without having to get a layout again.

[Ojan Vafai]

Committed r95577: <http://trac.webkit.org/changeset/95577>