Bug 90958

Marking bug 90959 as dependency.

Adding W3C draft link, and Mozilla compliance (w/ -moz prefix): https://developer.mozilla.org/en/CSS/text-decoration-style

Created attachment 155536 [details] Patch Proposed patch. Please note that patches from bugs 92000 and bug 90958 (respectively) should land before this.

Note: EWS is not able to apply patch because it depends on patches from bug 92000 and bug 90959 to be landed first.

Created attachment 155653 [details] Patch Moved style data from StyleRareInheritedData to StyleRareNonInheritedData. Please note that patch from bug 92000 should land before this.

Created attachment 156828 [details] Patch Various fixes, including: simpler design, fixed failing layout tests, does not interfere with editing feature, fixed relayout/repaint when property differs, does not depend on bug 92000 anymore.

Comment on attachment 156828 [details] Patch Attachment 156828 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13439942

Created attachment 156849 [details] Patch Added Skia text decoration style enumerations boilerplate.

Comment on attachment 156849 [details] Patch Attachment 156849 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13446584 New failing tests: fast/css/text-decoration-style.html

Created attachment 156862 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 156955 [details] Patch Added chromium-linux expected results.

Comment on attachment 156955 [details] Patch Attachment 156955 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13444858 New failing tests: fast/css/text-decoration-style.html

Created attachment 156977 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 156955 [details] Patch Attachment 156955 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13450559 New failing tests: fast/css/text-decoration-style.html

Created attachment 156991 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 157019 [details] Patch The local generated image expectations for chromium-linux had font aliasing issues, now using ones generated by build bot.

Comment on attachment 157019 [details] Patch This patch, as part of the new CSS3 text decoration properties work, is now ready for review. The implementation has changed significantly since first patch proposal. Please not this is rebased against master, so for commit purposes, it is recommended that patch from bug 90959 gets landed first, then we rebase it against it before landing.

Comment on attachment 157019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157019&action=review > Source/WebCore/ChangeLog:15 > + The "wavy" value is parsed, but not rendered (requires platform support). Maybe I am misunderstanding you here, but wouldn't the correct thing to do, be to not parse it, when it is not supported? If you parse it but do not render it, it could override the fallback value of text-decoration-style. * { property: fallback; property: new-stuff; } In this case 'fallback' should apply whenever 'new-stuff' is not supported.

Comment on attachment 157019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157019&action=review This looks pretty good to me > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1209 > +static PassRefPtr<CSSValue> renderTextDecorationStyleFlagsToCSSValue(int textDecorationStyle) why int and not the enum? I see it is done like that elsewehre I just wondered why > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:410 > + case WavyStroke: notImplemented() ? bug report? add bug link? > Source/WebCore/rendering/InlineTextBox.cpp:914 > + DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1.0f)); doesnt the style guide say to use 1 and not 1.0f ? > Source/WebCore/rendering/InlineTextBox.cpp:968 > + // Double acts like Solid, only twice punctuation mark > Source/WebCore/rendering/InlineTextBox.cpp:977 > + // FIXME: Currently not supported For all ports? bug link?

