Bug 118245

Summary:

Wrong linebox height, when block element parent has vertical-align property defined.

Product:

WebKit

Reporter:

zalan <zalan>

Component:

Layout and Rendering

Assignee:

zalan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, ddkilzer, esprehn+autocc, glenn, hyatt, kling, mitz, mjs, rniwa, robert, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar, Regression

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
test reduction	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2	none
Patch	none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2	none
Patch	none
Patch	none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2	none

zalan

Reported 2013-07-01 07:09:20 PDT

While vertical-align property only applies to inline elements, if we put it to a block element (<div>), it impacts the child inline element's height. (when the child inline box has a different vertical-align value and is empty) <!DOCTYPE html> <html> <body> <div style="width: 57px; vertical-align: top;"> <span></span><input type="submit"> </div> </body> </html> This renders an empty line for the <span></span> and forces the input to the next line. (FF disagrees) The empty line disappears when 1, vertical-align is removed from <div> 1, same vertical-align is added to the <span> http://trac.webkit.org/changeset/108111 changes this behaviour. It checks whether the content needs extra linebox (requiresLineBoxForContent) comparing the parent's vertical-align property with the child's and assumes that new linebox is required if they differ. With that condition, if both <div>(block) and child <span>(inline) have vertical-align, different combinations of types/values, results in odd renderings.

Attachments
test reduction (203 bytes, text/html) 2013-07-01 07:13 PDT, zalan	no flags	Details
Patch (4.79 KB, patch) 2013-07-02 03:56 PDT, zalan	no flags	Details Formatted Diff Diff
Patch (6.08 KB, patch) 2013-07-10 11:42 PDT, zalan	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (603.68 KB, application/zip) 2013-07-11 02:51 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (500.28 KB, application/zip) 2013-07-12 07:13 PDT, Build Bot	no flags	Details
Patch (11.37 KB, patch) 2013-07-12 12:32 PDT, zalan	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (980.60 KB, application/zip) 2013-07-12 16:03 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (618.02 KB, application/zip) 2013-07-12 19:20 PDT, Build Bot	no flags	Details
Patch (97.26 KB, patch) 2013-07-13 02:11 PDT, zalan	no flags	Details Formatted Diff Diff
Patch (97.97 KB, patch) 2013-07-17 04:06 PDT, zalan	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (496.38 KB, application/zip) 2013-07-17 04:54 PDT, Build Bot	no flags	Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

zalan

Comment 1 2013-07-01 07:10:10 PDT

<rdar://problem/14152444>

zalan

Comment 2 2013-07-01 07:13:54 PDT

Created attachment 205811 [details] test reduction

zalan

Comment 3 2013-07-01 12:23:36 PDT

