Bug 170176 – inline-block baseline not computed correctly for vertical-lr

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Javier Fernandez]

Created attachment 305586 [details]
Test case to reproduce the issue

A vertical-lr block with inline-block children does not compute correctly their baseline so they can be alignment properly.
This issue is not present when using other writing-modes

Attached a file with several test cases to reproduce the issue using different writing-modes and direction.

[Javier Fernandez]

This bug has been already FIXED in Blink, see the patch at the following CL: 

https://codereview.chromium.org/2523573003/

I'll try to port the same solution in WebKit.

[Javier Fernandez]

Created attachment 305588 [details]
Patch

[Build Bot]

Comment on attachment 305588 [details]
Patch

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

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/text/emphasis-avoid-ruby.html
fast/text/emphasis-overlap.html
fast/backgrounds/background-leakage-transforms.html
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305591 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305588 [details]
Patch

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

New failing tests:
fast/inline-block/baseline-vertical.html
fast/text/emphasis-avoid-ruby.html
fast/text/emphasis-overlap.html
fast/backgrounds/background-leakage-transforms.html
fast/writing-mode/border-styles-vertical-lr.html

[Build Bot]

Created attachment 305592 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305588 [details]
Patch

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

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/text/emphasis-avoid-ruby.html
fast/text/emphasis-overlap.html
fast/backgrounds/background-leakage-transforms.html
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305593 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305588 [details]
Patch

Attachment 305588 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3427430

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/inline-block/baseline-vertical.html
fast/backgrounds/background-leakage-transforms.html

[Build Bot]

Created attachment 305598 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Javier Fernandez]

Created attachment 305734 [details]
Patch

[Build Bot]

Comment on attachment 305734 [details]
Patch

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

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/text/emphasis-avoid-ruby.html
fast/text/emphasis-overlap.html
fast/backgrounds/background-leakage-transforms.html
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305738 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305734 [details]
Patch

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

New failing tests:
fast/inline-block/baseline-vertical.html
fast/text/emphasis-avoid-ruby.html
fast/text/emphasis-overlap.html
fast/backgrounds/background-leakage-transforms.html
fast/writing-mode/border-styles-vertical-lr.html

[Build Bot]

Created attachment 305741 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305734 [details]
Patch

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

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/text/emphasis-avoid-ruby.html
fast/text/emphasis-overlap.html
fast/backgrounds/background-leakage-transforms.html
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305744 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305734 [details]
Patch

Attachment 305734 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3434099

New failing tests:
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305748 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Javier Fernandez]

Created attachment 305785 [details]
Patch

[Build Bot]

Comment on attachment 305785 [details]
Patch

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

New failing tests:
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305797 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305785 [details]
Patch

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

New failing tests:
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305799 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 305785 [details]
Patch

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

New failing tests:
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305800 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Javier Fernandez]

Created attachment 305802 [details]
Patch

[Build Bot]

Comment on attachment 305802 [details]
Patch

Attachment 305802 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3436394

New failing tests:
fast/writing-mode/border-styles-vertical-lr.html
fast/inline-block/baseline-vertical.html
fast/backgrounds/background-leakage-transforms.html

[Build Bot]

Created attachment 305810 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Javier Fernandez]

Created attachment 305887 [details]
Patch

[Javier Fernandez]

Created attachment 305913 [details]
Patch

[Build Bot]

Comment on attachment 305913 [details]
Patch

Attachment 305913 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3443354

New failing tests:
fast/inline-block/baseline-vertical.html

[Build Bot]

Created attachment 305925 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Javier Fernandez]

Created attachment 306011 [details]
Patch

[Javier Fernandez]

@dhyatt, could you please take a look ?

[Javier Fernandez]

Gentle ping, reviewers.

[Jon Lee]

