whitespace before <BR> should be ignored (trailing whitespace at end of line should be collapsed in white-space:normal)

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Layout: Text
P2
normal
RESOLVED FIXED
16 years ago
4 years ago

People

(Reporter: Jeff Jackowski, Assigned: bz)

Tracking

(Depends on: 1 bug, {testcase})

Trunk
mozilla1.9alpha1
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.9) Gecko/20020311
BuildID:    2002031115

The last line of a table cell made with "align=right" includes an extra
whitespace on the right when the code "<BR>&nbsp;</TD>" is all on the same line.
When the code is in the same order, but on at least two lines in the HTML file,
no extra whitespace is rendered. The problem happens on Linux (build listed) and
on OS/2 (using an 0.9.9 version build). MSIE v5 does not exhibit the problem.

Reproducible: Always
Steps to Reproduce:
1. Load page with proper HTML code.
(Reporter)

Comment 1

16 years ago
Created attachment 75381 [details]
Shows the bug, how to make it happen, and how to work around it in HTML
(Reporter)

Comment 2

16 years ago
I just realized that to avoid the bug, the <BR>&nbsp; code must not have a space
bewteen it and the cell content; the part about being on a seporate line from
</TD> isn't relavent. For instance:
---
<TD>
text<BR>&nbsp;</TD>
--
avoids the bug, while
---
<TD>
text <BR>&nbsp;
</TD>
--
does not avoid the bug. All other details of the HTML code are in the first
attachment to this bug report.
Hmm... should a space before <br> be rendered?

In any case, this is layout, not tables.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: qawanted, testcase

Comment 4

16 years ago
Created attachment 75507 [details]
Testcase

Space before <BR> testcase.
Attachment #75381 - Attachment is obsolete: true

Comment 5

16 years ago
-> Layout
Assignee: karnaze → attinasi
Component: HTMLTables → Layout
QA Contact: amar → petersen

Updated

16 years ago
Priority: -- → P4
Target Milestone: --- → Future

Comment 6

16 years ago
Changing QA contact
QA Contact: petersen → moied

Updated

16 years ago
Keywords: qawanted
*** Bug 229408 has been marked as a duplicate of this bug. ***
Assignee: attinasi → nobody
Component: Layout → Layout: Fonts and Text
QA Contact: moied → core.layout.fonts-and-text
Summary: Improper whitespace in last line of cell → whitespace before <BR> should be ignored
Severity: minor → normal
OS: Linux → All
Priority: P4 → --
Hardware: PC → All
Target Milestone: Future → ---
Summary: whitespace before <BR> should be ignored → whitespace before <BR> should be ignored (trailing whitespace at end of line should be collapsed in white-space:normal)
Created attachment 141306 [details] [diff] [review]
This fixes it, actually...
Comment on attachment 141306 [details] [diff] [review]
This fixes it, actually...

Er, actually no. Totally wrong patch....
Attachment #141306 - Attachment is obsolete: true
Created attachment 141353 [details] [diff] [review]
This is the right thing

I was running out of flags for a bit there.... ;)
Comment on attachment 141353 [details] [diff] [review]
This is the right thing

David, what do you think?
Attachment #141353 - Flags: superreview?(dbaron)
Attachment #141353 - Flags: review?(dbaron)

Comment 12

14 years ago
If I am the David addressed in comment #11, I can't evaluate this.  I am a
software tester (retired), not a programmer.  I cannot verify the code.  

I only download and install release candidates and final versions of Mozilla. 
(I have a dial-up modem, and it takes too long to download Mozilla for me to
bother with alpha and beta versions.)  However, I can make an exception; if you
really want me to test it, send me E-mail.  

I do have a test case at <http://www.rossde.com/test/test_br.html>.  Browse that
page in a "normal" window (i.e., a window not maximized).  As you slowly make
the window more narrow, the lines in the inner box of the first box will begin
to wrap (indicated when the word "end" appears at the beginning of a line). 
Just before a line wraps, a blank line should appear below it if the bug is NOT
fixed.  I know this test case demonstrates the bug when my Preferences specify
my proportional font as serif sized at 11 or 12 and my default serif font as
Georgia.  
Oh, I didn't realize there was a David cced on the bug.  ;)  The question was
directed at David Baron (the guy I asked for review).
An I'm not sure this patch will fix that testcase; this code may be later in the
linebreaking, once the space and <br> are on a separate line....
Comment on attachment 141353 [details] [diff] [review]
This is the right thing

