Make positioned elements work with vertical text.
<rdar://problem/8612051>
Created attachment 83517 [details] Part 1: Make containing block width/height for positioned elements work with writing modes.
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.
(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.)
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.
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.
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
Created attachment 83537 [details] Part 2: Add logical accessors for left/right/top/bottom to RenderStyle
Attachment 83537 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7985238
Created attachment 83544 [details] Part 3: Patch computePositionedLogicalWidth to be writing-mode-aware.
Created attachment 83550 [details] Part 4: Patch computePositionedLogicalWidthUsing to be writing-mode aware.
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.
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.
Created attachment 83552 [details] Part 5: Patch computePositionedLogicalHeight to be writing-mode-aware.
Created attachment 83560 [details] Part 6: Patch computePositionedLogicalHeightUsing to be writing-mode aware.
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
(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.
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
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?
Rebaselined Mac expectations in http://trac.webkit.org/changeset/79532 per James' comment.
Created attachment 83713 [details] Part 7: Fixes for vertical-rl positioning and finally some tests!
Created attachment 83715 [details] Part 7: Fixes for vertical-rl positioning and finally some tests!
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.
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).
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
(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
Created attachment 83829 [details] Part 8: Patch computePositionedLogicalWidthReplaced to be writing-mode aware.
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.
Created attachment 83840 [details] Part 9: Patch replaced element height computation and add tests for positioned replaced elements.
Created attachment 83841 [details] Part 9: Patch replaced element height computation and add tests for positioned replaced elements.
Created attachment 83864 [details] Part 10: Make sure positioned objects with explicit positions work properly across mixed writing mode boundaries.
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.
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.
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
(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
(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.
Created attachment 83928 [details] rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html
(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.
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.
(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?
Committed r79760: <http://trac.webkit.org/changeset/79760>
http://trac.webkit.org/changeset/79760 might have broken Leopard Intel Debug (Build)
(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.
(In reply to comment #41) > Committed r79760: <http://trac.webkit.org/changeset/79760> Thanks for landing the patch!
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
Comment on attachment 83928 [details] rebaseline fast/table/fixed-with-auto-with-colspan-vertical.html This patch has been landed.
Created attachment 84123 [details] Part 11: Make the static distance computations when left/right/top/bottom are auto writing mode aware.
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.
All parts landed.