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.
<rdar://problem/14152444>
Created attachment 205811 [details] test reduction
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;
I'd say post the hack patch and ask other rendering experts if they know a better way.
Created attachment 205885 [details] Patch
I'd like to see a bit more history in the Changelog.
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.
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.
(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.
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.
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.
(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.
Created attachment 206401 [details] Patch
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
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
looking at the regression reported by ews.
(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)
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
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
Comment on attachment 206401 [details] Patch new patch is coming up (fix the regressions)
Created attachment 206563 [details] Patch
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.
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?
(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.
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
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
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
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
Created attachment 206598 [details] Patch
Comment on attachment 206598 [details] Patch more rebaseline
Comment on attachment 206598 [details] Patch r=me
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
Created attachment 206880 [details] Patch
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
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
(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)
Comment on attachment 206880 [details] Patch Clearing flags on attachment: 206880 Committed r152793: <http://trac.webkit.org/changeset/152793>
All reviewed patches have been landed. Closing bug.