Bug 76465

Summary:

REGRESSION: empty span creates renderers with non-zero height

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

Layout and Rendering

Assignee:

Robert Hogan <robert>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alheureux, darin, hyatt, mitz, robert, tabatkins

Priority:

Keywords:

GoogleBug

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	hyatt: review+

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	Details Formatted Diff Diff
Patch (22.15 KB, patch) 2012-02-01 13:49 PST, Robert Hogan	hyatt: review+	Details Formatted Diff Diff
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.