Closed Bug 453468 Opened 12 years ago Closed 12 years ago

<textarea wrap="off"> no longer works

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: neil, Assigned: smontagu)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(4 files, 1 obsolete file)

Since bug 99457 landed textareas always word wrap, even with wrap="off". The styles controlling wordrwap have changed and editor needs to be updated.
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.
Flags: blocking1.9.1?
(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.
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....
Um, BTW, what is this wrap attribute of which you speak? It doesn't seem to exist in the HTML spec.
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.
Attached patch PatchSplinter Review
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.
Assignee: nobody → smontagu
Status: NEW → ASSIGNED
Attachment #336716 - Flags: superreview?(dbaron)
Attachment #336716 - Flags: review?(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?
Who else implements 'word-wrap'?  Is this interaction with 'white-space' compatible with their behavior?  Is it compatible with the spec?
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'
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.
Attachment #336737 - Flags: superreview?(dbaron)
Attachment #336737 - Flags: review?(bzbarsky)
Duplicate of this bug: 453471
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?
We don't yet support text-wrap (as opposed to word-wrap) at all, but it's on my to-do list.
Attachment #336737 - Flags: superreview?(dbaron)
Attachment #336737 - Flags: review?(bzbarsky)
Comment on attachment 336737 [details] [diff] [review]
Micro-optimisation

Hmm, this doesn't work...
Attached patch Corrected micro-optimisation (obsolete) — Splinter Review
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.)
Attachment #336716 - Flags: superreview?(dbaron)
Attachment #336716 - Flags: superreview+
Attachment #336716 - Flags: review?(bzbarsky)
Attachment #336716 - Flags: review+
(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.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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.
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.
Attachment #336716 - Flags: review?(dbaron) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/b7253556900e (with !important for textarea removed and the reftests adjusted to correspond).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: --- → mozilla1.9.1b2
Comment on attachment 336836 [details] [diff] [review]
Corrected micro-optimisation

Neil, this patch (file) is _exactly_ the same as the previous one :-|
Attachment #336836 - Attachment is obsolete: true
(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 :-/
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Depends on: 460399
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.