My intention is to add all necessary boilerplate for "wavy" value support. As for the fallback support, this is what actually happens - paint it as a regular "solid" (initial value) text decoration line until all platforms have proper rendering support. You can check the fast/css/text-decoration-style.html expected result (the wavy value is painted as a solid line). That said, if not parsing "wavy" value at all is the best approach, I can modify the code accordingly. What do you think? (In reply to comment #18) > (From update of attachment 157019 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157019&action=review > > > Source/WebCore/ChangeLog:15 > > + The "wavy" value is parsed, but not rendered (requires platform support). > > Maybe I am misunderstanding you here, but wouldn't the correct thing to do, be to not parse it, when it is not supported? If you parse it but do not render it, it could override the fallback value of text-decoration-style. > * { > property: fallback; > property: new-stuff; > } > > In this case 'fallback' should apply whenever 'new-stuff' is not supported.

Thank you again for the review Kenneth :) Just answering the review questions below: (In reply to comment #19) > (From update of attachment 157019 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157019&action=review > > This looks pretty good to me > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1209 > > +static PassRefPtr<CSSValue> renderTextDecorationStyleFlagsToCSSValue(int textDecorationStyle) > > why int and not the enum? I see it is done like that elsewehre I just wondered why I am following the common used style as much as possible, and I asked myself the same question when implementing it :) I agree with you, this should use the enum value to avoid erroneous usage at build time. > > > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:410 > > + case WavyStroke: > > notImplemented() ? bug report? add bug link? There is a bug currently opened for it (bug 92868), which depends on this bug. I forgot to mention that, sorry :/ Will add a comment about it. > > > Source/WebCore/rendering/InlineTextBox.cpp:914 > > + DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1.0f)); > > doesnt the style guide say to use 1 and not 1.0f ? I used 1.0f because that was the previous value used, but indeed, using 1 is just more clear :) > > Source/WebCore/rendering/InlineTextBox.cpp:977 > > + // FIXME: Currently not supported > > For all ports? bug link? I've only tested the code for Qt and Chromium (Skia) ports. I've implemented support for all other missing values except "wavy" on Qt, but Skia also does not support "double" value yet. The bug link is the same described above (bug 92868), but thinking better now I should split this task in two new bugs, one related to Qt and other to Skia.

Created attachment 157275 [details] Patch Fixed issues pointed out by Kenneth.

Comment on attachment 157275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157275&action=review > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:411 > + // FIXME: Needs platform implementation. Please file bug after this has landed > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:181 > unsigned m_appearance : 6; // EAppearance > unsigned m_borderFit : 1; // EBorderFit > unsigned m_textCombine : 1; // CSS3 text-combine properties > + unsigned m_textDecorationStyle : TextDecorationStyleBits; // TextDecorationStyle > > unsigned m_wrapFlow: 3; // WrapFlow > unsigned m_wrapThrough: 1; // WrapThrough Why do this differently that everyone else? and not just write 3?

Comment on attachment 157275 [details] Patch Attachment 157275 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13451945

(In reply to comment #23) > (From update of attachment 157275 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157275&action=review > > > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:411 > > + // FIXME: Needs platform implementation. > > Please file bug after this has landed Right, the bug is already filled - bug 93509 ([Skia] Add support for text decoration "wavy" and "double" styles). I can add this information on the code if necessary. > > > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:181 > > unsigned m_appearance : 6; // EAppearance > > unsigned m_borderFit : 1; // EBorderFit > > unsigned m_textCombine : 1; // CSS3 text-combine properties > > + unsigned m_textDecorationStyle : TextDecorationStyleBits; // TextDecorationStyle > > > > unsigned m_wrapFlow: 3; // WrapFlow > > unsigned m_wrapThrough: 1; // WrapThrough > > Why do this differently that everyone else? and not just write 3? This follows the guideline from ETextDecoration enum, defined in Source/WebCore/rendering/style/RenderStyleConstants.h and used in Source/WebCore/rendering/style/StyleVisualData.h:52. I personally believe this is better approach because whenever "text-decoration-style" values changes in the specification, we can easily update the required bits in the same place where the values are updated, thus avoiding future errors only detected in runtime (the compiler doesn't check if the maximum value of enum can be stored on a 'n' bitsized unsigned variable).

(In reply to comment #24) > (From update of attachment 157275 [details]) > Attachment 157275 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/13451945 Inside http://queues.webkit.org/results/13451945 we can find: The following build commands failed: CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/GraphicsContextCG.o platform/graphics/cg/GraphicsContextCG.cpp normal x86_64 c++ com.apple.compilers.llvmgcc42 (1 failure) This does not seem to be related with the proposed patch, likely to be a build bot platform issue.

Created attachment 157557 [details] Patch Added detailed info about wavy value open bugs on ChangeLog, rebased after patch from bug 90959 got landed.

Comment on attachment 157557 [details] Patch Attachment 157557 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13471288

Comment on attachment 157557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157557&action=review > Source/WebCore/css/CSSParser.cpp:2110 > + // solid | double | dotted | dashed | wavy | inherit isn't this supposed to be [ solid | ... | wary ] | inherit? I am new to CSS so bear with me :-) The specs says "Inherited: no" so that just mean that it is not inherited by default? and not related to whether you can inherit? Firefox seems to support "inherit" here Is this being tested? > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178 > + unsigned m_textDecorationStyle : TextDecorationStyleBits; // TextDecorationStyle I still find it a bit weird that you are doing this, when noone else is doing it like that in this file. Then it would be better to convert it all in a separate patch and just follow consistency for now.

(In reply to comment #29) > (From update of attachment 157557 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157557&action=review > > > Source/WebCore/css/CSSParser.cpp:2110 > > + // solid | double | dotted | dashed | wavy | inherit > > isn't this supposed to be > > [ solid | ... | wary ] | inherit? > > I am new to CSS so bear with me :-) The specs says "Inherited: no" so that just mean that it is not inherited by default? and not related to whether you can inherit? Firefox seems to support "inherit" here > > Is this being tested? Ugh, I've copied the comment style of other properties and forgot to remove the "inherit" value :S You are right, it is explicitly said on the spec for the property not to be inherited and it is implemented as such (not inherited). Yes, It is currently being tested :) I am fixing the comment accordingly. > > > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178 > > + unsigned m_textDecorationStyle : TextDecorationStyleBits; // TextDecorationStyle > > I still find it a bit weird that you are doing this, when noone else is doing it like that in this file. Then it would be better to convert it all in a separate patch and just follow consistency for now. Alright, removing TextDecorationStyleBits as well.

Created attachment 157732 [details] Patch Fixed issues pointed out by Kenneth.

Comment on attachment 157732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157732&action=review Sorry for the delay in looking at your patches. > Source/WebCore/rendering/InlineTextBox.cpp:914 > + DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1)); It's not OK to use DEFINE_STATIC_LOCAL to define constants (const float textDecorationThickness = 1.f would be better). I couldn't find the rationale behind this use so I am holding the patch until I get a clarification here.

(In reply to comment #32) > (From update of attachment 157732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157732&action=review > > Sorry for the delay in looking at your patches. > > > Source/WebCore/rendering/InlineTextBox.cpp:914 > > + DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1)); > > It's not OK to use DEFINE_STATIC_LOCAL to define constants (const float textDecorationThickness = 1.f would be better). I couldn't find the rationale behind this use so I am holding the patch until I get a clarification here. The rationale for this variable is that its value is now used in multiple places (see Source/WebCore/rendering/InlineTextBox.cpp lines 934, 1003 and 1010) thus avoiding inconsistencies. I defined it as static because the given source file follows this style (lines 716, 742), and also because as a constant value, I though it wouldn't make sense to construct/destroy this variable every time the function is called (which looks like a lot of times). Said this, shall I change it for the suggested approach?

Comment on attachment 157732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157732&action=review >>> Source/WebCore/rendering/InlineTextBox.cpp:914 >>> + DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1)); >> >> It's not OK to use DEFINE_STATIC_LOCAL to define constants (const float textDecorationThickness = 1.f would be better). I couldn't find the rationale behind this use so I am holding the patch until I get a clarification here. > > The rationale for this variable is that its value is now used in multiple places (see Source/WebCore/rendering/InlineTextBox.cpp lines 934, 1003 and 1010) thus avoiding inconsistencies. I defined it as static because the given source file follows this style (lines 716, 742), and also because as a constant value, I though it wouldn't make sense to construct/destroy this variable every time the function is called (which looks like a lot of times). > > Said this, shall I change it for the suggested approach? Yes, please change. The macro ends up calling new and then dereferencing. > #define DEFINE_STATIC_LOCAL(type, name, arguments) \ > static type& name = *new type arguments > #endif This is probably not what you intended

Comment on attachment 157732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157732&action=review > Source/WebCore/css/CSSPrimitiveValueMappings.h:2214 > + default: I would remove the default label, that would enable the compiler to catch any mistakes at compile time. > Source/WebCore/rendering/InlineTextBox.cpp:757 > + int textStyle = styleToUse->textDecorationStyle(); An enum is technically an int but you lose a lot of information by not using the enum here. I would advocate passing the enum directly to paintDecoration (this means that textDecorations could modified in the same way). > Source/WebCore/rendering/InlineTextBox.cpp:911 > +void InlineTextBox::paintDecoration(GraphicsContext* context, const FloatPoint& boxOrigin, int deco, int style, const ShadowData* shadow) style is too general and usually refer to RenderStyle() in the render tree. textStyle or textStrokeStyle is better. > Source/WebCore/rendering/InlineTextBox.cpp:980 > + default: I would rather have a case value for each of the possible values for the enum so that the compiler can catch any forgotten case at compile time. > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178 > + unsigned m_textDecorationStyle : 3; // TextDecorationStyle You don't implement any invalidation logic currently. This means that any dynamic change to textDecorationStyle will not be repainted. This needs to be fixed and tested: the right fix is to implement a check in RenderStyle::diff and return StyleDifferenceRepaint

Comment on attachment 157732 [details] Patch A general comment, it's better to split parsing from actually implementing the feature which requires some flag (either compile or runtime) to avoid breaking CSS feature detection. This makes it easier for different domain experts to comment separately on the patch. On top of that, you wouldn't end up with a massive patch that reviewers usually avoid. Here as you expose the feature to the web, it is expected to work properly which is more difficult to get right than an incremental approach. Under this assumption, this patch needs another round.

Thanks for the comments Julien!

Hi Julien, thank you for your time :) I am about to send an updated version of the patch with the fixes pointed out by your review. I do, however, have small comments on that: (In reply to comment #35) > (From update of attachment 157732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157732&action=review > > > Source/WebCore/css/CSSPrimitiveValueMappings.h:2214 > > + default: > > I would remove the default label, that would enable the compiler to catch any mistakes at compile time. Good idea :) I removed this already from this patch, but almost all other switches on this source file uses "default", so I am going to handle these separately on bug 93781. > > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178 > > + unsigned m_textDecorationStyle : 3; // TextDecorationStyle > > You don't implement any invalidation logic currently. This means that any dynamic change to textDecorationStyle will not be repainted. This needs to be fixed and tested: the right fix is to implement a check in RenderStyle::diff and return StyleDifferenceRepaint I guess you skipped that out of the patch :) There is a line added already to Source/WebCore/rendering/style/StyleRender: diff --git a/Source/WebCore/rendering/style/RenderStyle.cpp b/Source/WebCore/rendering/style/RenderStyle.cpp index 2303ffa..a6bb525 100644 --- a/Source/WebCore/rendering/style/RenderStyle.cpp +++ b/Source/WebCore/rendering/style/RenderStyle.cpp @@ -646,6 +646,7 @@ StyleDifference RenderStyle::diff(const RenderStyle* other, unsigned& changedCon || rareInheritedData->userSelect != other->rareInheritedData->userSelect || rareNonInheritedData->userDrag != other->rareNonInheritedData->userDrag || rareNonInheritedData->m_borderFit != other->rareNonInheritedData->m_borderFit + || rareNonInheritedData->m_textDecorationStyle != other->rareNonInheritedData->m_textDecorationStyle || rareInheritedData->textFillColor != other->rareInheritedData->textFillColor || rareInheritedData->textStrokeColor != other->rareInheritedData->textStrokeColor || rareInheritedData->textEmphasisColor != other->rareInheritedData->textEmphasisColor

Created attachment 157905 [details] Patch Fixed issues pointed out by Julien and Kenneth.

Comment on attachment 157905 [details] Patch Attachment 157905 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13473967 New failing tests: animations/suspend-resume-animation-events.html

Created attachment 157906 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 157905 [details] Patch Attachment 157905 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13475848

Comment on attachment 157905 [details] Patch The following build commands failed: CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMSVGPathSegMovetoRel.o /Volumes/Data/WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMSVGPathSegMovetoRel.mm normal x86_64 objective-c++ com.apple.compilers.llvmgcc42 CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMSVGRect.o /Volumes/Data/WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMSVGRect.mm normal x86_64 objective-c++ com.apple.compilers.llvmgcc42 CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMSVGScriptElement.o /Volumes/Data/WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMSVGScriptElement.mm normal x86_64 objective-c++ com.apple.compilers.llvmgcc42 CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMSVGStopElement.o /Volumes/Data/WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMSVGStopElement.mm normal x86_64 objective-c++ com.apple.compilers.llvmgcc42 CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/FrameLoader.o loader/FrameLoader.cpp normal x86_64 c++ com.apple.compilers.llvmgcc42 CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/GraphicsContextCG.o platform/graphics/cg/GraphicsContextCG.cpp normal x86_64 c++ com.apple.compilers.llvmgcc42 (6 failures) This mac error and previous chromium build errors are not related to this patch - platform build issues (chromium already passing now), requesting flag again.

Created attachment 157915 [details] Patch Added missing assertions, "double" value works on Skia platform.

Comment on attachment 157915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157915&action=review I am not supportive of such massive changes (especially since there are several of them) and would prefer if those were split. You are a new contributor to WebCore which means that there are rough edges and there is so much that can be caught by inspection when the patch is so big. I would rather see you introduce an umbrella feature flag (runtime probably) for all the CSS 3 text changes and land your changes bits by bits so that each patch is atomic and well tested. That would also bring the average time for review down. The current patch is lacking enough testing to be landed on top of that. > Source/WebCore/ChangeLog:53 > + * rendering/style/StyleRareNonInheritedData.h: It's preferred to fill the function level entries. > LayoutTests/ChangeLog:27 > + * svg/css/getComputedStyle-basic-expected.txt: All in all, your testing is extremely poor. You implemented a way to get the value back from JavaScript (using getComputedStyle), yet it's not tested. So is the invalidation logic (which was indeed implemented in the previous patch, my bad) which would require a very simple repaint test.

Both parsing (bug 94093) and rendering (bug 94094) patches have landed. Marking bug as fixed.