Bug 97948 - flexbox does wrong baseline item alignment in columns
Summary: flexbox does wrong baseline item alignment in columns
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks: 62048
  Show dependency tree / graph
 
Reported: 2012-09-28 15:58 PDT by Tony Chang
Modified: 2012-10-01 18:51 PDT (History)
3 users (show)

See Also:

Attachments
Patch (22.18 KB, patch)
2012-10-01 15:13 PDT, Tony Chang 		no flags Details | Formatted Diff | Diff
Patch (23.55 KB, patch)
2012-10-01 16:06 PDT, Tony Chang 		no flags Details | Formatted Diff | Diff
Patch (22.85 KB, patch)
2012-10-01 16:07 PDT, Tony Chang 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-09-28 15:58:06 PDT 
The spec says, "If the flex item's inline axis is the same as the cross axis, this value is identical to ‘flex-start’."  We're not doing that.  See https://bugs.webkit.org/attachment.cgi?id=163005 for some test cases.
Comment 1 Tony Chang 2012-10-01 15:13:20 PDT 
Created attachment 166555 [details]
Patch
Comment 2 Ojan Vafai 2012-10-01 15:42:34 PDT 
Comment on attachment 166555 [details]
Patch

What if, instead of changing the logic of AlignBaseline, we made the chunk before the switch statement pretend that we have a different alignment. That would reduce some code duplication and more clearly match the spec text.

FlexAlign alignment = alignmentForChild(child);
if (alignment == AlignBaseline and hasOrthogonalFlow(child))
    alignment = (style()->flexWrap() == FlexWrapReverse) ? AlignFlexEnd : AlignFlexStart;

Up to you. I find this logic easier to follow.
Comment 3 Tony Chang 2012-10-01 16:06:02 PDT 
Created attachment 166563 [details]
Patch
Comment 4 Tony Chang 2012-10-01 16:07:36 PDT 
Created attachment 166564 [details]
Patch
Comment 5 Tony Chang 2012-10-01 16:08:05 PDT 
Comment on attachment 166564 [details]
Patch

Can you take another look? I moved the logic into alignmentForChild.
Comment 6 WebKit Review Bot 2012-10-01 16:47:57 PDT 
Comment on attachment 166564 [details]
Patch

Rejecting attachment 166564 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
webkit-commit-queue/Source/WebKit/chromium/webkit --revision 159019 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
51>At revision 159019.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/14089966
Comment 7 Build Bot 2012-10-01 17:46:06 PDT 
Comment on attachment 166564 [details]
Patch

Attachment 166564 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14084986

New failing tests:
http/tests/workers/terminate-during-sync-operation.html
Comment 8 WebKit Review Bot 2012-10-01 18:51:11 PDT 
Comment on attachment 166564 [details]
Patch

Clearing flags on attachment: 166564

Committed r130110: <http://trac.webkit.org/changeset/130110>
Comment 9 WebKit Review Bot 2012-10-01 18:51:14 PDT 
All reviewed patches have been landed.  Closing bug.