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)

defect

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)

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.
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
Attached file Testcase (deleted) —
Space before <BR> testcase.
Attachment #75381 - Attachment is obsolete: true
-> Layout
Assignee: karnaze → attinasi
Component: HTMLTables → Layout
QA Contact: amar → petersen
Priority: -- → P4
Target Milestone: --- → Future
Changing QA contact
QA Contact: petersen → moied
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)
Attached patch This fixes it, actually... (obsolete) (deleted) — Splinter Review
Comment on attachment 141306 [details] [diff] [review] This fixes it, actually... Er, actually no. Totally wrong patch....
Attachment #141306 - Attachment is obsolete: true
Attached patch This is the right thing (obsolete) (deleted) — Splinter Review
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)
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
Closed: 21 years ago
Resolution: --- → FIXED
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 → ---
>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
Depends on: 235223
Blocks: 25680
Blocks: 239477
No longer blocks: 239477
Attached file testcase without table (deleted) —
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.
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?
Attached patch Updated to tip (deleted) — Splinter Review
Attachment #141353 - Attachment is obsolete: true
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago19 years ago
Resolution: --- → FIXED
Attachment #171890 - Attachment mime type: text/plain → text/html
Depends on: 333560
Depends on: 336408
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
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 → ---
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 ago18 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: