FF 3.6 RC, Layout: Empty tag adds strange gap

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jm4000, Assigned: roc)

Tracking

(Depends on: 1 bug, {regression, testcase})

Trunk
x86
Windows Vista
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 wanted)

Details

(Whiteboard: [3.6.x])

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.2) Gecko/20100105 Firefox/3.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.2) Gecko/20100105 Firefox/3.6 (.NET CLR 3.5.30729)

I have a testcase (see below) which shows a strange behaviour in FF 3.6 RC: 
Inside a floating element (called "button") I have an empty span (which represents the button-icon) and another span containing some text. 
If I place a blank inside the "icon"-tag FF 3.6 RC renders the markup as I would expect. If I *remove the blank* from the "icon"-tag, FF 3.6. RC adds a strange gap between the icon and the text, resulting in an *unexpected line break*. If I remove the floating, the strange gap stays but the line-break vanishes.

If I check the same test case with FF 3.5, FF 3, Opera 10, IE8 there is no additional gap.

Reproducible: Always

Steps to Reproduce:
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="de" lang="de">

<head>
<style>
.icon {
	background:red;	
	padding-left:16px;
}


.button {
	padding:5px 0;
	background-color:orange;
	color:black;
	display:block;
	float:left;
	font-weight:bold;
	width:auto;		
}

.text {
	background:green;
}


</style>

</head>
<body>

with blank
	<a href="#" class="button">  	
		<span class="icon"> </span>  	<!-- blank inside span -->
		<span class="text">text</span>  
	</a>

<br/> <br/> <br/>

no blank
	<a href="#" class="button">  	
		<span class="icon"></span>  	<!-- no blank inside span -->
		<span class="text">text</span>  
	</a>

</body>

</html>


Expected Results:  
Both rendered blocks should look the same
(Reporter)

Updated

8 years ago
Version: unspecified → 3.6 Branch
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20100110 Minefield/3.7a1pre

Regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c6a2d1342334&tochange=662379876512
Component: General → Layout
Keywords: regression, testcase
Product: Firefox → Core
QA Contact: general → layout
Version: 3.6 Branch → Trunk
Flags: blocking1.9.2?
Most likely caused by bug 495385.
Blocks: 495385
Assignee: nobody → roc
Created attachment 421130 [details]
reduced testcase

Looks like we're removing the frame for the space between the two spans, but we shouldn't be.
Hmm, no, that's not it. The frame is there. Something's wrong with the intrinsic width calculation.
http://www.w3.org/TR/CSS21/text.html says

As each line is laid out,
   1. If a space (U+0020) at the beginning of a line has 'white-space' set to 'normal', 'nowrap', or 'pre-line', it is removed.

It's unclear whether a span with padding causes a space after it to not be "at the beginning of the line". Intrinsic width calculation says "no", reflow says "yes".
This difference goes back to Firefox 3 at least.

Webkit, Opera and IE8 all agree that a span with padding does not cause spaces after it to be "at the beginning of the line".
nsInlineFrame::IsSelfEmpty explicitly checks for nonzero left and right margin, border and padding. Should we stop doing that?

To be honest, although the other browsers disagree, it seems odd to me that a space after nonzero padding still counts as "beginning of the line".
Roc: what do you mean that the difference goes back to Firefox 3? I thought the regression range pegs this on something that was fixed in June, and not back-ported to the 1.9.1 branch?

Do you think we should block/respin on this? My feeling is that it's a 3.6.x issue, but would like your opinion, too.

