Closed Bug 955857 Opened 7 years ago Closed 4 years ago

accept the overflow-wrap property

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: sjw+bugzilla, Assigned: wisniewskit)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

"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
Depends on: 587438
Severity: normal → enhancement
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
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)
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-
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)
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+
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.
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
You need to log in before you can comment on or make changes to this bug.