| Summary: | [css3-text] Implement CSS3 text-decoration shorthand | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Bruno Abinader (history only) <bruno.abinader> | ||||||||||||||||||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Bruno Abinader <brunoabinader> | ||||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | 50167214, abecsi, allan.jensen, brunoabinader, buildbot, cmarcelo, commit-queue, dbates, dglazkov, donggwan.kim, eric, esprehn+autocc, glenn, gustavo, gyuyoung.kim, igor.oliveira, jchaffraix, jesus, kling, kojiishi, lamarque, macpherson, masayuki, menard, mitz, mmaxfield, noam, ojan.autocc, ojan, philn, rakuco, rniwa, senorblanco, simon.fraser, syoichi, tony, vestbo, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||
| URL: | http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-property | ||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 90958, 90959, 91638, 93863, 94093, 94094, 99804 | ||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 58491 | ||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Bruno Abinader (history only)
2012-07-23 07:41:39 PDT
Created attachment 153811 [details]
Proposed patch
This patch acts as a base for -line, -style and -color implementations. Currently, "-webkit-text-decoration*" does nothing, so no layout tests required for now.
I'm the implementer of CSS3 text-decoration on Gecko. I'd like to explain what we do for this property. Gecko handles the text-decoration property as both shorthand property and longhand property. I mean that CSSStyleDeclaration object returned from getComputedStyle() returns both null or used value for its textDecoration attribute. Here is actual code: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#2427 If the style or color is changed, it means the new CSS3 properties are used. Then, we assume that the author knows the text-decoration property is changed to a shorthand property. In this case, Gecko returns null for the attribute. Otherwise, we could break web pages written with CSS2.* if we returned null for it. Therefore, Gecko returns used value for that. Thanks for the info! I'll have a look at how it's implemented in Gecko :) I am considering this on the shorthand implementations, like "-webkit-text-emphasis" implementation does. But currently I am more interested in implementing the shorthand ones, then when these have landed I'll put effort in making "-webkit-text-decoration" work as expected in its full awesomeness! (In reply to comment #2) > I'm the implementer of CSS3 text-decoration on Gecko. I'd like to explain what we do for this property. > > Gecko handles the text-decoration property as both shorthand property and longhand property. > > I mean that CSSStyleDeclaration object returned from getComputedStyle() returns both null or used value for its textDecoration attribute. > > Here is actual code: > http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#2427 > > If the style or color is changed, it means the new CSS3 properties are used. Then, we assume that the author knows the text-decoration property is changed to a shorthand property. In this case, Gecko returns null for the attribute. > > Otherwise, we could break web pages written with CSS2.* if we returned null for it. Therefore, Gecko returns used value for that. I forgot to mention a fact. Note that Gecko doesn't allow web authors to specify the CSS3 properties to text-decoration (and also -moz-text-decoration). If the spec will be stable, we will support it. Created attachment 155021 [details]
Patch
Fixed inheritance for -line, -style and -color shorthands, as well as updated patch comments to better illustrate the purpose of this patch.
Created attachment 155146 [details]
Patch
Actually updated -line to be inherited, as prescribed in previous WebKit text-decoration implementation and CSS 2.1 prose.
If this is a draft specification, shouldn't this be disablable on compile-time? Or did the discussion on webkit-dev end up with something else? (In reply to comment #7) > If this is a draft specification, shouldn't this be disablable on compile-time? Or did the discussion on webkit-dev end up with something else? It clearly needs to go behind a feature flag. I'm looking for a css-text-specific ENABLE() but am not successful finding one so far (ie. CSS3 text-emphasis implementation also does not use a flag). Shall I create a new one, then? (In reply to comment #8) > (In reply to comment #7) > > If this is a draft specification, shouldn't this be disablable on compile-time? Or did the discussion on webkit-dev end up with something else? > > It clearly needs to go behind a feature flag. Created attachment 157234 [details]
Patch
Various fixes, including: simpler design, fixed failing layout tests, full backwards compatible with CSS2.1 text-decoration spec.
Comment on attachment 157234 [details] Patch Requesting review. EWS analysis (if submitted) will fail because this patch requires patches from bug 90959, bug 90958 and bug 91638 to be applied first. Created attachment 158833 [details]
Patch
Updated patch with additional layout tests and code now is guarded by feature flag.
Created attachment 159137 [details] Patch (EWS run only) This patch adds changes to make CSS3_TEXT_DECORATION feature flag enabled and remove skip of fast/css3-text-decoration layout test directory from chromium build (expected results included). This is intentional to get EWS results, but not intended for landing (as suggested by Peter Beverloo in http://lists.webkit.org/pipermail/webkit-dev/2012-August/021925.html ). This is a workaround to get proper EWS results for the previous patch. Comment on attachment 159137 [details] Patch (EWS run only) Attachment 159137 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13530109 New failing tests: fast/forms/range/slider-delete-while-dragging-thumb.html fast/selectors/017.html fast/loader/loadInProgress.html fast/loader/unload-form-post-about-blank.html css3/selectors3/html/css3-modsel-61.html fast/css/child-style-can-override-visited-style.html css2.1/t051103-c21-hover-ln-00-e-i.html fast/canvas/webgl/shader-precision-format.html css2.1/t060403-c21-pseu-cls-00-e-i.html http/tests/xmlhttprequest/zero-length-response.html css2.1/t0511-c21-pseud-link-03-e.html css2.1/t051103-c21-activ-ln-00-e-i.html css2.1/t051103-c21-focus-ln-00-e-i.html css3/selectors3/xml/css3-modsel-61.xml css3/selectors3/xml/css3-modsel-17.xml css2.1/t060403-c21-pseu-id-00-e-i.html fast/forms/range/slider-mouse-events.html fast/frames/cached-frame-counter.html css3/selectors3/html/css3-modsel-17.html fast/events/reveal-link-when-focused.html css1/text_properties/text_decoration.html fast/selectors/061.html css3/selectors3/xhtml/css3-modsel-17.xml css2.1/t0511-c21-pseud-link-02-e.html fast/forms/range/slider-onchange-event.html css2.1/20110323/c543-txt-decor-000.html css3/selectors3/xhtml/css3-modsel-61.xml Created attachment 159170 [details]
Archive of layout-test-results from gce-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
(In reply to comment #14) > (From update of attachment 159137 [details]) > Attachment 159137 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13530109 > > New failing tests: > fast/forms/range/slider-delete-while-dragging-thumb.html > fast/selectors/017.html > fast/loader/loadInProgress.html > fast/loader/unload-form-post-about-blank.html > css3/selectors3/html/css3-modsel-61.html > fast/css/child-style-can-override-visited-style.html > css2.1/t051103-c21-hover-ln-00-e-i.html > fast/canvas/webgl/shader-precision-format.html > css2.1/t060403-c21-pseu-cls-00-e-i.html > http/tests/xmlhttprequest/zero-length-response.html > css2.1/t0511-c21-pseud-link-03-e.html > css2.1/t051103-c21-activ-ln-00-e-i.html > css2.1/t051103-c21-focus-ln-00-e-i.html > css3/selectors3/xml/css3-modsel-61.xml > css3/selectors3/xml/css3-modsel-17.xml > css2.1/t060403-c21-pseu-id-00-e-i.html > fast/forms/range/slider-mouse-events.html > fast/frames/cached-frame-counter.html > css3/selectors3/html/css3-modsel-17.html > fast/events/reveal-link-when-focused.html > css1/text_properties/text_decoration.html > fast/selectors/061.html > css3/selectors3/xhtml/css3-modsel-17.xml > css2.1/t0511-c21-pseud-link-02-e.html > fast/forms/range/slider-onchange-event.html > css2.1/20110323/c543-txt-decor-000.html > css3/selectors3/xhtml/css3-modsel-61.xml This is not what the output link says (http://queues.webkit.org/results/13530109). Am I wrong or is it the failure output from some other build? Created attachment 161695 [details]
Patch
Simplified layout tests for readibility.
Comment on attachment 161695 [details] Patch Attachment 161695 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13721182 (In reply to comment #18) > (From update of attachment 161695 [details]) > Attachment 161695 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/13721182 This patch requires patches from bug 94094 and bug 91638 to be applied first. As EFL has the feature flag enabled by default, it crashed by missing "CSSPropertyWebKitTextDecorationColor" enum. Created attachment 161702 [details] Patch (EWS run only) Included required patches from bug 94094 and bug 91638. Comment on attachment 161702 [details] Patch (EWS run only) Attachment 161702 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13718207 Comment on attachment 161702 [details] Patch (EWS run only) Attachment 161702 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13713274 New failing tests: fast/css3-text-decoration/text-decoration-style.html css1/text_properties/text_decoration.html css2.1/20110323/c543-txt-decor-000.html fast/css3-text-decoration/repaint/repaint-text-decoration-style.html Created attachment 170667 [details]
Patch
Updates on namespace (now CSS3_TEXT), layout test directories and fixed segmentation fault when accessing an invalid pointer.
Comment on attachment 170667 [details] Patch Attachment 170667 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14563659 Created attachment 170972 [details] Patch (EWS only) More info on failing layout tests in bug 100546 "[css3-text] Provide pixel results for text decoration style layout tests". This patch also contains patch from dependency bug 91638. Comment on attachment 170972 [details] Patch (EWS only) Attachment 170972 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14610139 Comment on attachment 170972 [details] Patch (EWS only) Attachment 170972 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14549025 New failing tests: fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html fast/css3-text/css3-text-decoration/text-decoration-style.html fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-color.html Created attachment 171241 [details]
Patch
Fixed layout test internal paths after directory changes.
Created attachment 179064 [details] Patch Rebased after text-align-last changes and bug fix made in r134156. Comment on attachment 179064 [details] Patch Attachment 179064 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15280603 Created attachment 193065 [details] Patch Rebased after r145785. @Julien, do you mind reviewing this patch as well? :-) Comment on attachment 193065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193065&action=review > Source/WebCore/ChangeLog:23 > + This change is missing several important bits: * CSSComputedStyleDeclarations.cpp has a list of properties *excluding* shorthands (http://trac.webkit.org/browser/trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp#L94) which wasn't updated. * shorthands *shouldn't* be handled in StyleSelector / StyleBuilder and I don't see any change here. Btw, this now triggers an ASSERT after bug 111505 as it is wrong *not* to expand a shorthand into its longhands properties. > Source/WebCore/css/CSSParser.cpp:2297 > + // <text-decoration-line> || <text-decoration-style> || <text-decoration-color> || blink It's unclear to me why 'inherit' is not accepted anymore. It should be at least tested (in combination to the other comments on testing). This is probably a specification bug as they don't seem to accept that either. > Source/WebCore/css/CSSParser.cpp:9168 > + if (list->length() && (isValid || m_currentShorthand != CSSPropertyInvalid)) { I think this is better written: || isShorthand(). Also if you don't allow the extended parsing code if CSS3_TEXT is off, it seems like this should be common to both implementations. > Source/WebCore/css/CSSParser.cpp:9201 > + bool isShorthand = false; That is just wrong. The specification is fairly explicit that it is *always* a shorthand, not conditionally a shorthand. > LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-shorthand.js:11 > +shouldBeNull("e.style.getPropertyCSSValue('text-decoration')"); 'none' is different from null AFAICT, that doesn't seem correct. > LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-shorthand.js:48 > + This testing is lacking: you should be able to omit 'text-decoration-color' and 'text-decoration-style' and they would revert to their initial value ('none'). Also you should be able to change 'text-decoration-color' / 'text-decoration-style' and 'text-decoration-line' and see the result reflected when querying 'text-decoration'. Created attachment 204163 [details]
Proposed patch
Created attachment 204164 [details]
Proposed patch
Fixed typo
Created attachment 204165 [details]
Proposed patch
Fixed typo #2 (updated CSSPropertyWebkitTextDecoration comment on accepted values)
Created attachment 204166 [details]
Proposed patch
Added missing test expectation result
Comment on attachment 204166 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=204166&action=review > LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-shorthand.js:33 > +shouldBe("computedStyle.getPropertyCSSValue('-webkit-text-decoration-color').cssText", "'rgb(0, 0, 0)'"); You should check that the value returned by computedStyle is valid, meaning we can parse it again. Look getComputedStyle-background-shorthand checkComputedStyleValue(); Comment on attachment 204166 [details]
Proposed patch
You should implement the computed style for the shorthand too.
Created attachment 204338 [details]
Proposed patch
Added missing computed style calculation for shorthand property - thanks Alexis\!
Created attachment 206613 [details]
Patch
Rebased patch
Comment on attachment 206613 [details] Patch Attachment 206613 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1068121 New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html Created attachment 206619 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.3
(In reply to comment #43) > Created an attachment (id=206619) [details] > Archive of layout-test-results from webkit-ews-01 for mac-mountainlion > > The attached test failures were seen while running run-webkit-tests on the mac-ews. > Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.3 This failure is not related to this patch, it has been failing since r152433: http://build.webkit.org/TestFailures/#/Apple MountainLion Debug WK2 (Tests) Created attachment 209114 [details]
Patch
Updated patch from Blink's backport: - While in Blink it replaces previous text-decoration implementation, in WebKit it is added as a new, prefixed property -webkit-text-decoration; - No need to modify editing code - unprefixed text-decoration remains the same; - No need to modify layout test values - unprefixed text-decoration remains the same; Comment on attachment 209114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209114&action=review > Source/WebCore/css/StylePropertyShorthand.h:116 > const StylePropertyShorthand& webkitMaskRepeatShorthand(); > +#if ENABLE(CSS3_TEXT) > +const StylePropertyShorthand& webkitTextDecorationShorthand(); > +#endif // CSS3_TEXT > const StylePropertyShorthand& webkitTextEmphasisShorthand(); Awkward to have a conditional in the middle of a sorted list. Makes it hard to re-sort, for example. Instead would be better to put this in a separate sorted list/paragraph. The same way we do with includes. Also, the #endif comment really rubs me the wrong way when the #if is less than, say, 10 lines long. I understand the value of the comment when it might be scrolled off screen, but it seems like noise to me in these short ifs. Would be nice to remove some of these and maybe even make this official in our style guide. Unless others don’t agree with me on this. Comment on attachment 209114 [details]
Patch
Aborting commit to fix header inclusion as reviewed by Darin.
Comment on attachment 209114 [details] Patch Committed r154338: <http://trac.webkit.org/changeset/154338> @mmaxfield Safari nightly does not support text-decoration shorthand. |