accept the overflow-wrap property

RESOLVED FIXED in Firefox 49

Status

()

Core
CSS Parsing and Computation
--
enhancement
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: sjw, Assigned: Thomas Wisniewski)

Tracking

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

Trunk
mozilla49
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 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’."
http://dev.w3.org/csswg/css-text/#overflow-wrap
(Reporter)

Updated

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

Updated

4 years ago
Severity: normal → enhancement

Comment 1

4 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
(Assignee)

Comment 3

a year ago
Created attachment 8755647 [details] [diff] [review]
955857-add-support-for-css-overflow-wrap.diff

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)
(Assignee)

Comment 4

a year ago
Created attachment 8755648 [details] [diff] [review]
955857-add-tests-for-css-overflow-wrap.diff

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]
955857-add-support-for-css-overflow-wrap.diff

>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 https://wiki.mozilla.org/ReleaseEngineering/TryServer 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]
955857-add-tests-for-css-overflow-wrap.diff

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-
(Assignee)

Comment 7

a year ago
Created attachment 8755664 [details] [diff] [review]
955857-add-tests-for-css-overflow-wrap.diff

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)
(Assignee)

Comment 8

a year ago
Created attachment 8755666 [details] [diff] [review]
955857-add-support-for-css-overflow-wrap.diff

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]
955857-add-tests-for-css-overflow-wrap.diff

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: https://treeherder.mozilla.org/#/jobs?repo=try&revision=645e7269bc3c
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd39abf2cdb7f988fe7cc1af2f670bd086f309d
Bug 955857 - Replace CSS word-wrap with overflow-wrap, and add it back as a CSS_PROP_ALIAS.  r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/47a2042ad9cef82c21ff09cbe52ec764185574dd
Bug 955857 - Add tests for overflow-wrap.  r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/986a22158836bddedd8ea6dda4134ce20984ab2c
Bug 955857 - Adjust test to expect that there are now 17 CSS properties beginning with 'o' rather than 16.  No review.
(Assignee)

Comment 13

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ecd39abf2cdb
https://hg.mozilla.org/mozilla-central/rev/47a2042ad9ce
https://hg.mozilla.org/mozilla-central/rev/986a22158836
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I've moved the word-wrap page to 
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-wrap
[And adapted it of course]
and updated:
https://developer.mozilla.org/en-US/Firefox/Releases/49#CSS
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.