Closed
Bug 132561
Opened 23 years ago
Closed 18 years ago
whitespace before <BR> should be ignored (trailing whitespace at end of line should be collapsed in white-space:normal)
Categories
(Core :: Layout: Text and Fonts, defect, P2)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: jeffj, Assigned: bzbarsky)
References
(Depends on 1 open bug)
Details
(Keywords: testcase)
Attachments
(3 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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> </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•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
I just realized that to avoid the bug, the <BR> 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> </TD>
--
avoids the bug, while
---
<TD>
text <BR>
</TD>
--
does not avoid the bug. All other details of the HTML code are in the first
attachment to this bug report.
Assignee | ||
Comment 3•23 years ago
|
||
Hmm... should a space before <br> be rendered?
In any case, this is layout, not tables.
Comment 4•23 years ago
|
||
Space before <BR> testcase.
Attachment #75381 -
Attachment is obsolete: true
Comment 5•23 years ago
|
||
-> Layout
Assignee: karnaze → attinasi
Component: HTMLTables → Layout
QA Contact: amar → petersen
Updated•23 years ago
|
Priority: -- → P4
Target Milestone: --- → Future
Comment 7•21 years ago
|
||
*** Bug 229408 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
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
Updated•21 years ago
|
Severity: minor → normal
OS: Linux → All
Priority: P4 → --
Hardware: PC → All
Target Milestone: Future → ---
Updated•21 years ago
|
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)
Assignee | ||
Comment 8•21 years ago
|
||
Assignee | ||
Comment 9•21 years ago
|
||
Comment on attachment 141306 [details] [diff] [review]
This fixes it, actually...
Er, actually no. Totally wrong patch....
Attachment #141306 -
Attachment is obsolete: true
Assignee | ||
Comment 10•21 years ago
|
||
I was running out of flags for a bit there.... ;)
Assignee | ||
Comment 11•21 years ago
|
||
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•21 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.
Assignee | ||
Comment 13•21 years ago
|
||
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).
Assignee | ||
Comment 14•21 years ago
|
||
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•21 years ago
|
||
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 | ||
Updated•21 years ago
|
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
Comment 16•21 years ago
|
||
(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•21 years ago
|
||
No, for a bit he ran out of bits for a bit. :-)
Assignee | ||
Comment 18•21 years ago
|
||
Fixed, for a 1.7b bit.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 19•21 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.
Comment 20•21 years ago
|
||
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 → ---
Comment 21•21 years ago
|
||
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
Assignee | ||
Comment 22•21 years ago
|
||
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...
Assignee | ||
Comment 23•21 years ago
|
||
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•21 years ago
|
||
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
Assignee | ||
Comment 25•21 years ago
|
||
> 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).
Assignee | ||
Comment 26•21 years ago
|
||
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•21 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 :)
Comment 28•21 years ago
|
||
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
Assignee | ||
Comment 29•21 years ago
|
||
Scott, I don't think anyone was doubting you on that count....
Comment 30•21 years ago
|
||
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•20 years ago
|
||
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.
Assignee | ||
Comment 32•20 years ago
|
||
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•20 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
Comment 34•19 years ago
|
||
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 | ||
Updated•19 years ago
|
Assignee: nobody → bzbarsky
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Comment 35•19 years ago
|
||
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?
Assignee | ||
Comment 36•19 years ago
|
||
Attachment #141353 -
Attachment is obsolete: true
Assignee | ||
Comment 37•19 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago → 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #171890 -
Attachment mime type: text/plain → text/html
Comment 38•18 years ago
|
||
In which version of Gecko is this fixed? This still seems to be a problem in Gecko 1.8.0.4/20060516.
Comment 39•18 years ago
|
||
It's fixed on the trunk, which is what will eventually be branded as Gecko 1.9
Comment 40•18 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•18 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
Closed: 19 years ago → 18 years ago
Resolution: --- → FIXED
Comment 42•17 years ago
|
||
FYI, the 20060420 check-in seems to have caused bug 354778.
Depends on: 354778
You need to log in
before you can comment on or make changes to this bug.
Description
•