Bug 37261 - White space preceding a <br> affects layout of center or right aligned preceding line
Summary: White space preceding a <br> affects layout of center or right aligned preced...
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-08 02:46 PDT by Xianzhu Wang
Modified: 2010-12-30 01:11 PST (History)
7 users (show)

See Also:


Attachments
The test case (712 bytes, text/html)
2010-04-15 21:41 PDT, Xianzhu Wang
no flags Details
Patch containing only the changed source code and the new test case (116.18 KB, patch)
2010-04-15 21:47 PDT, Xianzhu Wang
abarth: review-
Details | Formatted Diff | Diff
The list of effected existing test cases (19.55 KB, text/plain)
2010-04-15 21:49 PDT, Xianzhu Wang
no flags Details
Full patch containing all updated test cases, part 1 (1.01 MB, patch)
2010-04-16 03:21 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Full patch containing all updated test cases, part 2 (1.31 MB, patch)
2010-04-16 03:26 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Updated full patch 5/12 part1 (1.53 MB, patch)
2010-05-11 22:12 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Updated full patch 5/12 part2 (1.34 MB, patch)
2010-05-11 22:13 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Updated full patch 5/12 part3 (1.51 MB, patch)
2010-05-11 22:14 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Updated full patch 5/12 part4 (1.39 MB, patch)
2010-05-11 22:15 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Updated patch containing only the changed code and added test case (6.68 KB, patch)
2010-07-21 00:42 PDT, Xianzhu Wang
abarth: review-
Details | Formatted Diff | Diff
Patch rebaslining simple text-only tests affected by this bug fix (deleted)
2010-07-22 01:04 PDT, Xianzhu Wang
abarth: review-
Details | Formatted Diff | Diff
Patch rebaslining simple DRT tests affected by this bug fix (853.54 KB, patch)
2010-07-22 01:22 PDT, Xianzhu Wang
abarth: review-
Details | Formatted Diff | Diff
Patch about other complex text-only and DRT tests affected by this bug fix (35.64 KB, patch)
2010-07-22 04:15 PDT, Xianzhu Wang
abarth: review-
Details | Formatted Diff | Diff
Patch temporarily skip tests of other platforms affected by this bug fix (7.85 KB, patch)
2010-07-22 19:56 PDT, Xianzhu Wang
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2010-04-08 02:46:41 PDT
Open the attached test case. The white spaces preceding <br>s effect the layout of center or right aligned preceding lines.

In Firefox, IE, etc., the white spaces preceding <br>s won't effect layout.

Reference: http://www.w3.org/TR/CSS2/text.html#white-space-model

Chromium bug: http://crbug.com/40634
Comment 1 Xianzhu Wang 2010-04-15 21:41:59 PDT
Created attachment 53509 [details]
The test case
Comment 2 Xianzhu Wang 2010-04-15 21:47:59 PDT
Created attachment 53511 [details]
Patch containing only the changed source code and the new test case

