230555 - Add support for pre-line value of white-space

Status: VERIFIED FIXED

(Core :: Layout: Block and Inline, enhancement, P2)

Description

[Bob]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040106
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040106

Mozilla DOES respond correctly to the CSS 2.1 white-space property for the
values 'normal' and 'pre' and 'nowrap', but DOES NOT seem to respond at all to
the values 'pre-wrap', or 'pre-line'.

Reproducible: Always

Steps to Reproduce:
1. Create a style sheet with an element given the property 'white-space: pre-wrap'.
2. Also create an element with the property 'white-space: pre-line'.
3. Display the new page, and compare the results to the results as described in
the CSS 2.1 latest draft for text properties, under "Whitespace..."

Actual Results:  
The result produced by Mozilla was not the result expected of 'pre-wrap', as
whitespace was collapsed when it should not be. The result produced by Mozilla
was not the result expected of 'pre-line', as whitespace is collapsed, but
newlines are not observed.

Expected Results:  
'pre-wrap', according to the CSS 2.1 draft, "prevents user agents from
collapsing sequences of whitespace. Lines are broken at newlines in the source,
at occurrences of "\A" in generated content, and as necessary to fill line boxes."
'pre-line', according to the CSS 2.1 draft, "directs user agents to collapse
sequences of whitespace. Lines are broken at newlines in the source, at
occurrences of "\A" in generated content, and as necessary to fill line boxes."

Comment 1

[Isaac Hwak Han]

Xref to bug 99457. Also bug 206152 is line breaking meta bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that those property values are new in CSS2.1 (they are not in CSS 2.0) and
CSS2.1 is not yet in CR, so implementing its new properties without vendor
prefixes is highly undesirable at this point.  We implement -moz-pre-wrap (and
have for ages), but we'd need to add pre-line stuff...

Resummarizing to that effect and confirming.

Comment 3

[Hixie (not reading bugmail)]

Actually they're also in CSS3 Text and CSS3 Text is in CR so no prefixes are
needed on these.

Comment 4

[Anne (:annevk)]

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

Comment 5

[Anne (:annevk)]

Attachment: test case

The test case tests all the values 'white-space' can have according to CSS2.1.
'pre-line' and 'pre-wrap' have also a '-moz-' prefixed test.

Comment 6

[Sekundes]

CSS 2.1 is already in CR.
http://www.w3.org/TR/2004/CR-CSS21-20040225/

Comment 7

[Boris Zbarsky [:bzbarsky]]

As of yesterday, yes.  And it'll stay there for 6 months or so, during which
time they are calling for people to maybe implement it.

I await your patches most eagerly.

Comment 8

[Bill Mason]

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

Comment 9

[Anne (:annevk)]

See also bug 261081.

Comment 10

[Justin Wood (:Callek)]

I'll look into...hopefully

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #7)
> As of yesterday, yes.  And it'll stay there for 6 months or so, during which
> time they are calling for people to maybe implement it.
> 
> I await your patches most eagerly.

Boris:

Don't we have any reason for blocking to fix this bug?
We don't have a patch for dropping -moz- prefix only?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah... sorry. -moz-pre-line doesn't break line by \n.
I'll try to fix this.

Comment 13

[Gérard Talbot]

Testcase:
http://www.gtalbot.org/BugzillaSection/Bug230555WhiteSpacePreLine.html

Opera 9.50a1 build 9500 and Safari 3.0.3 build 522.15.5 for Windows pass that testcase. Adding whiteboard parity tags for Opera and Safari.

Note that -moz-pre-line is currently not supported and is being reported as an error in parsing value in Seamonkey 2.0a1pre rv:1.9a8pre build 2007090402.

Comment 14

[Joshua Cranmer [:jcranmer]]

Well, I've looked into some of how to implement this, but it's not something I can work on anytime soon, so I'll post some of my notes here.

In a conversation with fantasai over IRC, I determined that CSS 3 Text is relatively stable in concept with respect to the relevant portions of white-space (actual final value names and property names are subject to change, and hyphenation falls under the category of "don't go there").

According to the current texts of CSS 3 Text, the white-space property is a shorthand property for white-space-collapsing and text-wrap properties, both of which are moderately handled under nsStyleText::WhiteSpaceIsSignificant and nsStyleText::WhiteSpaceCanWrap. The other four values of white-space represent the four boolean combinations of these two functions. The problem with pre-line is that it only certain types of white space are significant.