(In reply to Javier Fernandez from comment #36)
> Gentle ping, reviewers.

Is the latest patch the one for review? It doesn't have the right flags on it, so I don't think anyone saw it. Given how long it's been, it also may require a rebaseline.

[Javier Fernandez]

(In reply to Jon Lee from comment #37)
> (In reply to Javier Fernandez from comment #36)
> > Gentle ping, reviewers.
> 
> Is the latest patch the one for review? It doesn't have the right flags on
> it, so I don't think anyone saw it. Given how long it's been, it also may
> require a rebaseline.

Yes, it is. Sorry about the missing review flag :(

[Javier Fernandez]

@zalan @hyatt, could you please provide some feedback on this patch ? Thanks

[Antti Koivisto]

Comment on attachment 306011 [details]
Patch

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

Zalan/Hyatt should review.

> LayoutTests/fast/inline-block/baseline-vertical-expected.txt:4
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x503
> +  RenderBlock {HTML} at (0,0) size 800x504

Can you make a refest instead? Render tree dumps are fragile and annoying.

[Antti Koivisto]

*reftest

[Javier Fernandez]

(In reply to Antti Koivisto from comment #40)
> Comment on attachment 306011 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306011&action=review
> 
> Zalan/Hyatt should review.

Yes, I supposed it.

> 
> > LayoutTests/fast/inline-block/baseline-vertical-expected.txt:4
> > +layer at (0,0) size 800x600
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x503
> > +  RenderBlock {HTML} at (0,0) size 800x504
> 
> Can you make a refest instead? Render tree dumps are fragile and annoying.

Well, I don't like at all the tree dumps and image tests. However, for this baseline alignment cases I wasn't able to make a valid reference tests. Many of these baseline tests are implemented using tree dumps as well. In some cases they use flexbox vs block to define reference tests, but in this case both layouts use the same code, so we wouldn't notice regressions. 

The problem with baseline test is that it highly depends on the font family and size. I could have used 'ahem' but then it'd use a different baseline (central if I remember correctly) so we don't verify completely the changes applied in the code.

I thought about using absolute positioned for the reference tests, but as I said, it may fail because of different font family and size when running it in different platforms.

If anybody has an idea, I'd be happy to define a reference tests instead, of course.

[Javier Fernandez]

Created attachment 328963 [details]
Patch

Replace pixel tests by reference tests.

[EWS Watchlist]

Comment on attachment 328963 [details]
Patch

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

New failing tests:
fast/inline-block/baseline-vertical-08.html
fast/backgrounds/background-leakage-transforms.html
fast/inline-block/baseline-vertical-02.html
fast/inline-block/baseline-vertical-06.html
fast/writing-mode/border-styles-vertical-lr.html
fast/inline-block/baseline-vertical-07.html
fast/inline-block/baseline-vertical-01.html
fast/inline-block/baseline-vertical-05.html
fast/inline-block/baseline-vertical-04.html
fast/inline-block/baseline-vertical-03.html

[EWS Watchlist]

Created attachment 328966 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 328963 [details]
Patch

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

New failing tests:
fast/inline-block/baseline-vertical-08.html
fast/backgrounds/background-leakage-transforms.html
fast/inline-block/baseline-vertical-02.html
fast/inline-block/baseline-vertical-06.html
fast/writing-mode/border-styles-vertical-lr.html
fast/inline-block/baseline-vertical-07.html
fast/inline-block/baseline-vertical-01.html
fast/inline-block/baseline-vertical-05.html
fast/inline-block/baseline-vertical-04.html
fast/inline-block/baseline-vertical-03.html

[EWS Watchlist]

Created attachment 328968 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 328963 [details]
Patch

Attachment 328963 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5614447

New failing tests:
fast/inline-block/baseline-vertical-08.html
fast/backgrounds/background-leakage-transforms.html
fast/inline-block/baseline-vertical-02.html
fast/inline-block/baseline-vertical-06.html
fast/writing-mode/border-styles-vertical-lr.html
fast/inline-block/baseline-vertical-07.html
fast/inline-block/baseline-vertical-01.html
fast/inline-block/baseline-vertical-05.html
fast/inline-block/baseline-vertical-04.html
fast/inline-block/baseline-vertical-03.html

[EWS Watchlist]

Created attachment 328970 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 328963 [details]
Patch

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

New failing tests:
fast/inline-block/baseline-vertical-08.html
fast/backgrounds/background-leakage-transforms.html
fast/inline-block/baseline-vertical-02.html
fast/inline-block/baseline-vertical-06.html
fast/writing-mode/border-styles-vertical-lr.html
fast/inline-block/baseline-vertical-07.html
fast/inline-block/baseline-vertical-01.html
fast/inline-block/baseline-vertical-05.html
fast/inline-block/baseline-vertical-04.html
fast/inline-block/baseline-vertical-03.html

[EWS Watchlist]

Created attachment 328972 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Javier Fernandez]

Created attachment 329053 [details]
Patch

[EWS Watchlist]

Comment on attachment 329053 [details]
Patch

Attachment 329053 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5623432

New failing tests:
fast/inline-block/baseline-vertical-02.html
fast/inline-block/baseline-vertical-01.html
fast/inline-block/baseline-vertical-04.html
fast/inline-block/baseline-vertical-03.html

[EWS Watchlist]

Created attachment 329063 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Javier Fernandez]

Created attachment 330898 [details]
Patch

[EWS Watchlist]

Comment on attachment 330898 [details]
Patch

Attachment 330898 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6020397

New failing tests:
fast/inline-block/baseline-vertical-02.html
fast/inline-block/baseline-vertical-01.html
fast/inline-block/baseline-vertical-04.html
fast/inline-block/baseline-vertical-03.html

[EWS Watchlist]

Created attachment 330908 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 330898 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[EWS Watchlist]

Created attachment 330959 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Javier Fernandez]

Created attachment 331356 [details]
Patch

Added Failure TestExppectations for ios-sim

[Manuel Rego Casasnovas]

Comment on attachment 331356 [details]
Patch

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

The patch LGTM.

Just a question about border-styles-vertical-lr.html test, for the new expected files
I see color changes in the borders, is that expected?

> Source/WebCore/rendering/RenderBlockFlow.cpp:3061
> +            // InlineFlowBox::placeBoxesInBlockDirection will flip lines in
> +            // case of verticalLR mode, so we can assume verticalRL for now.

Nit: This could be just one line.

> LayoutTests/ChangeLog:30
> +        * fast/text/emphasis-overlap-expected.txt:

Nit: I believe this ChangeLog is not accurate.

> LayoutTests/platform/ios-simulator/TestExpectations:59
> +# These tests seem to fail due pixel rounding errors in the absolute positioned elements used as reference.

Nit: Shouldn't we explain that we're skipping these in the ChangeLog.

[Javier Fernandez]

Comment on attachment 331356 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlockFlow.cpp:3061
>> +            // case of verticalLR mode, so we can assume verticalRL for now.
> 
> Nit: This could be just one line.

ok

>> LayoutTests/ChangeLog:30
>> +        * fast/text/emphasis-overlap-expected.txt:
> 
> Nit: I believe this ChangeLog is not accurate.

Yes, it seems some files are missing from the list.

>> LayoutTests/platform/ios-simulator/TestExpectations:59
>> +# These tests seem to fail due pixel rounding errors in the absolute positioned elements used as reference.
> 
> Nit: Shouldn't we explain that we're skipping these in the ChangeLog.

ok. It'd be also good to have a bug report to point to, but I wasn't able to find a simple test case to reproduce it.

[Javier Fernandez]

(In reply to Manuel Rego Casasnovas from comment #61)
> Comment on attachment 331356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331356&action=review
> 
> The patch LGTM.
> 
> Just a question about border-styles-vertical-lr.html test, for the new
> expected files
> I see color changes in the borders, is that expected?

Yes, it's expected because those colors are rendered differently now in gtk+. Since the PNGs are not used unless there is a diff in the expected.txt files, we haven't noticed such color differences in the PNGs.

[Javier Fernandez]

Created attachment 332756 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 332756 [details]
Patch

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

r=me

> Source/WebCore/rendering/RenderBlockFlow.cpp:3064
> +            if (style.isFlippedLinesWritingMode())
> +                lastBaseline = logicalHeight() - lastRootBox()->logicalBottom() + style.fontMetrics().ascent(lastRootBox()->baselineType());
> +            else
> +                lastBaseline = lastRootBox()->logicalTop() + style.fontMetrics().ascent(lastRootBox()->baselineType());

Nit: Maybe we can write this like:

    lastBaseline = style.isFlippedLinesWritingMode() ? logicalHeight() - lastRootBox()->logicalBottom() : lastRootBox()->logicalTop();
    lastBaseline += style.fontMetrics().ascent(lastRootBox()->baselineType());

[Javier Fernandez]

Created attachment 332759 [details]
Patch

[Javier Fernandez]

Created attachment 332769 [details]
Patch

[EWS Watchlist]

Comment on attachment 332769 [details]
Patch

Attachment 332769 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6295278

New failing tests:
fast/writing-mode/text-orientation-basic.html

[EWS Watchlist]

Created attachment 332790 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Javier Fernandez]

Created attachment 332794 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 332794 [details]
Patch

Clearing flags on attachment: 332794

Committed r227947: <https://trac.webkit.org/changeset/227947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37096952>

[Matt Lewis]

Test that were introduced with this patch:
  fast/inline-block/baseline-vertical-01.html 
  fast/inline-block/baseline-vertical-02.html 
  fast/inline-block/baseline-vertical-03.html
  fast/inline-block/baseline-vertical-04.html

are showing up as ImageOnlyFailures on iOS simulator.

Looks like you intended to mark them as failing for iOS Simulator. Judging by the EWS output, it should have been marked [ ImageOnlyFailure ] instead of just failure. Went ahead and fixed it for you in:
https://trac.webkit.org/changeset/227975/webkit