r+sr=dbaron, but fix the comment in the last chunk of the patch
Attachment #141353 - Flags: superreview?(dbaron)
Attachment #141353 - Flags: superreview+
Attachment #141353 - Flags: review?(dbaron)
Attachment #141353 - Flags: review+
Assignee: nobody → bzbarsky
Priority: -- → P2
Summary: whitespace before <BR> should be ignored (trailing whitespace at end of line should be collapsed in white-space:normal) → [FIXr]whitespace before <BR> should be ignored (trailing whitespace at end of line should be collapsed in white-space:normal)
Target Milestone: --- → mozilla1.7beta
(In reply to comment #10)
> 
> I was running out of flags for a bit there.... ;)

Surely you mean you were running out of bits for a flag... ;-)
No, for a bit he ran out of bits for a bit. :-)
Fixed, for a 1.7b bit.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 19

14 years ago
bz, I think this change broke white space in HTML compose. Backing out this
checkin fixes the problem.

See Bug #235223 for more details...

this is a pretty severe regression for HTML compose so if you have a chance to
look at it quickly, I'd appreciate it. 
HTML compose broken on trunk for over a week is a smoketest blocker (maybe
people smoketest only their own default compose mode in mail, which might be
plain text). Let's back out this patch until we have a version of it that
doesn't regress HTML compose.  There's no reason to break something like HTML
compose for two weeks just to declare this bug fixed.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm confident we can fix this bug without regressing anything else for 1.7beta.
 I'll help drive any fixes needed into 1.7b and 1.7final if need be.  So don't
get me wrong -- I and other drivers want this bug fixed.  Just not at the
expense of HTML compose broken on the trunk for weeks.

/be
I have suggested a fix in that bug that would address the issue, no?  It's one
line of CSS that would need to get placed in whatever stylesheet is applied to
the mail composition document.  I am not familiar enough with the mail code to
know what stylesheet that is.

May I ask what the purpose of reopening this bug is, given that you have not
backed out the patch?

And per http://www.mozilla.org/quality/smoketests/, bug 235223 is not in fact a
smoketest blocker...
I should note that at this point I have spent more time on bug 235223 than I did
on this bug, time mostly spent repeating the same thing over and over again to
people who seem to be intent on ignoring it.
bz: where I come from, and in Mozilla when it's working well, the rule is "you
broke it, you fix it."  What kind of model of individual responsibility do you
have that says other people have to read your comments and do the actual work of
unregressing that bug?

I didn't back out your patch because I expect you to do that, or else fix the
regression, fast.  If you can collaborate with someone else who volunteers to
help unregress things, great.

I'm not intent on the means to the end, but we need working HTML compose on the
trunk by tomorrow.  But expecting others to take your partial work in bug 235223
and complete a fix from it is simply not being responsible for your own work and
its direct effects.  It doesn't matter how much time you spent.

As for "not a smoketest blocker", Mail test M.1 says to HTML-compose a message
and send it.  I'm told you can't type a space between two words in such a
message.  If I'm misinformed, please say how.  If not, then tell me why the
regression is not a smoketest blocker?

/be
> the rule is "you broke it, you fix it."

Yes.  And as soon as I have a large block of time to find the style sheet we
apply to the mail compose window, I will.  In the meantime, I provided enough
information that someone familiar with mail could have fixed the bug very
quickly if they were interested in fixing the bug.

> I'm told you can't type a space between two words in such a
> message.  If I'm misinformed, please say how.

You're misinformed.  Please see bug 235223 comment 10, bug 235223 comment 0, and
bug 235223 comment 12.  All that happens is if you type a space at the end of
the line the space doesn't show up until you type the next char.  Most people
probably even don't notice it... I know I did not (and I did compose some HTML
mail with the build that had the patch in it).
I will not have time to work on this in the foreseeable future.
Assignee: bzbarsky → nobody
Status: REOPENED → NEW
Priority: P2 → --
Summary: [FIXr]whitespace before <BR> should be ignored (trailing whitespace at end of line should be collapsed in white-space:normal) → whitespace before <BR> should be ignored (trailing whitespace at end of line should be collapsed in white-space:normal)
Target Milestone: mozilla1.7beta → ---

