WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68129
change RenderFlexibleBox to act on logical coordinates
https://bugs.webkit.org/show_bug.cgi?id=68129
Summary
change RenderFlexibleBox to act on logical coordinates
Ojan Vafai
Reported
2011-09-14 15:40:08 PDT
change RenderFlexibleBox to act on logical coordinates
Attachments
Patch
(17.62 KB, patch)
2011-09-14 15:46 PDT
,
Ojan Vafai
no flags
Details
Formatted Diff
Diff
Patch
(33.20 KB, patch)
2011-09-18 13:20 PDT
,
Ojan Vafai
no flags
Details
Formatted Diff
Diff
Patch
(33.49 KB, patch)
2011-09-19 11:57 PDT
,
Ojan Vafai
no flags
Details
Formatted Diff
Diff
Patch
(34.45 KB, patch)
2011-09-20 15:22 PDT
,
Ojan Vafai
hyatt
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Ojan Vafai
Comment 1
2011-09-14 15:46:26 PDT
Created
attachment 107413
[details]
Patch
Tony Chang
Comment 2
2011-09-14 16:09:34 PDT
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 3
2011-09-14 16:53:23 PDT
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
Comment 4
2011-09-18 13:20:16 PDT
Created
attachment 107793
[details]
Patch
Ojan Vafai
Comment 5
2011-09-18 13:24:11 PDT
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
Comment 6
2011-09-18 13:25:52 PDT
(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
Comment 7
2011-09-19 11:57:44 PDT
Created
attachment 107899
[details]
Patch
Dave Hyatt
Comment 8
2011-09-20 11:41:07 PDT
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
Comment 9
2011-09-20 13:40:38 PDT
(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
Comment 10
2011-09-20 15:22:55 PDT
Created
attachment 108063
[details]
Patch
Dave Hyatt
Comment 11
2011-09-20 15:33:25 PDT
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
Comment 12
2011-09-20 15:45:22 PDT
Committed
r95577
: <
http://trac.webkit.org/changeset/95577
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug