Bug 94475 – CSS 3 ‘overflow-wrap’ property implementation

• Component: CSS
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

WebKit is missing ‘overflow-wrap’ property.

http://www.w3.org/TR/2012/WD-css3-text-20120814/#overflow-wrap

The specification says that ‘overflow-wrap’ is actually just another name for the currently implemented 
'word-wrap' property (which also should be supported for legacy reasons).

[Mikhail Pozdnyakov]

Created attachment 159408 [details]
preliminary patch.

Preliminary patch. Dedicated test case and ChangeLog description are coming.

[Mikhail Pozdnyakov]

Created attachment 159417 [details]
patch

Patch with layout test added.

[Kenneth Rohde Christiansen]

Comment on attachment 159417 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159417&action=review

> LayoutTests/ChangeLog:8
> +        Added âoverflow-wrapâ property implementation according to

â something wrong with the changelog?

[Kenneth Rohde Christiansen]

Comment on attachment 159417 [details]
patch

Maybe Jullien wants wants a look before you commit

[WebKit Review Bot]

Comment on attachment 159417 [details]
patch

Attachment 159417 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13528994

New failing tests:
fast/text/overflow-wrap.html

[WebKit Review Bot]

Created attachment 159433 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 159417 [details]
patch

Attachment 159417 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13537749

New failing tests:
fast/text/overflow-wrap.html

[WebKit Review Bot]

Created attachment 159445 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Julien Chaffraix]

Comment on attachment 159417 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159417&action=review

> LayoutTests/fast/text/overflow-wrap-expected.txt:20
> +          text run at (2,110) width 127: "word CSS property."

First you are missing the image result (png) which is why the cq is rejecting your patch. Now my question is: why do we need a pixel test here? Couldn't we get away with a ref-test?

See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree for writing good pixel tests (hint: adding this test is platform-dependent which is bad) and http://trac.webkit.org/wiki/Writing%20Reftests about ref-tests. Also it would be nice to state better what you expect from the test: basically you are only testing "longlonglonglonglongword" wrapping, the rest of the explanations are just here to confuse.

> Source/WebCore/ChangeLog:9
> +        http://www.w3.org/TR/2012/WD-css3-text-20120814/#overflow-wrap.

Where do you implement the following sentence?

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’.

> Source/WebCore/ChangeLog:26
> +        (WebCore::StyleResolver::collectMatchingRulesForList):

Those entries should be filled with details about the implementation.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:162
> +    CSSPropertyOverflowWrap,

What's the rationale behind landing this unprefixed and without a compile time flag?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1925
> +        case CSSPropertyOverflowWrap:
> +            return cssValuePool().createValue(style->wordWrap());

This is not covered by any test you have. You should add more testing to cover setting the style through JS / CSS and getting it via getComputedStyle. This *does* include testing that inheritance is properly implemented.

[Julien Chaffraix]

Tab, if I am reading http://lists.w3.org/Archives/Public/www-style/2012May/0541.html properly, we shouldn't be implementing 'overflow-wrap' in WebKit as it will be made an alias of 'word-wrap' in a follow-up version of CSS3 text.

[Mikhail Pozdnyakov]

Created attachment 159746 [details]
patch v2

Julien's feedback is taken into consideration. Improved the added layout test and ChangeLog description.

[Mikhail Pozdnyakov]

Created attachment 159748 [details]
patch v3

Fixed typo in ChangeLog.

[Mikhail Pozdnyakov]

Comment on attachment 159417 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159417&action=review

>> LayoutTests/fast/text/overflow-wrap-expected.txt:20
>> +          text run at (2,110) width 127: "word CSS property."
> 
> First you are missing the image result (png) which is why the cq is rejecting your patch. Now my question is: why do we need a pixel test here? Couldn't we get away with a ref-test?
> 
> See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree for writing good pixel tests (hint: adding this test is platform-dependent which is bad) and http://trac.webkit.org/wiki/Writing%20Reftests about ref-tests. Also it would be nice to state better what you expect from the test: basically you are only testing "longlonglonglonglongword" wrapping, the rest of the explanations are just here to confuse.

Thanks for the hint and links!

>> Source/WebCore/ChangeLog:9
>> +        http://www.w3.org/TR/2012/WD-css3-text-20120814/#overflow-wrap.
> 
> Where do you implement the following sentence?
> 
> 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’.

It is implemented by applying the same handlers for both properties.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:162
>> +    CSSPropertyOverflowWrap,
> 
> What's the rationale behind landing this unprefixed and without a compile time flag?

