Bug 96188 - inline-flex baseline is sometimes wrong
Summary: inline-flex baseline is sometimes wrong
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: 95889
  Show dependency treegraph
 
Reported: 2012-09-08 18:07 PDT by Dave Barton
Modified: 2012-10-04 09:47 PDT (History)
9 users (show)

See Also:


Attachments
inconsistent inline-flex baselines (1.15 KB, text/html)
2012-09-08 18:07 PDT, Dave Barton
no flags Details
column, baseline, tall really strange (1.13 KB, text/html)
2012-09-09 12:07 PDT, Dave Barton
no flags Details
Patch (76.80 KB, patch)
2012-10-03 15:09 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (78.69 KB, patch)
2012-10-03 15:33 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (85.46 KB, patch)
2012-10-03 16:53 PDT, Tony Chang
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Barton 2012-09-08 18:07:06 PDT
Created attachment 162972 [details]
inconsistent inline-flex baselines

The attached file shows that inline-flex elements with { align-items: baseline } seem to export flaky baselines to their containing block. I believe that "x" and "a" should always have the same baseline, according to <http://dev.w3.org/csswg/css3-flexbox/#flex-baselines>.

I am experimenting with using flex boxes for MathML, and except for this issue it looks good so far. The order, align-*, flex-direction, etc. properties would be very useful, as well as the prohibition against floats & columns. MathML doesn't need actual flexing, but does need these other features, including a precise baseline. If you know a reason flex boxes would be a bad idea for MathML, please state it, thanks!
Comment 1 Dave Barton 2012-09-09 12:07:36 PDT
Created attachment 163005 [details]
column, baseline, tall really strange

Here the "1"s appear to the right of the "a b"s! This is really strange. You need -webkit-flex-direction: column; and -webkit-align-items: baseline; and the .tall style rule to see this happen.
Comment 2 Tony Chang 2012-09-10 16:20:26 PDT
I don't think we have any tests for the baseline of the flexbox itself yet.  I'm marking this as part of the "fix the baseline of the flexbox" bug.
Comment 3 Tony Chang 2012-09-28 16:00:16 PDT
The second test case is a bug in align-items.  I've filed bug 97948 for it.
Comment 4 Tony Chang 2012-10-03 15:09:42 PDT
Created attachment 166968 [details]
Patch
Comment 5 Tony Chang 2012-10-03 15:25:48 PDT
Comment on attachment 166968 [details]
Patch

Actually, I'm going to change one of the results.
Comment 6 Tony Chang 2012-10-03 15:33:28 PDT
Created attachment 166972 [details]
Patch
Comment 7 WebKit Review Bot 2012-10-03 15:35:42 PDT
Attachment 166972 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/qt/TestExpectations:2730:  Path does not exist.  [test/expectations] [5]
LayoutTests/platform/qt/TestExpectations:2731:  Path does not exist.  [test/expectations] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Ojan Vafai 2012-10-03 16:14:38 PDT
Comment on attachment 166972 [details]
Patch

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

Just fix the marginBefore issue. The rest looks good.

For thoroughness sake, might want to add a test to verify the following text from the spec:

When calculating the baseline according to the above rules, if the box contributing a baseline has an ‘overflow’ value that allows scrolling, the box must be treated as being in its initial scroll position for the purpose of determining its baseline.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:242
> +        return baseline + marginBefore();

marginBefore is wrong here. For horizontal we need marginTop and for vertical we need marginRight.

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

Extra line break?
Comment 9 Tony Chang 2012-10-03 16:53:05 PDT
Created attachment 166996 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-10-03 19:58:56 PDT
Comment on attachment 166996 [details]
Patch for landing

Rejecting attachment 166996 [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:
ols/Scripts/new-run-webkit-tests', '--chromium', '--skip-failing-tests', '--no-ne..." exit_code: 10
forms/time-multiple-fields/time-multiple-fields-appearance-style.html [ ImageOnlyFailure ] 
  fast/forms/week-multiple-fields/week-multiple-fields-appearance-basic.html [ ImageOnlyFailure ] 
  fast/forms/week-multiple-fields/week-multiple-fields-appearance-pseudo-elements.html [ ImageOnlyFailure ] 
  fast/forms/week-multiple-fields/week-multiple-fields-appearance-style.html [ ImageOnlyFailure ] 


Full output: http://queues.webkit.org/results/14129905
Comment 11 Tony Chang 2012-10-04 09:47:36 PDT
Committed r130405: <http://trac.webkit.org/changeset/130405>