RESOLVED FIXED Bug 76465
REGRESSION: empty span creates renderers with non-zero height 
https://bugs.webkit.org/show_bug.cgi?id=76465
Summary REGRESSION: empty span creates renderers with non-zero height
Ryosuke Niwa
Reported 2012-01-17 11:26:53 PST
In the following markup, span between the divs is rendered as an empty line. <!DOCTYPE html> <html> <head></head> <body> <div>Before empty span</div> <span style="line-height:1.24;font-size:13px"></span> <div>After empty span</div> </body> </html> http://crbug.com/109811
Attachments
Patch (6.40 KB, patch)
2012-01-23 14:46 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (22.15 KB, patch)
2012-02-01 13:49 PST, Robert Hogan 		hyatt: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Robert Hogan
Comment 1 2012-01-17 14:53:19 PST
The culprit here is: if (!flow->document()->inQuirksMode() && flow->style(lineInfo.isFirstLine())->lineHeight() != flow->parent()->style(lineInfo.isFirstLine())->lineHeight()) return true; in RenderBlockLineLayout::inlineFlowRequiresLineBox(). It should only add a linebox when there is some other non-whitespace inline content. There doesn't seem an obvious way of doing this without inspecting the RenderInline's siblings, which would make the line layout very expensive.
Ryosuke Niwa
Comment 2 2012-01-17 14:55:58 PST
Alternatively, can we ignore these inline boxes when we're doing layout?
Robert Hogan
Comment 3 2012-01-18 12:26:51 PST
http://www.w3.org/TR/CSS21/visudet.html#line-height says: "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." And this appears to be what: return !flow->firstChild() && flow->hasInlineDirectionBordersPaddingOrMargin(); is implementing in inlineFlowRequiresLineBox() (except for lineHeight, which is covered by the line mentioned in comment #1). So is this an open question for the spec?
Tab Atkins
Comment 4 2012-01-18 15:25:47 PST
Look on that page, but down in section 9.4.2. Specifically, the paragraph starting with "Line boxes are created as needed...". The <span> creates a linebox, but it's then thrown away as it's empty. The presence of 'line-height' on the span shouldn't have any effect on this. The only properties that matter are margin, border, padding. (Plus the other inline-level display values - they always force the generation of a line box.)
Robert Hogan
Comment 5 2012-01-23 14:46:07 PST
Created attachment 123627 [details] Patch
Robert Hogan
Comment 6 2012-01-23 14:52:42 PST
Comment on attachment 123627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123627&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:1832 > + if (!flow->document()->inQuirksMode() && flow->style(lineInfo.isFirstLine())->lineHeight() != flow->parent()->style(lineInfo.isFirstLine())->lineHeight()) there's an extra whitespace in there.
Dave Hyatt
Comment 7 2012-01-30 10:55:15 PST
Comment on attachment 123627 [details] Patch This is all just a hack. I'm not necessarily averse to putting something in just to pass the test, but that's all this really is. As we discussed on IRC, where we seem to differ from other browsers is that they honor any impact an empty inline flow has on a line as long as the line is non-empty. This isn't unique to line-height, so that's why it's discouraging to have line-height-specific code like this in the patch. It applies to font-size as well, e.g., <span style="font-size:36px"></span> Hello world! I think the right way to fix this bug is simply to always collect runs for empty inlines (so get rid of inlineFlowRequiresLineBox and contributesLineHeight completely). Make sure they don't cause lineInfo.setEmpty(false) though except under the circumstances they used to. Then only if the line ends up being non-empty do you make the line boxes over in constructLine.
Dave Hyatt
Comment 8 2012-01-30 11:00:15 PST
Also we would only need to collect the extra inlines in standards mode, since quirks mode isn't going to pay any attention to them anyway.
Dave Hyatt
Comment 9 2012-01-30 11:10:07 PST
I think your patch is probably fine if you just rename the line height function and expand it to cover more cases, e.g., differing fonts, vertical-align, etc.
Robert Hogan
Comment 10 2012-02-01 13:49:22 PST
Created attachment 125009 [details] Patch
Robert Hogan
Comment 11 2012-02-11 02:30:11 PST
*** Bug 78379 has been marked as a duplicate of this bug. ***
Dave Hyatt
Comment 12 2012-02-16 11:41:54 PST
Comment on attachment 125009 [details] Patch r=me
Ryosuke Niwa
Comment 13 2012-02-17 12:17:19 PST
Hi Robert, could you land this patch ASAP? It's affecting Google products and folks want this fix be landed ASAP.
Robert Hogan
Comment 14 2012-02-17 13:11:28 PST
Committed r108111: <http://trac.webkit.org/changeset/108111>
Note You need to log in before you can comment on or make changes to this bug.