It's defined the same way as "word-wrap" property.

[Tab Atkins]

(In reply to comment #10)
> Tab, if I am reading http://lists.w3.org/Archives/Public/www-style/2012May/0541.html properly, we shouldn't be implementing 'overflow-wrap' in WebKit as it will be made an alias of 'word-wrap' in a follow-up version of CSS3 text.

We've gone ahead and formalized the aliasing method (treat one of them as a shorthand for the "good" one), and so that's just been folded into the current draft of CSS3 Text.  So, we're okay to proceed on this bug.


(In reply to comment #13)
> >> Source/WebCore/ChangeLog:9
> >> +        http://www.w3.org/TR/2012/WD-css3-text-20120814/#overflow-wrap.
> > 
> > Where do you implement the following sentence?
> > 
> > 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’.
> 
> It is implemented by applying the same handlers for both properties.

It looks like you handle the "shorthand" behavior with your addition to StyleBuilder.cpp, right?  If so, I think you have it backwards - word-wrap is the shorthand for overflow-wrap, but you've implemented it as overflow-wrap being the shorthand for word-wrap.
 
> >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:162
> >> +    CSSPropertyOverflowWrap,
> > 
> > What's the rationale behind landing this unprefixed and without a compile time flag?
> 
> It's defined the same way as "word-wrap" property.

Yes, it has zero new behavior, and is just a rename.  That's acceptable to land without prefixing, I think.

[Mikhail Pozdnyakov]

Created attachment 159906 [details]
patch v4

Tab, thanks for your comments. I have made 'overflow-wrap' essential and 'text-wrap' a shorthand.

[WebKit Review Bot]

Attachment 159906 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSPrimitiveValueMappings.h:2621:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 159906 [details]
patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=159906&action=review

> LayoutTests/fast/text/overflow-wrap-expected.html:1
> +<script>

where is the html5 doctype, html, head? Also please link to the releavant part of the spec as suggested by http://wiki.csswg.org/test/format

[Mikhail Pozdnyakov]

(In reply to comment #16)
> Attachment 159906 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> Source/WebCore/css/CSSPrimitiveValueMappings.h:2621:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
> Total errors found: 1 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Such indentation is everywhere inside Source/WebCore/css/CSSPrimitiveValueMappings.h, it might worth aligning with style guides, however think it's out of this bug's scope.

[Mikhail Pozdnyakov]

Created attachment 159916 [details]
patch v5

Kenneth, thanks for review and for the link!

[WebKit Review Bot]

Attachment 159916 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSPrimitiveValueMappings.h:2621:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mikhail Pozdnyakov]

Any further comments? review?

[Ojan Vafai]

Comment on attachment 159916 [details]
patch v5

View in context: https://bugs.webkit.org/attachment.cgi?id=159916&action=review

> Source/WebCore/css/StyleBuilder.cpp:2065
> +    setPropertyHandler(CSSPropertyWordWrap, ApplyPropertyDefault<EOverflowWrap, &RenderStyle::overflowWrap, EOverflowWrap, &RenderStyle::setOverflowWrap, EOverflowWrap, &RenderStyle::initialOverflowWrap>::createHandler());

This could probably use a comment explaining why we are setting EOverflowWrap for CSSPropertyWordWrap.

[Mikhail Pozdnyakov]

Created attachment 162438 [details]
to be landed

Added required comment. Rebased to master.

[WebKit Review Bot]

Attachment 162438 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/css/CSSPrimitiveValueMappings.h:2626:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 162438 [details]
to be landed

Rejecting attachment 162438 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
mit-queue/Source/WebKit/chromium/third_party/skia/gyp --revision 5398 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
45>At revision 5398.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13768374

[Mikhail Pozdnyakov]

(In reply to comment #25)
> (From update of attachment 162438 [details])
> Rejecting attachment 162438 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> mit-queue/Source/WebKit/chromium/third_party/skia/gyp --revision 5398 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 45>At revision 5398.
> 
> ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 
> ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> Updating webkit projects from gyp files...
> 
> Full output: http://queues.webkit.org/results/13768374

Merge conflict during commit: Conflict at '/trunk/LayoutTests/ChangeLog' at /usr/lib/git-core/git-svn line 570 
Sound weird...

[Dominik Röttsches (drott)]

Let's try again.

[WebKit Review Bot]

Comment on attachment 162438 [details]
to be landed

Clearing flags on attachment: 162438

Committed r127737: <http://trac.webkit.org/changeset/127737>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.