To place pre-line within this mesh, a little more is required. The relevant place to muck around with is WhiteSpaceIsSignificant; we have two options:
1. Add a new boolean function (LineBreaksAreSignificant) that is true only for pre-line (and pre as well, I guess, but white space is significant there, so it doesn't matter too much...)
2. Make WhiteSpaceIsSignificant return an enum representing the possible values of white-space-collapsing (and change its name to something else).

I personally lean to the latter since it makes transitioning to CSS 3 Text easier.

A cursory examination of grep identified the following files as likely to need to be changed:
editor/libeditor/base/nsEditor.cpp
layout/forms/nsGfxButtonControlFrame.cpp
layout/generic/nsBlockFrame.cpp
layout/generic/nsFrame.cpp
layout/generic/nsLineLayout.cpp
layout/generic/nsTextFrameThebes.cpp
layout/inspector/src/inDOMUtils.cpp

Questions/Comments/Concerns ?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Here's a patch.

One issue here is the interaction with text-align:justify. The spec tries to disable justification for white-space:pre and pre-line, but it doesn't make much sense. I raised the issue on www-style. For now I'm leaving our existing behaviour unchanged; pre-line text can be justified, pre-wrap can't. There's a test for this now.

Comment 16

[James Benson]

Just wanted to point out that this defect also discusses pre-wrap rendering behavior... FF3 now honors rendering per CSS 2.1, but the selection copy still appears to ignore "\n".

Testcase: http://jjmmjmmm.com/ff3-testcase.html

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please file that as a separate bug.

Comment 18

[James Benson]

https://bugzilla.mozilla.org/show_bug.cgi?id=441648

Comment 19

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

So, the IsJustificationEnabled method and its callers don't make sense to me, since you'd want to check mTextAlign on the containing block and the white-space properties on the text.  Is the current code incorrectly checking mTextAlign on the text?  Or is something else going on?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're right, the current code incorrectly checks text-align on the text when it should be going out to the containing block. Would you like me to fix that as part of this patch, before this patch, or after this patch?

Comment 21

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

Any of the three is OK, but I'd prefer not to add an IsJustificationEnabled method that institutionalizes the bug in this patch.  In the places where you're switching to IsJustificationEnabled, the old code was closer to correct since at least the checks were separated at the caller.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2, now with tests

OK, this fixes the text-align behaviour (and adds a test for it of course).

Comment 23

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

Comment on attachment 328820 [details] [diff] [review]
fix v2, now with tests

+static const nsTextFrameUtils::CompressionMode CSSWhitespaceToCompressionMode[] =
+{
+  nsTextFrameUtils::COMPRESS_WHITESPACE_NEWLINE, // normal
+  nsTextFrameUtils::COMPRESS_NONE,               // pre
+  nsTextFrameUtils::COMPRESS_WHITESPACE_NEWLINE, // nowrap
+  nsTextFrameUtils::COMPRESS_NONE,               // pre-wrap
+  nsTextFrameUtils::COMPRESS_WHITESPACE          // pre-line
+};

Please precede this with:

PR_STATIC_ASSERT(NS_STYLE_WHITESPACE_NORMAL == 0);
PR_STATIC_ASSERT(NS_STYLE_WHITESPACE_PRE == 1);
PR_STATIC_ASSERT(NS_STYLE_WHITESPACE_NOWRAP == 2);
PR_STATIC_ASSERT(NS_STYLE_WHITESPACE_PRE_WRAP == 3);
PR_STATIC_ASSERT(NS_STYLE_WHITESPACE_PRE_LINE == 4);


+  // This should not be used during reflow
+  NS_ASSERTION(!(GetStateBits() & NS_FRAME_FIRST_REFLOW),
+               "Can only call this on frames that have been reflowed");

Seems like you want to check NS_FRAME_IN_REFLOW here too?  Or maybe
NS_FRAME_IS_DIRTY (although you'd really need to check that on
ancestors)?  Even if you can't assert it, could you describe in the
comment exactly what the rule is?

-FindFirstLetterRange(const nsTextFragment* aFrag,
+FindFirstLetterRange(const nsTextFragment* aFrag, const nsStyleText* aStyleText,

Not needed anymore.


BuildTextRunsScanner::BuildTextRunForFrames uses TEXT_JUSTIFICATION_ENABLED.
Is that guaranteed to happen after it's set by nsTextFrame::Reflow?


Did you check
http://www.hixie.ch/tests/adhoc/css/text/white-space/mixed/001.html ?
(I know we have a bug on the test being invalid or undefined... but do
we still behave the same?)


r+sr=dbaron

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #23)
> +  // This should not be used during reflow
> +  NS_ASSERTION(!(GetStateBits() & NS_FRAME_FIRST_REFLOW),
> +               "Can only call this on frames that have been reflowed");
> 
> Seems like you want to check NS_FRAME_IN_REFLOW here too?

Yes.

> BuildTextRunsScanner::BuildTextRunForFrames uses TEXT_JUSTIFICATION_ENABLED.
> Is that guaranteed to happen after it's set by nsTextFrame::Reflow?

No. That's actually a bug, I'll revise the patch to not depend on TEXT_JUSTIFICATION_ENABLED there.

> Did you check
> http://www.hixie.ch/tests/adhoc/css/text/white-space/mixed/001.html ?
> (I know we have a bug on the test being invalid or undefined... but do
> we still behave the same?)

I'll check.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #24)
> > Did you check
> > http://www.hixie.ch/tests/adhoc/css/text/white-space/mixed/001.html ?
> > (I know we have a bug on the test being invalid or undefined... but do
> > we still behave the same?)
> 
> I'll check.

Our behaviour on that test is unchanged (and it doesn't use pre-line).

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed f07d3fa9b667.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

New tests justification-1 and pre-line-3 are failing on Windows. I commented them out and I'll look into them shortly.

Comment 28

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

Might be better to mark them fails-if() than commenting them out.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

justification-1 fails because I assumed that white-space:pre-wrap on text that contains no newlines or double spaces would have line-breaking like white-space:normal. But that's not the case, because we can't trim spaces with pre-wrap, so when a word would fit on a line by less than a space's width, normal lets the word fit but pre-wrap doesn't.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

pre-line-3 fails because of a change Simon made so that if we can't find a letter for first-letter, then we don't apply the first-letter style to any text in the frame.

I fixed justification-1 by using &nbsp;<wbr> instead of spaces in the reference, to emulate pre-wrap behaviour. And I fixed pre-line-3 to check for first-letter by making the first-letter style a color change instead of a line-height change.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix tests

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed the test fix as changeset 078c7e6455b6.