accept the overflow-wrap property

RESOLVED FIXED in Firefox 49



CSS Parsing and Computation
3 years ago
11 months ago


(Reporter: sjw, Assigned: Thomas Wisniewski)


(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)



(2 attachments, 2 obsolete attachments)



3 years ago
"For legacy reasons, UAs must treat ‘word-wrap’ as an alternate name for the ‘overflow-wrap’ property, as if it were a shorthand of ‘overflow-wrap’."


3 years ago
Blocks: 104960, 913153
Depends on: 587438


3 years ago
Severity: normal → enhancement

Comment 1

3 years ago
This is a Gecko bug wrt spec compliance. As this is part of CSS3-Text (and not a new spec), I reset the severity to normal. IMHO, only if this concerned a new spec that has no prior implementation in Gecko, the "enhancement" value would be justified.
Severity: enhancement → normal
It's a new feature; word-wrap predates the spec, but overflow-wrap does not.  It's still trivial, and we should do it when the spec reaches CR.
Severity: normal → enhancement
See Also: → bug 959735
Keywords: dev-doc-needed

Comment 3

11 months ago
Created attachment 8755647 [details] [diff] [review]

Here's a patch. I'm not sure if I went too far with the renames/comment changes here, but they seemed reasonable given that overflow-wrap is now considered the property proper in the spec.
Attachment #8755647 - Flags: review?(dbaron)

Comment 4

11 months ago
Created attachment 8755648 [details] [diff] [review]

Here are the tests. I'm not sure if the file names for the new refrests should be re-numbered or kept to match the corresponding word-wrap tests they were copied from.
Attachment #8755648 - Flags: review?(dbaron)
Comment on attachment 8755647 [details] [diff] [review]

>diff --git a/layout/style/nsComputedDOMStylePropertyList.h b/layout/style/nsComputedDOMStylePropertyList.h

I believe the list in this file needs to be sorted alphabetically, or it will fail tests.

r=dbaron with that fixed

I think we're probably ok not having a pref for this (which would actually be nontrivial, since we'd need a pref that controlled the main property but not the alias).

This should get a run once the patch is revised before it lands.  needinfo? me and I can push it for you.
Attachment #8755647 - Flags: review?(dbaron) → review+
Comment on attachment 8755648 [details] [diff] [review]

overflowwrap-04 and overflowwrap-05 should use overflowWrap rather than wordWrap

There's also no need to copy the -ref files (even though you did change some of them), but you need to fix the manifest accordingly.  For the ones where the -ref file uses overflow-wrap (4, 5, 8, 9, I think), you could do an == test between the two tests instead of having a second copy of the reference.

r=dbaron with those things fixed, although I should probably look at the revisions, so not marking as review+
Attachment #8755648 - Flags: review?(dbaron) → review-

Comment 7

11 months ago
Created attachment 8755664 [details] [diff] [review]

It was only tests 4 and 9, after a second look. Turns out that 8 was really testing the output of overflow:scroll against the results of word-wrap, so I don't suspect it's worth duping that one. The rest I adjusted as you suggested.
Attachment #8755648 - Attachment is obsolete: true
Attachment #8755664 - Flags: review?(dbaron)

Comment 8

11 months ago
Created attachment 8755666 [details] [diff] [review]

Here's the revision with the re-ordering.

needinfoing you as requested.
Attachment #8755647 - Attachment is obsolete: true
Flags: needinfo?(dbaron)
Comment on attachment 8755664 [details] [diff] [review]

r=dbaron, although I'm not sure why I suggested comparing 4 and 9 to the test rather than reference; they may as well compare to the reference like the others.
Attachment #8755664 - Flags: review?(dbaron) → review+
Attachment #8755666 - Flags: review+
Try push at:
Flags: needinfo?(dbaron)
One failure so far that seems related, in mochitest-devtools-8:

INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js | Number of suggestions match - Got 17, expected 16

Given that it's testing the devtools autocompletion for the number of properties that start with "o", I think it's just fine to bump the number to 17.  I'll test that locally without bothering with another try run.
Bug 955857 - Replace CSS word-wrap with overflow-wrap, and add it back as a CSS_PROP_ALIAS.  r=dbaron
Bug 955857 - Add tests for overflow-wrap.  r=dbaron
Bug 955857 - Adjust test to expect that there are now 17 CSS properties beginning with 'o' rather than 16.  No review.

Comment 13

11 months ago
Thanks for the try run. It seems to have completed now, and I can't see the other two failures being related to this patchset. (Also thanks for fixing the other failure).

Let me know if there's anything else I should deal with before this can be considered for landing (whenever that becomes appropriate).
comment 12 (which is script-generated) says I already landed it on mozilla-inbound, which is merged to mozilla-central once or twice per day.
Assignee: nobody → wisniewskit

Comment 15

11 months ago
Last Resolved: 11 months ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I've moved the word-wrap page to
[And adapted it of course]
and updated:
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.