Bug 101294 – While absolute positioning is put before the first flexitem, flexitems will move to a new line.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[wayfind]

an absolute div before all flex item in a container will lead to flex item lmove to a new line.

[wayfind]

Created attachment 172473 [details]
Patch

[wayfind]

If absolute div before all flex item in a container,and the set flex container to "-webkit-flex-flow: row wrap" the flex item after absolute div will move to a new line.

[wayfind]

We may come across this problem in RenderFlexibleBox::computeNextFlexLine.when it comes to the move-to-next-line issue, 2 condition must be taken into consideration 1) whether the width has gone beyond lineBreakLength , 2) whether the rderedChildren is empty. 
If the width has gone beyond ineBreakLength and absloute layout is ongoing in the container, wrong line-moving might occur. 
The way to solve the problem is to jutify whether orderedChildren could collect at least one flex item, if yes, move to next line

[wayfind]

Created attachment 172511 [details]
Prev patch lost a LayoutTesst Changelog.

[wayfind]

Created attachment 172525 [details]
Add more Changelog

[wayfind]

Created attachment 172560 [details]
Finally patch

[wayfind]

Created attachment 172566 [details]
Really finally patch

[Ojan Vafai]

Comment on attachment 172566 [details]
Really finally patch

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

Thanks for the fix. The change looks good. Just a bunch of style nits.

You're ChangeLog description does not match the standard format. I recommend deleting them and uploading this patch using "webkit-patch upload". Then it will create the ChangeLog entries for you and you just need to fill in the detailed description section. Also, that will properly mark the patch as ready for review by setting r?.

> LayoutTests/css3/flexbox/flex-algorithm.html:202
> +  <div style="position: absolute;"></div>

May as well check the x/y positions on this element as well.

> LayoutTests/css3/flexbox/flex-algorithm.html:203
> +  <div data-offset-y="0" style="width: 700px;"></div>

May as well add data-offset-x=0 as well for good measure.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:898
> +    bool hasOneItemCollected = false;

I'd prefer a name like...lineHasInFlowItem. Also, a line break after this line would break up the code to make it a bit more readable.

[wayfind]

Created attachment 172802 [details]
Patch

[Ojan Vafai]

Comment on attachment 172802 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

You need to put a description of the change here.

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

You can delete this line.

> LayoutTests/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Need a short description here.

[wayfind]

Created attachment 172928 [details]
Patch

[Ojan Vafai]

Comment on attachment 172928 [details]
Patch

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

This patch doesn't seem to apply to tip of tree (see all the bot failures).

> Source/WebCore/ChangeLog:10
> +		If there is an absolutely positioned flex item before other items in a container and the container is set to "-webkit-flex-flow: row wrap", the flex item after the absolutely positioned will move to a new line.
> +		This issue has to do with RenderFlexibleBox::computeNextFlexLine. When determing line breaks, the algorithm sees if 1) the total width exceeds lineBreakLength and 2) whether orderedChildren is non-empty.  But then, if the total width exceeds lineBreakLength and there's only absolutely positioned elemments in orderedChildren then the conditions are met and the algorithm mistakenly breaks the line.
> +		The solution is to see if orderedChildren collects at least a flex item. If it does, break the line.

Please use only spaces and not tabs.

[wayfind]

Created attachment 172931 [details]
Patch

[Ojan Vafai]

Comment on attachment 172931 [details]
Patch

Patch looks good. Thanks. Just need to figure out why it's not applying. Looks like there's a conflict in RenderFlexibleBox.cpp. e.g. see http://queues.webkit.org/results/14757641.

[wayfind]

Created attachment 172975 [details]
Patch

[wayfind]

Created attachment 172986 [details]
Patch

[Ojan Vafai]

Comment on attachment 172986 [details]
Patch

Yay! Thanks! I assume you want me to commit this as well. I'm adding cq+ (puts in in the commit queue). For future reference, you request cq by setting cq?. That lets the reviewer know you would like a cq+ once the patch is r+'ed.

[WebKit Review Bot]

Comment on attachment 172986 [details]
Patch

Clearing flags on attachment: 172986

Committed r133906: <http://trac.webkit.org/changeset/133906>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Comment on attachment 172975 [details]
Patch

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

> LayoutTests/css3/flexbox/flex-algorithm.html:201
> +<div class="flexbox" style="height: 60px; -webkit-flex-flow: row wrap; position: relative;">

Nit: Instead of using -webkit-flex-flow, you should use the row and wrap class from flexbox.css so this code will run in Firefox.