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

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Jeff Jackowski]

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.

Comment 1

[Jeff Jackowski]

Attachment: Shows the bug, how to make it happen, and how to work around it in HTML

Comment 2

[Jeff Jackowski]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm... should a space before <br> be rendered?

In any case, this is layout, not tables.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase

Space before <BR> testcase.

Comment 5

[Mats Palmgren (inactive)]

-> Layout

Comment 6

[Christine Hoffman]

Changing QA contact

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 229408 has been marked as a duplicate of this bug. ***

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes it, actually...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141306 [details] [diff] [review]
This fixes it, actually...

Er, actually no. Totally wrong patch....

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: This is the right thing

I was running out of flags for a bit there.... ;)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141353 [details] [diff] [review]
This is the right thing

David, what do you think?

Comment 12

[David E. Ross]

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.  

Comment 13

[Boris Zbarsky [:bzbarsky]]

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).

Comment 14

[Boris Zbarsky [:bzbarsky]]

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 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 16

[Hixie (not reading bugmail)]

(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... ;-)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, for a bit he ran out of bits for a bit. :-)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Fixed, for a 1.7b bit.

Comment 19

[Scott MacGregor]

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. 

Comment 20

[Brendan Eich [:brendan]]

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

Comment 21

[Brendan Eich [:brendan]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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...

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Brendan Eich [:brendan]]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 26

[Boris Zbarsky [:bzbarsky]]

I will not have time to work on this in the foreseeable future.

Comment 27

[Scott MacGregor]

>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 :)

Comment 28

[Scott MacGregor]

Just so folks don't think I was manufacturing all the complaints I've been
getting from users about this regression...here are just a few of the postings:

http://forums.mozillazine.org/viewtopic.php?t=57856
http://forums.mozillazine.org/viewtopic.php?t=56951
http://forums.mozillazine.org/viewtopic.php?p=395086#395086
http://forums.mozillazine.org/viewtopic.php?p=395099#395099
http://forums.mozillazine.org/viewtopic.php?p=396842#396842
http://forums.mozillazine.org/viewtopic.php?p=400543#400543
http://forums.mozillazine.org/viewtopic.php?p=400960#400960
http://forums.mozillazine.org/viewtopic.php?t=56477

Comment 29

[Boris Zbarsky [:bzbarsky]]

Scott, I don't think anyone was doubting you on that count....

Comment 30

[Brendan Eich [:brendan]]

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

Comment 31

[Diego Biurrun]

Attachment: 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.

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

[David E. Ross]

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

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 35

[Martijn Wargers (dead)]

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?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip

Comment 37

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 38

[David E. Ross]

In which version of Gecko is this fixed?  This still seems to be a problem in Gecko 1.8.0.4/20060516.  

Comment 39

[Ryan VanderMeulen [:RyanVM]]

It's fixed on the trunk, which is what will eventually be branded as Gecko 1.9

Comment 40

[David E. Ross]

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.  

Comment 41

[Andrew Schultz]

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.

Comment 42

[Wayne Woods]

FYI, the 20060420 check-in seems to have caused bug 354778.