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).
Created attachment 159408 [details] preliminary patch. Preliminary patch. Dedicated test case and ChangeLog description are coming.
Created attachment 159417 [details] patch Patch with layout test added.
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?
Comment on attachment 159417 [details] patch Maybe Jullien wants wants a look before you commit
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
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
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
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
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.
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.
Created attachment 159746 [details] patch v2 Julien's feedback is taken into consideration. Improved the added layout test and ChangeLog description.
Created attachment 159748 [details] patch v3 Fixed typo in ChangeLog.
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.
(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.
Created attachment 159906 [details] patch v4 Tab, thanks for your comments. I have made 'overflow-wrap' essential and 'text-wrap' a shorthand.
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.
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
(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.
Created attachment 159916 [details] patch v5 Kenneth, thanks for review and for the link!
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.
Any further comments? review?
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.
Created attachment 162438 [details] to be landed Added required comment. Rebased to master.
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.
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
(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...
Let's try again.
Comment on attachment 162438 [details] to be landed Clearing flags on attachment: 162438 Committed r127737: <http://trac.webkit.org/changeset/127737>
All reviewed patches have been landed. Closing bug.