accept the overflow-wrap property

RESOLVED FIXED in Firefox 49

Status

()

Core
CSS Parsing and Computation
--
enhancement
RESOLVED FIXED
3 years ago
11 months 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

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

Updated

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

Updated

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

Comment 3

11 months 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

11 months 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

11 months 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

11 months 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

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
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: 11 months 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.