(confirming, as there's a testcase)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [3.6.x]
(In reply to comment #9)
> Roc: what do you mean that the difference goes back to Firefox 3? I thought the
> regression range pegs this on something that was fixed in June, and not
> back-ported to the 1.9.1 branch?

I'm not sure about the originally reported bug, but my reduced testcase should fit on one line no matter what we decide about the spec, and it doesn't in Firefox 3.0.

> Do you think we should block/respin on this? My feeling is that it's a 3.6.x
> issue, but would like your opinion, too.

I don't think we should respin.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
(In reply to comment #7)
> This difference goes back to Firefox 3 at least.
> 
> Webkit, Opera and IE8 all agree that a span with padding does not cause spaces
> after it to be "at the beginning of the line".

I presume you mean "not at the beginning of the line" (likewise in comment 6)?


(In reply to comment #8)
> nsInlineFrame::IsSelfEmpty explicitly checks for nonzero left and right margin,
> border and padding. Should we stop doing that?

Hmmm.  IsSelfEmpty might be used for other things, such as vertical-alignment code, that wants such things to be non-empty.  Though I haven't actually gone through the current users.

> To be honest, although the other browsers disagree, it seems odd to me that a
> space after nonzero padding still counts as "beginning of the line".

Perhaps.

Though maybe for common use cases like a dotted border around a link, perhaps it makes sense to ignore padding/border/margin for purposes of computing emptiness.
(In reply to comment #11)
> Hmmm.  IsSelfEmpty might be used for other things, such as vertical-alignment
> code, that wants such things to be non-empty.  Though I haven't actually gone
> through the current users.

I guess I'll have to look into rationalizing the whole IsEmpty setup.

> Though maybe for common use cases like a dotted border around a link, perhaps
> it makes sense to ignore padding/border/margin for purposes of computing
> emptiness.

You're right.
It seems to me that IsEmpty(), IsSelfEmpty() and CachedIsEmpty() are defined correctly to support margin-collapsing. An empty span with a border prevents margin-collapsing in Webkit, Opera and Gecko.

So we need to define a different kind of emptiness here.
Created attachment 421752 [details] [diff] [review]
fix

OK, this turned out to be simpler than I expected. nsLineLayout already has dedicated logic to determine whether a frame represents content that makes subsequent content "not at the start of the line". There is a special case for inline elements (pfd->mSpan non-null) that calls IsSelfEmpty. The only frames which induce spans in line layout are nsInlineFrame (and subclasses) and nsFirstLetterFrame. nsInlineFrame::IsSelfEmpty just checks for borders, margins and padding, which is exactly what we want to ignore here. nsFirstLetterFrame->IsSelfEmpty return PR_FALSE. In both cases, just not calling IsSelfEmpty is what we want.
Attachment #421752 - Flags: review?(dbaron)
Whiteboard: [3.6.x] → [3.6.x][needs review]
Comment on attachment 421752 [details] [diff] [review]
fix

r=dbaron
Attachment #421752 - Flags: review?(dbaron) → review+
Whiteboard: [3.6.x][needs review] → [3.6.x][needs landing]
http://hg.mozilla.org/mozilla-central/rev/170943fba62d
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [3.6.x][needs landing] → [3.6.x]
Comment on attachment 421752 [details] [diff] [review]
fix

fix for a layout regression
Attachment #421752 - Flags: approval1.9.2.1?
Comment on attachment 421752 [details] [diff] [review]
fix

a1922=beltzner
Attachment #421752 - Flags: approval1.9.2.2? → approval1.9.2.2+
Whiteboard: [3.6.x] → [3.6.x][needs 192 landing]
Did this cause bug 549184 ?
Whiteboard: [3.6.x][needs 192 landing] → [3.6.x]
We should back this out of 1.9.2 until bug 549184 is ready to land on 1.9.2.
Whiteboard: [3.6.x] → [3.6.x][needs 192 landing]
Whiteboard: [3.6.x][needs 192 landing] → [3.6.x]
Comment on attachment 421752 [details] [diff] [review]
fix

This will miss 1.9.2.2; someone should make sure to nominate the dependency as well for next time :(
Attachment #421752 - Flags: approval1.9.2.3?
Attachment #421752 - Flags: approval1.9.2.2-
Attachment #421752 - Flags: approval1.9.2.2+
status1.9.2: --- → wanted
Comment on attachment 421752 [details] [diff] [review]
fix

If you still want to land this one on 1.9.2 please create a combined patch that includes the regression fix, or request approval on both bugs when they are ready to go. Thanks.
Attachment #421752 - Flags: approval1.9.2.4? → approval1.9.2.4-

Updated

7 years ago
Depends on: 647542
You need to log in before you can comment on or make changes to this bug.