Add support for pre-line value of white-space

VERIFIED FIXED in mozilla1.9.1a2

Status

()

Core
Layout: Block and Inline
P2
enhancement
VERIFIED FIXED
15 years ago
2 years ago

People

(Reporter: Bob, Assigned: roc)

Tracking

(Blocks: 1 bug, {css2, testcase})

Trunk
mozilla1.9.1a2
css2, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p-safari][p-opera])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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

15 years ago
Xref to bug 99457. Also bug 206152 is line breaking meta bug.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: CSS white-space property seems incomplete → Add support for pre-line value of white-space
Actually they're also in CSS3 Text and CSS3 Text is in CR so no prefixes are
needed on these.

Comment 4

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

Comment 5

14 years ago
Created attachment 139646 [details]
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.

Updated

14 years ago
Keywords: css2, testcase

Comment 6

14 years ago
CSS 2.1 is already in CR.
http://www.w3.org/TR/2004/CR-CSS21-20040225/
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.
Keywords: helpwanted

Updated

14 years ago
QA Contact: core.layout.block-and-inline → ian

Comment 8

14 years ago
*** Bug 261081 has been marked as a duplicate of this bug. ***
Summary: Add support for pre-line value of white-space → Add support for pre-line/pre-wrap value of white-space

Comment 9

14 years ago
See also bug 261081.
Summary: Add support for pre-line/pre-wrap value of white-space → Add support for pre-line value of white-space

Updated

14 years ago
No longer depends on: 261081

Updated

14 years ago
Blocks: 206152
I'll look into...hopefully
(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?
Ah... sorry. -moz-pre-line doesn't break line by \n.
I'll try to fix this.

Comment 13

11 years ago
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.
Whiteboard: [p-safari][p-opera]
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 ?
Created attachment 326243 [details] [diff] [review]
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #326243 - Flags: superreview?(dbaron)
Attachment #326243 - Flags: review?(dbaron)

Comment 16

10 years ago
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
Please file that as a separate bug.
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?
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?
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.
Attachment #326243 - Attachment is obsolete: true
Attachment #326243 - Flags: superreview?(dbaron)
Attachment #326243 - Flags: review?(dbaron)
Created attachment 328820 [details] [diff] [review]
fix v2, now with tests

OK, this fixes the text-align behaviour (and adds a test for it of course).
Attachment #328820 - Flags: superreview?(dbaron)
Attachment #328820 - Flags: review?(dbaron)
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
Attachment #328820 - Flags: superreview?(dbaron)
Attachment #328820 - Flags: superreview+
Attachment #328820 - Flags: review?(dbaron)
Attachment #328820 - Flags: review+
(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.
(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).
Pushed f07d3fa9b667.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite+

Updated

10 years ago
Keywords: helpwanted
Target Milestone: --- → mozilla1.9.1a2
New tests justification-1 and pre-line-3 are failing on Windows. I commented them out and I'll look into them shortly.
Might be better to mark them fails-if() than commenting them out.
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.
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.
Pushed the test fix as changeset 078c7e6455b6.
Status: RESOLVED → VERIFIED

Updated

10 years ago
Depends on: 459968
You need to log in before you can comment on or make changes to this bug.