Comment 27

14 years ago
>Most people
>probably even don't notice it... I know I did not (and I did compose some HTML
>mail with the build that had the patch in it).

heh...sadly that's not true. People have been complaining in the MZ forums about
this regression since I first released a build that had the problem. There are
many threads in the thunderbird forums with folks complaining about why spaces
aren't working right in compose anymore :)

Scott, I don't think anyone was doubting you on that count....
bz: ok, not a smoketest blocker, but try typing an HTML message and watching as
you type -- it ain't pretty.

The patch here will be re-landed by mscott, I'm pretty sure he'll do the work,
provided someone can say in bug 235223 how to bind the style rule bz proposed to
the editor instance used by HTML mail compose.

/be

Updated

14 years ago
No longer blocks: 239477

Comment 31

13 years ago
Created attachment 171890 [details]
testcase without table

This does not only show up with tables.  I've created a very simple testcase
that displays the problem with right-aligned text.  If you set right-align via
CSS on two lines separated by a newline and <br>, the newline gets rendered as
a space, thus breaking alignment.

I have been discussing this with James Ross on IRC and apparently this behavior
is per spec, see

http://www.w3.org/TR/html4/appendix/notes.html#notes-line-breaks

which says that linebreaks immediately after a start tag and immediately before
an end tag must be ignored, and
 
http://www.w3.org/TR/html4/struct/text.html#h-9.1

which says that sequences of whitespace characters should be collapsed into a
single space character for latin scripts.

Thus Firefox (20050119) is behaving strictly per spec when converting a
linebreak followed by <br> into a space and a linebreak.  Nevertheless this
behavior is confusing since effectively two whitespace characters are rendered.
 I would expect the right alignment to be honored.

Other browsers (IE6SP2, Opera 7.50) do not render the space.
The HTML spec is talking about what the DOM should look like, not what the
rendering is supposed to look like.  The latter is controlled by the CSS spec,
and we get it wrong.

Comment 33

13 years ago
Reference comment #31:  
Try the test case I provide in comment #12.  

For the first box, I do not believe the spec supports the insertion of a blank
line in the rendering.  Yet that is what I get as the window slowly becomes
narrow.  

For the pairs of "text" at the very bottom, both pairs were coded the same with
one exception.  The first pair was coded all on one line.  The second pair was
coded in two lines.  I'm not sure Section B.3.1 of the HTML 4.01 specification
applies here since the line-break is immediately before (not after) the <br>
tag.  However, Section 9.3.2 says: "The BR element forcibly breaks (ends) the
current line of text."  Thus, it might be reasonable to treat the <br> as an
end-tag for this situation, which makes Section B.3.1 applicable.   

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.5) Gecko/20041217
If the reflow branch lands before this does, this patch should add a virtual
function override, BRFrame::AddInlinePrefWidth, that looks just like the one I'm
adding there to nsPlaceholderFrame.
Assignee: nobody → bzbarsky
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
From bug 235223, comment 53:
"This patch and the one in bug 132561 should be able to go in once bug 287813
lands."
So I guess the patch in this bug is now ready to be checked in?
Created attachment 219230 [details] [diff] [review]
Updated to tip
Attachment #141353 - Attachment is obsolete: true
Checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #171890 - Attachment mime type: text/plain → text/html

Updated

12 years ago
Depends on: 333560

Updated

12 years ago
Depends on: 336408

Comment 38

12 years ago
In which version of Gecko is this fixed?  This still seems to be a problem in Gecko 1.8.0.4/20060516.  
It's fixed on the trunk, which is what will eventually be branded as Gecko 1.9

Comment 40

11 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070111 SeaMonkey/1.1

This is still a problem.  See comment #12 for a test case.  View with Georgia font at 13 pixels.  If you slowly resize the window, the first box of "text text text . . . end" will occasionally show blank lines as a result of wrapping.  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 41

11 years ago
David: see comment 39.  SeaMonkey 1.1.x is based on of the Gecko 1.8.1.x.  SeaMonkey 1.5 will be based on Gecko 1.9.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED

Comment 42

11 years ago
FYI, the 20060420 check-in seems to have caused bug 354778.
Depends on: 354778

Updated

9 years ago
Duplicate of this bug: 312718
You need to log in before you can comment on or make changes to this bug.