Closed
Bug 538935
Opened 15 years ago
Closed 15 years ago
FF 3.6 RC, Layout: Empty tag adds strange gap
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
| Tracking | Status | |
|---|---|---|
| status1.9.2 | --- | wanted |
People
(Reporter: jm4000, Assigned: roc)
References
(Depends on 1 open bug)
Details
(Keywords: regression, testcase, Whiteboard: [3.6.x])
Attachments
(3 files)
|
909 bytes,
text/html
|
Details | |
|
246 bytes,
text/html
|
Details | |
|
2.63 KB,
patch
|
dbaron
:
review+
beltzner
:
approval1.9.2.2-
dveditz
:
approval1.9.2.4-
|
Details | Diff | Splinter Review |
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
|
||
Comment 1•15 years ago
|
||
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
|
|
||
Comment 2•15 years ago
|
||
|
|
||
Updated•15 years ago
|
Flags: blocking1.9.2?
Most likely caused by bug 495385.
Blocks: 495385
|
Assignee | |
Updated•15 years ago
|
Assignee: nobody → roc
|
|
Assignee | |
Comment 4•15 years ago
|
||
Looks like we're removing the frame for the space between the two spans, but we shouldn't be.
|
|
Assignee | |
Comment 5•15 years ago
|
||
Hmm, no, that's not it. The frame is there. Something's wrong with the intrinsic width calculation.
|
|
Assignee | |
Comment 6•15 years ago
|
||
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".
|
|
Assignee | |
Comment 7•15 years ago
|
||
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".
|
|
Assignee | |
Comment 8•15 years ago
|
||
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".
|
||
Comment 9•15 years ago
|
||
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]
|
|
Assignee | |
Comment 10•15 years ago
|
||
(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.
|
|
||
Updated•15 years ago
|
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.
|
|
Assignee | |
Comment 12•15 years ago
|
||
(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.
|
|
Assignee | |
Comment 13•15 years ago
|
||
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.
|
|
Assignee | |
Comment 14•15 years ago
|
||
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)
|
|
Assignee | |
Updated•15 years ago
|
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+
|
|
Assignee | |
Updated•15 years ago
|
Whiteboard: [3.6.x][needs review] → [3.6.x][needs landing]
|
|
Assignee | |
Comment 16•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [3.6.x][needs landing] → [3.6.x]
|
|
Assignee | |
Comment 17•15 years ago
|
||
Comment on attachment 421752 [details] [diff] [review]
fix
fix for a layout regression
Attachment #421752 -
Flags: approval1.9.2.1?
|
|
||
Comment 18•15 years ago
|
||
Comment on attachment 421752 [details] [diff] [review]
fix
a1922=beltzner
Attachment #421752 -
Flags: approval1.9.2.2? → approval1.9.2.2+
|
|
Assignee | |
Updated•15 years ago
|
Whiteboard: [3.6.x] → [3.6.x][needs 192 landing]
|
||
Comment 19•15 years ago
|
||
Did this cause bug 549184 ?
|
|
Assignee | |
Updated•15 years ago
|
Whiteboard: [3.6.x][needs 192 landing] → [3.6.x]
|
||
Comment 20•15 years ago
|
||
status1.9.2:
--- → .2-fixed
|
|
Assignee | |
Comment 21•15 years ago
|
||
We should back this out of 1.9.2 until bug 549184 is ready to land on 1.9.2.
|
|
Assignee | |
Updated•15 years ago
|
Whiteboard: [3.6.x] → [3.6.x][needs 192 landing]
Backed out of 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e3af803ff889
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/61e5436ac87c
status1.9.2:
.2-fixed → ---
|
|
Assignee | |
Updated•15 years ago
|
Whiteboard: [3.6.x][needs 192 landing] → [3.6.x]
|
|
||
Comment 23•15 years ago
|
||
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+
|
||
Updated•15 years ago
|
status1.9.2:
--- → wanted
|
|
||
Comment 24•15 years ago
|
||
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-
You need to log in
before you can comment on or make changes to this bug.
Description
•