453468 - <textarea wrap="off"> no longer works

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[[e-mail]]

Since bug 99457 landed textareas always word wrap, even with wrap="off". The styles controlling wordrwap have changed and editor needs to be updated.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So presumably the div inside the textarea should inherit the word-wrap (which it does now) and we need to map the wrap attribute into the word-wrap style.

Of course to make that work the !important on the word-wrap in forms.css will need to go.  I can't tell why the !important was added there anyway; it makes sense to allow the page to override the wrapping as needed...

This really needs to get fixed for 1.9.1, in my opinion.

Comment 2

[Simon Montagu :smontagu]

(In reply to comment #1)
> I can't tell why the !important was added there anyway

For interoperability with IE and Safari. To actually achieve that, we need |wrap="off"| to be honoured, but |word-wrap: normal| to be ignored on textareas.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I don't see a good reason to ignore "word-wrap: normal" when explicitly specified, to be honest... do you expect it to cause problems?

If so, then making wrap="off" work will be that much more fun....

Comment 4

[Simon Montagu :smontagu]

Um, BTW, what is this wrap attribute of which you speak? It doesn't seem to exist in the HTML spec.

Comment 5

[Boris Zbarsky [:bzbarsky]]

It's not in the HTML spec.  Nevertheless, it's widely supported (both IE and Netscape implemented it in their time) and relied on by sites.  http://msdn.microsoft.com/en-us/library/ms535152(VS.85).aspx documents it pretty nicely.

Comment 6

[Simon Montagu :smontagu]

Attachment: Patch

Easier than I anticipated :)

(In reply to comment #3)
> I don't see a good reason to ignore "word-wrap: normal" when explicitly
> specified, to be honest... do you expect it to cause problems?

I thought it was better to be consistent with the existing implementations, but I'm not particularly committed to it if people would rather remove the !important.

Comment 7

[Boris Zbarsky [:bzbarsky]]

That also changes behavior for <div style="white-space: pre; word-break: break-word">, no?  I have a hard time believing that both behaviors are correct per CSS spec.  Which one is correct?  Do we have a reftest for this?

Comment 8

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

Who else implements 'word-wrap'?  Is this interaction with 'white-space' compatible with their behavior?  Is it compatible with the spec?

Comment 9

[Simon Montagu :smontagu]

The spec (http://www.w3.org/TR/css3-text/) says:

a) [word-wrap] only has an effect when 'text-wrap' is either 'normal' or 'suppress'
b) The 'white-space' property is a shorthand for the 'white-space-collapse'  and 'text-wrap' properties. Not all combinations are represented. Values have the following meanings:

normal    Sets ... 'text-wrap' to 'normal'
pre    Sets ... 'text-wrap' to 'none'
nowrap    Sets ... 'text-wrap' to 'none'
pre-wrap    Sets ... 'text-wrap' to 'normal'
pre-line    Sets ... 'text-wrap' to 'normal'

Comment 10

[[e-mail]]

Attachment: Micro-optimisation

I independently read the spec and came up with the same basic patch, although I was then tempted to include some microoptimisation that also helps clarify the interaction between white-space, text-wrap and white-space-collapse.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Do we not implement the "white-space is a shorthand" thing?  That is would it be simpler to make parsing of "white-space:pre" actually set text-wrap to none?

Comment 13

[Simon Montagu :smontagu]

We don't yet support text-wrap (as opposed to word-wrap) at all, but it's on my to-do list.

Comment 14

[[e-mail]]

Comment on attachment 336737 [details] [diff] [review]
Micro-optimisation

Hmm, this doesn't work...

Comment 15

[[e-mail]]

Attachment: Corrected micro-optimisation

Comment 16

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

Comment on attachment 336716 [details] [diff] [review]
Patch

r+sr=dbaron on the condition that this is compatible with IE.  (I'm assuming IE implementing this is why we implemented the property without a -moz- prefix.)


(As far as Neil's patches go, I think we should hold off on adding the constants for the split properties until we actually implement those.)

Comment 17

[Simon Montagu :smontagu]

(In reply to comment #16)
> (From update of attachment 336716 [details] [diff] [review])
> r+sr=dbaron on the condition that this is compatible with IE.

Unfortunately that condition resolves to 'false'.

Firstly, I was wrong in saying that we need the !important to be compatible with IE: it's only Safari that doesn't honor "word-wrap: normal" on <textarea>. But even without the !important we aren't compatible with IE, because with "word-wrap: normal" IE doesn't wrap even at spaces.

Secondly, in the <div style="white-space: pre; word-break: break-word"> case, we are compatible with neither IE nor Safari: IE breaks at spaces and in mid-word; Safari doesn't break at spaces, but does break in mid-word.

Comment 18

[Simon Montagu :smontagu]

Attachment: Testcase for different combinations of white-space and word-wrap

Comment 19

[Simon Montagu :smontagu]

Attachment: Screenshots of attachment 338726

Comment 20

[Simon Montagu :smontagu]

So where are we going to go with this? The patch gives us compatibility with the current version of CSS3 Text, to the best of my understanding, but not compatibility with either of the existing mututally incompatible implementations.

Comment 21

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

Given that the incompatibility is only in the white-space: pre / nowrap case, I presume, that's OK.

So I think we should land the patch I reviewed above; my review still stands.

I tend to lean towards removing the "!important" for textarea { word-wrap: break-word; } (if that's the one you're talking about in your "Firstly" comment) if that would mean that an author specifying word-wrap: normal would cause long words in textareas to overflow, but wrapping to occur as it currently does at spaces.

Comment 22

[Simon Montagu :smontagu]

Pushed http://hg.mozilla.org/mozilla-central/rev/b7253556900e (with !important for textarea removed and the reftests adjusted to correspond).

Comment 23

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/mozilla-central/rev/a415c35ef915
{
Work around failing (new) test
}

See
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1224195941.1224199317.2344.gz
WINNT 5.2 mozilla-central moz2-win32-slave07 dep unit test on 2008/10/16 15:25:41
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1224196047.1224200710.5915.gz
WINNT 5.2 mozilla-central moz2-win32-slave08 dep unit test on 2008/10/16 15:27:27
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1224195150.1224200190.4400.gz
Win2k3 comm-central dep unit test on 2008/10/16 15:12:30

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 336836 [details] [diff] [review]
Corrected micro-optimisation

Neil, this patch (file) is _exactly_ the same as the previous one :-|

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> Pushed http://hg.mozilla.org/mozilla-central/rev/b7253556900e (with !important
> for textarea removed and the reftests adjusted to correspond).

I don't understand what you landed: it seems to be parts of the two patches, but not one nor the other nor the addition of the two :-/

Comment 26

[Simon Montagu :smontagu]

What I landed was the essential code change that fixes the bug, which is common to both patches:

   PRBool WordCanWrap() const {
-    return mWordWrap == NS_STYLE_WORDWRAP_BREAK_WORD;
+    return WhiteSpaceCanWrap() && mWordWrap == NS_STYLE_WORDWRAP_BREAK_WORD;
   }

plus the change to textarea in forms.css, which was in Neil's patch but not mine, but we reached consensus that we want the change:

-  word-wrap: break-word !important;
+  word-wrap: break-word;

I've opened bug 460399 on the failing test, thanks for checking in the workaround.

Comment 27

[Aakash Desai [:aakashd]]

Seeing as there hasn't been any discussions about this bug for 5 months, I'm
guessing there aren't any residual issues. I'm moving this to verified as a
result. If anyone has any qualms, feel free to bring them up.