Bug 46500 – Make positioned elements work with vertical text

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

[Dave Hyatt]

Make positioned elements work with vertical text.

[Adele Peterson]

<rdar://problem/8612051>

[Dave Hyatt]

Created attachment 83517 [details]
Part 1: Make containing block width/height for positioned elements work with writing modes.

[Darin Adler]

Comment on attachment 83517 [details]
Part 1: Make containing block width/height for positioned elements work with writing modes.

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

> Source/WebCore/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=46500, make positioned elements work with vertical text.

Do you plan to add test cases?

> Source/WebCore/rendering/RenderBox.cpp:2077
> +        return containingBlockLogicalWidthForPositioned(containingBlock, false);

I hate booleans for this kind of thing! I usually ask people to use enums for something where you always are passing a constant to it.

[Dave Hyatt]

(In reply to comment #3)
> (From update of attachment 83517 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83517&action=review
> 
> > Source/WebCore/ChangeLog:5
> > +        https://bugs.webkit.org/show_bug.cgi?id=46500, make positioned elements work with vertical text.
> 
> Do you plan to add test cases?
> 

Yeah eventually.  Until I patch the levels above this, though, it's untestable.  (Positioned elements don't even show up yet if vertical text is used.)

[Simon Fraser (smfr)]

Comment on attachment 83517 [details]
Part 1: Make containing block width/height for positioned elements work with writing modes.

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

> Source/WebCore/rendering/RenderBox.cpp:2046
> +        return containingBlockLogicalHeightForPositioned(containingBlock, false);

An enum rather than a bool would make this more readable.

> Source/WebCore/rendering/RenderBox.cpp:2093
> +    IntRect boundingBox = toRenderInline(containingBlock)->linesBoundingBox();

Why not use 'flow' here?

> Source/WebCore/rendering/RenderBox.h:431
>  

Shame to add new bool params.

[Dave Hyatt]

I think the boolean is fine in this case, since the "public" callers don't use it, and its internal value is obvious given that the two functions are right there in the code next to each other.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79467 might have broken Qt Linux Release
The following tests are not passing:
fast/parser/test-unicode-characters-in-attribute-name.html

[Dave Hyatt]

Created attachment 83537 [details]
Part 2: Add logical accessors for left/right/top/bottom to RenderStyle

[WebKit Review Bot]

Attachment 83537 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7985238

[Dave Hyatt]

Created attachment 83544 [details]
Part 3: Patch computePositionedLogicalWidth to be writing-mode-aware.

[Dave Hyatt]

Created attachment 83550 [details]
Part 4: Patch computePositionedLogicalWidthUsing to be writing-mode aware.

[WebKit Review Bot]

Attachment 83550 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderBox.cpp:2307:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

http://trac.webkit.org/changeset/79485/ seems to have changed the render tree for fast/table/fixed-with-auto-with-colspan-vertical.html, here's a diff (from chromium linux):

--- /mnt/data/build/slave/Webkit_Linux/build/src/webkit/Release/../../../layout-test-results/fast/table/fixed-with-auto-with-colspan-vertical-expected.txt 
+++ /mnt/data/build/slave/Webkit_Linux/build/src/webkit/Release/../../../layout-test-results/fast/table/fixed-with-auto-with-colspan-vertical-actual.txt 
@@ -3,8 +3,8 @@
 layer at (0,0) size 800x600
   RenderBlock {HTML} at (0,0) size 800x600
     RenderBody {BODY} at (8,8) size 784x584
-layer at (0,0) size 385x0
-  RenderBlock (positioned) {DIV} at (0,0) size 385x0
+layer at (0,0) size 385x400
+  RenderBlock (positioned) {DIV} at (0,0) size 385x400
     RenderTable {TABLE} at (5,0) size 50x445
       RenderTableSection {TBODY} at (0,0) size 50x445
         RenderTableRow {TR} at (0,0) size 50x445
@@ -82,8 +82,8 @@
           RenderTableCell {TD} at (0,304) size 50x2 [bgcolor=#FF0000] [r=0 c=5 rs=1 cs=1]
           RenderTableCell {TD} at (0,359) size 50x2 [bgcolor=#FF0000] [r=0 c=6 rs=1 cs=1]
           RenderTableCell {TD} at (0,414) size 50x2 [bgcolor=#FF0000] [r=0 c=7 rs=1 cs=1]
-layer at (0,0) size 385x0
-  RenderBlock (positioned) {DIV} at (0,0) size 385x0
+layer at (0,0) size 385x430
+  RenderBlock (positioned) {DIV} at (0,0) size 385x430
     RenderTable {TABLE} at (5,0) size 50x445
       RenderTableSection {TBODY} at (0,0) size 50x445
         RenderTableRow {TR} at (0,0) size 50x445

is this intentional? it doesn't seem like a breakage.

[Dave Hyatt]

Created attachment 83552 [details]
Part 5: Patch computePositionedLogicalHeight to be writing-mode-aware.

[Dave Hyatt]

Created attachment 83560 [details]
Part 6: Patch computePositionedLogicalHeightUsing to be writing-mode aware.

[James Robinson]

As of 79497 it seems that fast/table/fixed-with-auto-with-colspan-vertical.html needs a render tree rebaseline, but is passing.  fast/repaint/text-emphasis-h.html seems genuinely broke still:

expected:
http://build.webkit.org/results/Chromium%20Linux%20Release%20(Tests)/r79498%20(13836)/fast/repaint/text-emphasis-h-expected.png

http://build.webkit.org/results/Chromium%20Linux%20Release%20(Tests)/r79498%20(13836)/fast/repaint/text-emphasis-h-actual.png

[James Robinson]

(In reply to comment #16)
> As of 79497 it seems that fast/table/fixed-with-auto-with-colspan-vertical.html needs a render tree rebaseline, but is passing.  fast/repaint/text-emphasis-h.html seems genuinely broke still:
> 
> expected:
> http://build.webkit.org/results/Chromium%20Linux%20Release%20(Tests)/r79498%20(13836)/fast/repaint/text-emphasis-h-expected.png
> 
> http://build.webkit.org/results/Chromium%20Linux%20Release%20(Tests)/r79498%20(13836)/fast/repaint/text-emphasis-h-actual.png

Actually safari/mac and chromium/mac appears to render this test correctly, so seems possible that it's some chromium bug on windows/linux or this exposes some system deficiency.

[Ryosuke Niwa]

emphasis-h-pretty-diff.html has been failing since http://build.webkit.org/builders/SnowLeopard%20Intel%20Leaks/builds/15088.

http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r79495%20(15088)/fast/repaint/text-emphasis-h-pretty-diff.html

[Ryosuke Niwa]

fast/table/fixed-with-auto-with-colspan-vertical.html also seems to be failing on Snow Leopard since these two patches are landed.

Should we just rebaseline them?

[Ryosuke Niwa]

Rebaselined Mac expectations in http://trac.webkit.org/changeset/79532 per James' comment.

[Dave Hyatt]

Created attachment 83713 [details]
Part 7: Fixes for vertical-rl positioning and finally some tests!

[Dave Hyatt]

Created attachment 83715 [details]
Part 7: Fixes for vertical-rl positioning and finally some tests!

[Simon Fraser (smfr)]

Comment on attachment 83715 [details]
Part 7: Fixes for vertical-rl positioning and finally some tests!

> vertical-lr/001-expected.png
> vertical-rl/001-expected.png
If you removed the text, this would be cross-platform. Is the black necessary?

> vertical-lr/002-expected.png
> vertical-rl/002-expected.png
Avoid the scrollbar in pixel results.

[Dave Hyatt]

The test cases are clones of their horizontal counterparts.  For my own testing at least, it's valuable that they be identical to the horizontal versions (even if the horizontal versions are badly written) so that I can easily see if the rendering is the same (just rotated).

[Sergio Villar Senin]

After r79618 these 3 tests started to fail in GTK bots

fast/table/colspanMinWidth-vertical.html
fast/table/fixed-with-auto-with-colspan-vertical.html
tables/mozilla_expected_failures/marvin/table_overflow_dirty_reflow_tbody.html

The last one fails inttermitently but the other two constantly.

Taking  a look at some diffs it does not seem to be a matter of a rebaseline:

- layer at (0,0) size 385x400
-   RenderBlock (positioned) {DIV} at (0,0) size 385x400
+ layer at (415,0) size 385x400
+  RenderBlock (positioned) {DIV} at (415,0) size 385x400

I'll skip these three for the moment

[Sergio Villar Senin]

(In reply to comment #25)
> After r79618 these 3 tests started to fail in GTK bots
> 
> fast/table/colspanMinWidth-vertical.html
> fast/table/fixed-with-auto-with-colspan-vertical.html
> tables/mozilla_expected_failures/marvin/table_overflow_dirty_reflow_tbody.html
> 
> The last one fails inttermitently but the other two constantly.
> 
> Taking  a look at some diffs it does not seem to be a matter of a rebaseline:
> 
> - layer at (0,0) size 385x400
> -   RenderBlock (positioned) {DIV} at (0,0) size 385x400
> + layer at (415,0) size 385x400
> +  RenderBlock (positioned) {DIV} at (415,0) size 385x400
> 
> I'll skip these three for the moment

:m actually looks like the rebaseline would be correct http://trac.webkit.org/changeset/79640

[Dave Hyatt]

Created attachment 83829 [details]
Part 8: Patch computePositionedLogicalWidthReplaced to be writing-mode aware.

[WebKit Review Bot]

Attachment 83829 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderBox.cpp:2794:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/RenderBox.cpp:2796:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Created attachment 83840 [details]
Part 9: Patch replaced element height computation and add tests for positioned replaced elements.

[Dave Hyatt]

Created attachment 83841 [details]
Part 9: Patch replaced element height computation and add tests for positioned replaced elements.

[Dave Hyatt]

Created attachment 83864 [details]
Part 10: Make sure positioned objects with explicit positions work properly across mixed writing mode boundaries.

[WebKit Review Bot]

Attachment 83864 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBox.cpp:2569:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihai Parparita]

This (http://trac.webkit.org/changeset/79725) broke fast/table/fixed-with-auto-with-colspan-vertical.html:

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r79725%20(25939)/results.html

I don't know enough about what's being tested to decide whether this is a progression or not.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79725 might have broken GTK Linux 32-bit Release, GTK Linux 32-bit Debug, and Qt Linux Release
The following tests are not passing:
fast/table/fixed-with-auto-with-colspan-vertical.html

[James Robinson]

(In reply to comment #33)
> This (http://trac.webkit.org/changeset/79725) broke fast/table/fixed-with-auto-with-colspan-vertical.html:
> 
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r79725%20(25939)/results.html
> 
> I don't know enough about what's being tested to decide whether this is a progression or not.

Pixel results (from chromium mac snow leopard) before:
http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r79746%20(5000)/fast/table/fixed-with-auto-with-colspan-vertical-expected.png

after:
http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r79746%20(5000)/fast/table/fixed-with-auto-with-colspan-vertical-actual.png

[Ryosuke Niwa]

(In reply to comment #35)
> Pixel results (from chromium mac snow leopard) before:
> http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r79746%20(5000)/fast/table/fixed-with-auto-with-colspan-vertical-expected.png
> 
> after:
> http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r79746%20(5000)/fast/table/fixed-with-auto-with-colspan-vertical-actual.png

This looks like a progression.  The table has position: absolute; left: 0px; top: 0px.

[Ryosuke Niwa]

Created attachment 83928 [details]
rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html

[Ryosuke Niwa]

(In reply to comment #37)
> Created an attachment (id=83928) [details]
> rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html

I think new result is better but I'm going to ask for a review since I'm not that familiar with CSS layout.  Note that this failure is currently blocking the commit queue to land patches so it's crucial that we do this rebaseline or fix the layout.

[Eric Seidel (no email)]

Comment on attachment 83928 [details]
rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html

Sigh.  The cq can't land this if its blocked.

[Ryosuke Niwa]

(In reply to comment #39)
> (From update of attachment 83928 [details])
> Sigh.  The cq can't land this if its blocked.

Oops! :(  I'm out of office already.  Would you be able to land my patch on behalf of me?

[Eric Seidel (no email)]

Committed r79760: <http://trac.webkit.org/changeset/79760>

[WebKit Review Bot]

http://trac.webkit.org/changeset/79760 might have broken Leopard Intel Debug (Build)

[Csaba Osztrogonác]

(In reply to comment #37)
> Created an attachment (id=83928) [details]
> rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html

Thanks for fixing it.

[Ryosuke Niwa]

(In reply to comment #41)
> Committed r79760: <http://trac.webkit.org/changeset/79760>

Thanks for landing the patch!

[WebKit Commit Bot]

Comment on attachment 83928 [details]
rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html

Rejecting attachment 83928 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
tTests/platform/mac/fast/table/fixed-with-auto-with-colspan-vertical-expected.txt.rej
patching file LayoutTests/platform/qt/fast/table/fixed-with-auto-with-colspan-vertical-expected.txt
Hunk #1 FAILED at 3.
Hunk #2 FAILED at 82.
2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/qt/fast/table/fixed-with-auto-with-colspan-vertical-expected.txt.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8019759

[Ryosuke Niwa]

Comment on attachment 83928 [details]
rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html

This patch has been landed.

[Dave Hyatt]

Created attachment 84123 [details]
Part 11: Make the static distance computations when left/right/top/bottom are auto writing mode aware.

[WebKit Review Bot]

Attachment 84123 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBlockLineLayout.cpp:1301:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1305:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1306:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 3 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 84123 [details]
Part 11: Make the static distance computations when left/right/top/bottom are auto writing mode aware.

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

I am a little confused by the handling of static inline position for RTL containing blocks. Maybe it was already wrong, but it looks like it’s changing. Aren’t there tests covering this?

> Source/WebCore/rendering/RenderBox.cpp:2461
> +    int staticTop = child->layer()->staticBlockPosition() - containerBlock->borderBefore();

May want to call this staticLogicalTop

> Source/WebCore/rendering/RenderFlexibleBox.cpp:420
> +                        child->layer()->setStaticBlockPosition(yPos);

Can use childLayer here too.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:679
> -                if (child->style()->hasStaticX()) {
> -                    if (style()->isLeftToRightDirection())
> -                        child->layer()->setStaticX(borderLeft()+paddingLeft());
> -                    else
> -                        child->layer()->setStaticX(borderRight()+paddingRight());
> -                }
> -                if (child->style()->hasStaticY()) {
> +                if (child->style()->hasStaticInlinePosition(style()->isHorizontalWritingMode()))
> +                    child->layer()->setStaticInlinePosition(borderLeft() + paddingLeft());

Was the RTL code wrong?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:683
> +                        child->layer()->setStaticBlockPosition(height());

Can use childLayer.

[Dave Hyatt]

All parts landed.