This patch effects many existing layout tests. Please see the next attachment for the list of effected tests. I'm still working on fixing these test failures and will submit a separate patch for them.
Comment 3 Xianzhu Wang 2010-04-15 21:49:15 PDT
Created attachment 53512 [details]
The list of effected existing test cases
Comment 4 Xianzhu Wang 2010-04-16 03:21:54 PDT
Created attachment 53525 [details]
Full patch containing all updated test cases, part 1
Comment 5 Xianzhu Wang 2010-04-16 03:26:41 PDT
Created attachment 53526 [details]
Full patch containing all updated test cases, part 2
Comment 6 WebKit Review Bot 2010-05-11 02:46:16 PDT
Attachment 53526 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 mitz 2010-05-11 02:55:01 PDT
Does the patch make it impossible to select the trailing space or to visualize that it is selected?
Comment 8 Xianzhu Wang 2010-05-11 03:03:49 PDT
(In reply to comment #7)
> Does the patch make it impossible to select the trailing space or to visualize that it is selected?

Yes, but this should be the expected behavior. Other trailing spaces not preceding <br> in white space collapsing context (e.g. white-space: normal) are also not selectable and invisible.
Comment 9 Xianzhu Wang 2010-05-11 22:12:33 PDT
Created attachment 55805 [details]
Updated full patch 5/12 part1
Comment 10 Xianzhu Wang 2010-05-11 22:13:37 PDT
Created attachment 55806 [details]
Updated full patch 5/12 part2
Comment 11 Xianzhu Wang 2010-05-11 22:14:35 PDT
Created attachment 55807 [details]
Updated full patch 5/12 part3
Comment 12 Xianzhu Wang 2010-05-11 22:15:23 PDT
Created attachment 55808 [details]
Updated full patch 5/12 part4
Comment 13 WebKit Review Bot 2010-05-11 22:16:55 PDT
Attachment 55805 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/RenderBlockLineLayout.cpp:1889:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 1594 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Xianzhu Wang 2010-05-11 22:18:04 PDT
Comment on attachment 53511 [details]
Patch containing only the changed source code and the new test case

This patch is a reduced patch that can reduce your review effort. This patch contains only the changed source code and one added layout test. The other "full patches" are so big because they contain many effected layout tests, including the 4000+ just added js sputnik tests.
Comment 15 WebKit Review Bot 2010-05-11 22:25:41 PDT
Attachment 55808 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 WebKit Review Bot 2010-05-11 22:27:49 PDT
Attachment 53511 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/RenderBlockLineLayout.cpp:1889:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Dave Hyatt 2010-06-03 14:21:11 PDT
A couple of comments:

(1) As you can see from my original code comment, this behavior of retaining the space before a <br> was actually deliberate.  At the time (many years ago), I was definitely matching some other browser's behavior (probably Firefox).  Have you verified that both IE for Windows and Firefox discard the spaces before a <br>?

(2) Have you tested HTML editing with this patch in place?  My concern is that the editing code may open a sequence of alternating nbsps and spaces with a space, and that when you insert a br after that space, that space would suddenly disappear.  It may be that editing opens with nbsp when you first type a space, but it might not.
Comment 18 Xianzhu Wang 2010-06-03 20:09:18 PDT
(In reply to comment #17)
Thanks for your comments.

> A couple of comments:
> 
> (1) As you can see from my original code comment, this behavior of retaining the space before a <br> was actually deliberate.  At the time (many years ago), I was definitely matching some other browser's behavior (probably Firefox).  Have you verified that both IE for Windows and Firefox discard the spaces before a <br>?
> 

I just verified again in other browsers with the attached test case. The results are:
- IE8: spaces before a <br> doesn't affect the layout, but is selectable.
- Firefox 3.6: spaces before a <br> neither affect the layout nor is selectable.
- Opera 10: same as Firefox

According to the standard, I think current Firefox and Opera's behavior is correct. The patch can make WebKit behave like them.

> (2) Have you tested HTML editing with this patch in place?  My concern is that the editing code may open a sequence of alternating nbsps and spaces with a space, and that when you insert a br after that space, that space would suddenly disappear.  It may be that editing opens with nbsp when you first type a space, but it might not.

Just verified that the editing behavior is OK with the patch.
Comment 19 Xianzhu Wang 2010-06-20 19:40:31 PDT
Comment on attachment 55805 [details]
Updated full patch 5/12 part1

I'll create a new patch after I get a Snow Leopard mac book.
Comment 20 Adam Barth 2010-06-21 19:36:54 PDT
Comment on attachment 53511 [details]
Patch containing only the changed source code and the new test case

I'm skeptical about this patch.  It seems to be a big change for a somewhat theoretical problem.  Are there any web sites that render better with this change?  How many web sites render worse?  I'd like some estimate of the compatibility impact of this change before reviewing the code to technical correctness.
Comment 21 Xianzhu Wang 2010-06-29 00:43:19 PDT
(In reply to comment #20)
> (From update of attachment 53511 [details])
> I'm skeptical about this patch.  It seems to be a big change for a somewhat theoretical problem.  Are there any web sites that render better with this change?  How many web sites render worse?  I'd like some estimate of the compatibility impact of this change before reviewing the code to technical correctness.

I just finished a test of 1000 popular web site home pages. 51 of them are affected by this problem. All of the affected pages will render better with the patch (though some differ slightly, some are very visible): center aligned texts will be better aligned; right aligned texts will be correctly aligned.

My testing criteria are: center aligned text, or right aligned ltr text, or left aligned rtl text; and computed value of 'white-space' style is 'normal'; and with spaces before <br>. (Left aligned ltr texts and right aligned rtl texts are not counted because space before <br> has no visual effect.)

This problem breaks the basic rule of HTML about spaces. It also affects editing because of the extra spaces before line breaks. I think this should be fixed.
Comment 22 Adam Barth 2010-06-29 18:28:20 PDT
> I just finished a test of 1000 popular web site home pages. 51 of them are affected by this problem. All of the affected pages will render better with the patch (though some differ slightly, some are very visible): center aligned texts will be better aligned; right aligned texts will be correctly aligned.

Woah, cool!  Can you list a bunch of them so we can see?
Comment 23 Xianzhu Wang 2010-06-30 02:07:20 PDT
Most detected sites effected by this issue are about the center aligned copyright notices. Because the lines are long, the misalignment caused by a single space is not easily visible.

Here are some websites that have visible effects of the issue (some might have been out-dated because of change of the website contents):

http://americanas.com.br/ : 2224Whitespace before BR|Whitespace : Telefones <br>
C芒meras & <br>
Utilidades <br>

http://eastmoney.com/ : 2224Whitespace before BR|Whitespace : 30.16%
<br>
23.81%
<br>
46.03%
<br>
19.05%
<br>
26.19%
<br>
54.76%
<br>

http://gazetevatan.com/ : 2224Whitespace before BR|Whitespace :         26.06.2010
<br>
        23.06.2010
<br>
        28.06.2010
<br>
        24.06.2010
<br>

http://laposte.net/ : 2224Whitespace before BR|Whitespace : Achetez 

<br>
Tarifs

<br>
Envoyez

<br>
Trouvez 

<br>
un bureau

<br>
Envoyez 

<br>
Achetez 

<br>

http://nick.com/ : 2224Whitespace before BR|Whitespace : Play the new <br>

http://pagesjaunes.fr/ : 2224Whitespace before BR|Whitespace : 
			Le bulletin <br>

			L'茅tat du <br>

				Les sorties <br>

			Les films 脿 <br>

http://uol.com.br/ : 2224Whitespace before BR|Whitespace : Compare <br>
Guia de <br>
Letras de <br>
Comment 24 Xianzhu Wang 2010-07-21 00:42:32 PDT
Created attachment 62149 [details]
Updated patch containing only the changed code and added test case

Also fixed an issue that the trailing space affects shrink-to-fit width.
Comment 25 Xianzhu Wang 2010-07-22 01:04:32 PDT
Created attachment 62272 [details]
Patch rebaslining simple text-only tests affected by this bug fix

Note to reviewer:
This patch rebaselines 5239 tests (mostly js sputnik tests) that have text-only expectations whose new versions differ only about trailing spaces from the original versions.
Comment 26 Xianzhu Wang 2010-07-22 01:22:46 PDT
Created attachment 62274 [details]
Patch rebaslining simple DRT tests affected by this bug fix

Note to reviewer:
This patch rebaselines 273  tests that have DRT expectations whose new versions differ only about trailing spaces from the original versions.
Comment 27 Xianzhu Wang 2010-07-22 04:15:36 PDT
Created attachment 62284 [details]
Patch about other complex text-only and DRT tests affected by this bug fix

For now, all tests affected by this bug fix has been rebaselined. Some tests in this patch are not simply rebaselined, but are modified to reflect their original intentions.
Note: all of the above patches about test cases are only done on Snow Leopard, and don't include pixel tests. I'll work on other platforms tomorrow.
Comment 28 Xianzhu Wang 2010-07-22 19:56:30 PDT
Created attachment 62373 [details]
Patch temporarily skip tests of other platforms affected by this bug fix

Now I believe the patches are complete.
Comment 29 Xianzhu Wang 2010-08-23 23:27:21 PDT
Hi, all,

What are your opinions about this issue?

Thanks,
Xianzhu
Comment 30 Adam Barth 2010-10-10 11:45:32 PDT
Comment on attachment 62149 [details]
Updated patch containing only the changed code and added test case

I looked at the example sites you reference above.  I couldn't really see any difference in the rendering around the text that you cited between WebKit and Firefox.  This seems to be a very minor issue.  I'm not sure the risks of taking these patches are worth the potential upside.  However, I'll defer to the rendering experts if they have a different opinion.

Rather than have these patches sit in pending-review, I'm making them R-.  If a reviewer is interested in moving this bug forward, please feel free to review them.
Comment 31 Xianzhu Wang 2010-10-10 19:55:52 PDT
(In reply to comment #30)
Thanks Adam very much for your attention.

I just reviewed the example web pages. 3 of them (http://americanas.com.br/, http://www.pagesjaunes.fr/ and http://www.laposte.net/) still have problems, and the other 4 changed their layout or contents and the original problems couldn't be found. 

The problems of the web pages are: some headings are expected center-aligned, but some lines are not actually at the center of the containing block, but are shifted to the left because of the spaces before <br>s, like the following:

ABCDEFG
 HIJKLM

I think many web developers knowing this issue will avoid spaces before <br>s to get good rendering in all browsers. However, this is an extra burden when formatting HTML source code, especially when we are using a automatic formatting tool, because we must manually remove some spaces that are automatically added by the tool.

Besides alignment, the extra spaces also sometimes effect the width of the containing block.