A hack like this fixes the issue, but I'm sure there's a better way if ((parent->isInline() && flow->style()->verticalAlign() != parent->style()->verticalAlign()) || (!parent->isInline() && flow->style()->verticalAlign() != BASELINE) return true;

Maciej Stachowiak

Comment 4 2013-07-01 14:09:30 PDT

I'd say post the hack patch and ask other rendering experts if they know a better way.

zalan

Comment 5 2013-07-02 03:56:28 PDT

Created attachment 205885 [details] Patch

Simon Fraser (smfr)

Comment 6 2013-07-02 17:17:56 PDT

I'd like to see a bit more history in the Changelog.

Dave Hyatt

Comment 7 2013-07-03 09:57:17 PDT

Comment on attachment 205885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205885&action=review r- > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2511 > + if ((parent->isInline() && flow->style()->verticalAlign() != parent->style()->verticalAlign()) > + || (!parent->isInline() && flow->style()->verticalAlign() != BASELINE)) > return true; Is this really a block issue? I'm suspicious. <!DOCTYPE html> <html> <head> <style> .va { width: 30px; background: green; vertical-align: 100px; } </style> </head> <body> <div>This tests if the vertical-align on the block element is ignored, when the child inline element is empty</div> <div><span class="va"><span></span></span><input type="submit"></div> </body> </html> The above also looks like a regression to me from shipping Safari, and it involves only spans. Looking into the code some more, it looks like: && !alwaysRequiresLineBox(toRenderInline(it.m_obj), lineInfo) && !requiresLineBoxForContent(toRenderInline(it.m_obj), lineInfo) might be the problem. If you're an empty inline, then you return false from alwaysRequiresLineBox, and so that condition of the and is now true. Maybe I'm missing something, but I would have thought an empty inline should never apply line-height or vertical-alignment. It sure seems like the above two functions should simply merge, with the empty-without-borders-padding condition just causing requiresLineBoxForContent to return false. It does look like before the patch we were allowing line-height to apply to empty inlines and trump them, but I'm not sure that was correct. Maybe Robert can show a counter-example. I don't think empty inlines should ever get line boxes (unless they have border/padding that make them non-empty of course). Anyway, I don't think that's the issue. I think we just want to treat the vertical-align values of "top" and "bottom" as though they are not equivalent, since I think you could cause the same bug to occur with nested inlines.

Dave Hyatt

Comment 8 2013-07-03 09:58:51 PDT

Ignore "Anyway, I don't think that's the issue. I think we just want to treat the vertical-align values of "top" and "bottom" as though they are not equivalent, since I think you could cause the same bug to occur with nested inlines." That was a stale comment that I didn't mean to include.

zalan

Comment 9 2013-07-04 11:12:09 PDT

(In reply to comment #7) > (From update of attachment 205885 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205885&action=review > > r- > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2511 > > + if ((parent->isInline() && flow->style()->verticalAlign() != parent->style()->verticalAlign()) > > + || (!parent->isInline() && flow->style()->verticalAlign() != BASELINE)) > > return true; > > Is this really a block issue? I'm suspicious. > > <!DOCTYPE html> > <html> > <head> > <style> > .va { > width: 30px; > background: green; > vertical-align: 100px; > } > </style> > </head> > <body> > <div>This tests if the vertical-align on the block element is ignored, when the child inline element is empty</div> > <div><span class="va"><span></span></span><input type="submit"></div> > </body> > </html> > > The above also looks like a regression to me from shipping Safari, and it involves only spans. > > Looking into the code some more, it looks like: > > && !alwaysRequiresLineBox(toRenderInline(it.m_obj), lineInfo) && !requiresLineBoxForContent(toRenderInline(it.m_obj), lineInfo) > > might be the problem. If you're an empty inline, then you return false from alwaysRequiresLineBox, and so that condition of the and is now true. Maybe I'm missing something, but I would have thought an empty inline should never apply line-height or vertical-alignment. It sure seems like the above two functions should simply merge, with the empty-without-borders-padding condition just causing requiresLineBoxForContent to return false. > > It does look like before the patch we were allowing line-height to apply to empty inlines and trump them, but I'm not sure that was correct. Maybe Robert can show a counter-example. I don't think empty inlines should ever get line boxes (unless they have border/padding that make them non-empty of course). > My thinking (and my original patch) was along the same lines, but that introduced a layouttest regression on the fast/css/non-empty-span.html (the naming is a bit misleading as the span element is actually empty, but the line is not) added by http://trac.webkit.org/changeset/108111. Interestingly, Opera ignores those properties (vertical-align, line-height and font-size) on empty inlines , unless margin/border/padding is present as opposed to FF. FF behaves like Safari atm. Scanning through bug 76465, I couldn't really figure out why it was done that way (matching FF?), but surely I am missing some irc discussion.

Dave Hyatt

Comment 10 2013-07-05 12:10:45 PDT

The CSS spec, section 10.8, states the following: "Empty inline elements generate empty inline boxes, but these boxes still have margins, padding, borders and a line height, and thus influence these calculations just like elements with content." There is no mention of vertical-alignment influencing calculations, but if line height does, my personal expectation would be that vertical-align should influence as well. I really think looking at this more that vertical-alignment is a red herring. Consider the following test case: <!DOCTYPE html> <html> <body> <div style="width: 59px; padding:1px"><span style="padding:1px"></span><input type="submit"></div> </body> </html> All I did was increase the width by 2px to account for the 2px-wide span. This is the same principle as the test case you opened the bug with, in that the presence of an empty inline seems to cause the content to think it has to break to the next line, even though it should be able to fit. In other words, all vertical-align did was caused the span to get a line box. The fact that this line box is somehow disrupting the rendering you expect is the real issue here, not the vertical alignment itself.

Dave Hyatt

Comment 11 2013-07-08 11:18:49 PDT

My complete guess (I'd have to study the code to make sure) is that the button doesn't fit even on the first line in your initial test case. Therefore when a break opportunity gets pushed before the button because of a span with a line box, we try to put the button on the next line hoping it will fit there. Assuming this theory is correct, the fix is probably to just ignore break opportunities at the start of a line if no width has been committed yet.

zalan

Comment 12 2013-07-09 04:54:49 PDT

(In reply to comment #11) > My complete guess (I'd have to study the code to make sure) is that the button doesn't fit even on the first line in your initial test case. Therefore when a break opportunity gets pushed before the button because of a span with a line box, we try to put the button on the next line hoping it will fit there. > > Assuming this theory is correct, the fix is probably to just ignore break opportunities at the start of a line if no width has been committed yet. I believe this assumption is correct. I'll poke around that area.

zalan

Comment 13 2013-07-10 11:42:32 PDT

Created attachment 206401 [details] Patch

Build Bot

Comment 14 2013-07-11 02:51:55 PDT

Comment on attachment 206401 [details] Patch Attachment 206401 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1052269 New failing tests: http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html fast/text/midword-break-before-surrogate-pair.html fast/forms/textarea-metrics.html fast/overflow/overflow-focus-ring.html http/tests/inspector/inspect-element.html fast/forms/basic-textareas.html fast/text/hyphenate-first-word.html fast/text/whitespace/pre-break-word.html fast/text/embed-at-end-of-pre-wrap-line.html fast/text/midword-break-before-surrogate-pair-2.html fast/loader/text-document-wrapping.html fast/text/word-break.html fast/text/hyphenate-locale.html fast/text/midword-break-after-breakable-char.html plugins/mouse-events.html fast/forms/basic-textareas-quirks.html editing/selection/modify-up-on-rtl-wrapping-text.html editing/selection/click-left-of-rtl-wrapping-text.html fast/exclusions/shape-inside/shape-inside-outside-shape.html fast/lists/inlineBoxWrapperNullCheck.html plugins/mouse-move-over-plugin-in-frame.html platform/mac/editing/deleting/deletionUI-single-instance.html fast/forms/textarea-width.html fast/loader/javascript-url-in-object.html fast/text/hyphenate-first-word-after-skipped-space.html fast/text/whitespace/tab-character-basics.html fast/text/basic/015.html

Build Bot

Comment 15 2013-07-11 02:51:58 PDT

Created attachment 206441 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.3

zalan

Comment 16 2013-07-11 12:29:41 PDT

looking at the regression reported by ews.

zalan

Comment 17 2013-07-12 05:50:23 PDT

(In reply to comment #14) > (From update of attachment 206401 [details]) > Attachment 206401 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/1052269 > > New failing tests: > http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html > fast/text/midword-break-before-surrogate-pair.html > fast/forms/textarea-metrics.html > fast/overflow/overflow-focus-ring.html > http/tests/inspector/inspect-element.html > fast/forms/basic-textareas.html > fast/text/hyphenate-first-word.html > fast/text/whitespace/pre-break-word.html > fast/text/embed-at-end-of-pre-wrap-line.html > fast/text/midword-break-before-surrogate-pair-2.html > fast/loader/text-document-wrapping.html > fast/text/word-break.html > fast/text/hyphenate-locale.html > fast/text/midword-break-after-breakable-char.html > plugins/mouse-events.html > fast/forms/basic-textareas-quirks.html > editing/selection/modify-up-on-rtl-wrapping-text.html > editing/selection/click-left-of-rtl-wrapping-text.html > fast/exclusions/shape-inside/shape-inside-outside-shape.html > fast/lists/inlineBoxWrapperNullCheck.html > plugins/mouse-move-over-plugin-in-frame.html > platform/mac/editing/deleting/deletionUI-single-instance.html > fast/forms/textarea-width.html > fast/loader/javascript-url-in-object.html > fast/text/hyphenate-first-word-after-skipped-space.html > fast/text/whitespace/tab-character-basics.html > fast/text/basic/015.html These regressions are due to the fact that the patch increments to the next break, when it shouldn't. (like when there's nothing but a long string on the line, it considers it empty and increments by one character)

Build Bot

Comment 18 2013-07-12 07:13:14 PDT

Comment on attachment 206401 [details] Patch Attachment 206401 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/971957 New failing tests: http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html fast/text/midword-break-before-surrogate-pair.html fast/forms/textarea-metrics.html fast/overflow/overflow-focus-ring.html http/tests/inspector/inspect-element.html fast/forms/basic-textareas.html fast/text/hyphenate-first-word.html fast/text/whitespace/pre-break-word.html fast/forms/textarea-width.html fast/text/midword-break-before-surrogate-pair-2.html fast/loader/text-document-wrapping.html editing/pasteboard/copy-image-with-alt-text.html fast/text/word-break.html fast/text/hyphenate-locale.html fast/text/midword-break-after-breakable-char.html fast/forms/basic-textareas-quirks.html editing/selection/modify-up-on-rtl-wrapping-text.html fast/lists/inlineBoxWrapperNullCheck.html fast/text/embed-at-end-of-pre-wrap-line.html fast/exclusions/shape-inside/shape-inside-outside-shape.html editing/selection/click-left-of-rtl-wrapping-text.html fullscreen/full-screen-iframe-with-max-width-height.html fast/text/hyphenate-first-word-after-skipped-space.html fast/text/whitespace/tab-character-basics.html fast/text/basic/015.html

Build Bot

Comment 19 2013-07-12 07:13:16 PDT

Created attachment 206543 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

zalan

Comment 20 2013-07-12 11:04:34 PDT

Comment on attachment 206401 [details] Patch new patch is coming up (fix the regressions)

zalan

Comment 21 2013-07-12 12:32:13 PDT

Created attachment 206563 [details] Patch

zalan

Comment 22 2013-07-12 12:34:41 PDT

LayoutTests/fast/lists/inlineBoxWrapperNullCheck.html is actually a progression (both FF and Opera agree with the new rendering). I'll fix the other platforms' -expected, soon after it gets reviewed.

Dave Hyatt

Comment 23 2013-07-12 12:46:07 PDT

Comment on attachment 206563 [details] Patch Looks good to me. Is it intentional that none of this work gets done at all if the segment does not allow overflow? Before you would at least do an increment. Now you won't. Will this get you in trouble?

zalan

Comment 24 2013-07-12 13:54:31 PDT

(In reply to comment #23) > (From update of attachment 206563 [details]) > Looks good to me. Is it intentional that none of this work gets done at all if the segment does not allow overflow? Before you would at least do an increment. Now you won't. Will this get you in trouble? Thanks, I'll check, but AFAICT each increment was guarded with segmentAllowsOverflow before. I just rearranged the code a bit and added that empty line check.

Build Bot

Comment 25 2013-07-12 16:03:30 PDT

Comment on attachment 206563 [details] Patch Attachment 206563 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/962460 New failing tests: tables/mozilla/marvin/backgr_index.html svg/batik/filters/feTile.svg

Build Bot

Comment 26 2013-07-12 16:03:35 PDT

Created attachment 206577 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.3

Build Bot

Comment 27 2013-07-12 19:20:37 PDT

Comment on attachment 206563 [details] Patch Attachment 206563 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1067091 New failing tests: tables/mozilla/marvin/backgr_index.html

Build Bot

Comment 28 2013-07-12 19:20:41 PDT

Created attachment 206584 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

zalan

Comment 29 2013-07-13 02:11:46 PDT

Created attachment 206598 [details] Patch

zalan

Comment 30 2013-07-13 02:12:22 PDT

Comment on attachment 206598 [details] Patch more rebaseline

Dave Hyatt

Comment 31 2013-07-16 09:52:52 PDT

Comment on attachment 206598 [details] Patch r=me

David Kilzer (:ddkilzer)

Comment 32 2013-07-16 13:40:12 PDT

Comment on attachment 206598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206598&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:3527 > + // Behave as if it was actually empty and cosume at least one object. Typo: cosume -> consume

zalan

Comment 33 2013-07-17 04:06:55 PDT

Created attachment 206880 [details] Patch

Build Bot

Comment 34 2013-07-17 04:54:30 PDT

Comment on attachment 206880 [details] Patch Attachment 206880 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1086452 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html

Build Bot

Comment 35 2013-07-17 04:54:32 PDT

Created attachment 206883 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

zalan

Comment 36 2013-07-17 06:25:44 PDT

(In reply to comment #34) > (From update of attachment 206880 [details]) > Attachment 206880 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/1086452 > > New failing tests: > fullscreen/full-screen-iframe-with-max-width-height.html This fails even with my fix removed. (and interestingly the previous patch passed the wk2 bot and there's only grammar fix diff in the latest patch)

Andreas Kling

Comment 37 2013-07-17 11:45:28 PDT

Comment on attachment 206880 [details] Patch Clearing flags on attachment: 206880 Committed r152793: <http://trac.webkit.org/changeset/152793>

Andreas Kling

Comment 38 2013-07-17 11:45:44 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.