Bug 116363 – -webkit-text-underline-position should not be inherited

• Component: CSS
• Status: RESOLVED FIXED

[Joseph Pecoraro]

The spec for this property says that it should not be inherited:
<http://www.w3.org/TR/2003/CR-css3-text-20030514/#text-underline-position>

However it returns true from CSSProperty::isInheritedProperty.

[Joseph Pecoraro]

Should be easy to fix. Hard part is making a test case. I could look into this later on, but if someone familiar with this wants to take a look; then rock on.

[Lamarque V. Souza]

Created attachment 202189 [details]
Patch

Proposed patch

[Bruno Abinader (history only)]

Comment on attachment 202189 [details]
Patch

>Subversion Revision: 150274
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c3462a41eeba42daea64b3dd52e1cd5a2921f833..685c3d43ebf4ebd16dd619c4daf21bda767fe233 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>[phone]  Lamarque V. Souza  <[e-mail]>
>+
>+        -webkit-text-underline-position should not be inherited
>+        https://bugs.webkit.org/show_bug.cgi?id=116363
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Specification says text-underline-position should not be inherited.
>+
>+        No new tests, this updates existing tests.
>+
>+        * css/CSSProperty.cpp:
>+        (WebCore::CSSProperty::isInheritedProperty): Make
>+        CSSPropertyWebkitTextUnderlinePosition return false.
>+        * rendering/style/RenderStyle.h: Treat TextUnderlinePosition as
>+        non-inherited.
>+        * rendering/style/StyleRareInheritedData.cpp:
>+        (WebCore::StyleRareInheritedData::StyleRareInheritedData):
>+        (WebCore::StyleRareInheritedData::operator==): Remove m_textUnderlinePosition.
>+        * rendering/style/StyleRareInheritedData.h:
>+        (StyleRareInheritedData):
>+        * rendering/style/StyleRareNonInheritedData.cpp:
>+        (WebCore::StyleRareNonInheritedData::StyleRareNonInheritedData):
>+        (WebCore::StyleRareNonInheritedData::operator==): Add m_textUnderlinePosition.
>+        * rendering/style/StyleRareNonInheritedData.h:
>+        (StyleRareNonInheritedData):
>+
> 2013-05-17  Darin Adler  <[e-mail]>
> 
>         [EFL] Move EFL port off legacy clipboard
>diff --git a/Source/WebCore/css/CSSProperty.cpp b/Source/WebCore/css/CSSProperty.cpp
>index bedf7d7e84b6cfb28aa2910912690ce5537507f9..b3fa626f54a231e25f973d6e455fe42dae946d93 100644
>--- a/Source/WebCore/css/CSSProperty.cpp
>+++ b/Source/WebCore/css/CSSProperty.cpp
>@@ -332,7 +332,6 @@ bool CSSProperty::isInheritedProperty(CSSPropertyID propertyID)
>     case CSSPropertyWebkitTextDecorationLine:
>     case CSSPropertyWebkitTextAlignLast:
>     case CSSPropertyWebkitTextJustify:
>-    case CSSPropertyWebkitTextUnderlinePosition:
> #endif // CSS3_TEXT
>     case CSSPropertyWebkitTextDecorationsInEffect:
>     case CSSPropertyWebkitTextEmphasis:
>@@ -648,6 +647,7 @@ bool CSSProperty::isInheritedProperty(CSSPropertyID propertyID)
> #if ENABLE(CSS3_TEXT)
>     case CSSPropertyWebkitTextDecorationStyle:
>     case CSSPropertyWebkitTextDecorationColor:
>+    case CSSPropertyWebkitTextUnderlinePosition:
> #endif // CSS3_TEXT
>     case CSSPropertyWebkitTransform:
>     case CSSPropertyWebkitTransformOrigin:
>diff --git a/Source/WebCore/rendering/style/RenderStyle.h b/Source/WebCore/rendering/style/RenderStyle.h
>index 908ec30ce32d6ad868752a986ff1d44e7125d16d..fb2a2732239d4d1cc2b6ced01bb5c6a91b5529a1 100644
>--- a/Source/WebCore/rendering/style/RenderStyle.h
>+++ b/Source/WebCore/rendering/style/RenderStyle.h
>@@ -580,7 +580,7 @@ public:
>     TextDecorationStyle textDecorationStyle() const { return static_cast<TextDecorationStyle>(rareNonInheritedData->m_textDecorationStyle); }
>     TextAlignLast textAlignLast() const { return static_cast<TextAlignLast>(rareInheritedData->m_textAlignLast); }
>     TextJustify textJustify() const { return static_cast<TextJustify>(rareInheritedData->m_textJustify); }
>-    TextUnderlinePosition textUnderlinePosition() const { return static_cast<TextUnderlinePosition>(rareInheritedData->m_textUnderlinePosition); }
>+    TextUnderlinePosition textUnderlinePosition() const { return static_cast<TextUnderlinePosition>(rareNonInheritedData->m_textUnderlinePosition); }
> #else
>     TextDecorationStyle textDecorationStyle() const { return TextDecorationStyleSolid; }
> #endif // CSS3_TEXT
>@@ -1150,7 +1150,7 @@ public:
>     void setTextDecorationStyle(TextDecorationStyle v) { SET_VAR(rareNonInheritedData, m_textDecorationStyle, v); }
>     void setTextAlignLast(TextAlignLast v) { SET_VAR(rareInheritedData, m_textAlignLast, v); }
>     void setTextJustify(TextJustify v) { SET_VAR(rareInheritedData, m_textJustify, v); }
>-    void setTextUnderlinePosition(TextUnderlinePosition v) { SET_VAR(rareInheritedData, m_textUnderlinePosition, v); }
>+    void setTextUnderlinePosition(TextUnderlinePosition v) { SET_VAR(rareNonInheritedData, m_textUnderlinePosition, v); }
> #endif // CSS3_TEXT
>     void setDirection(TextDirection v) { inherited_flags._direction = v; }
>     void setLineHeight(Length specifiedLineHeight);
>diff --git a/Source/WebCore/rendering/style/StyleRareInheritedData.cpp b/Source/WebCore/rendering/style/StyleRareInheritedData.cpp
>index 2e1d9d0a6a3952d6995b0a5bdf3a5bf4062fafc1..b73a9baa1969fd52be4671aabc2390d4cc3febc5 100644
>--- a/Source/WebCore/rendering/style/StyleRareInheritedData.cpp
>+++ b/Source/WebCore/rendering/style/StyleRareInheritedData.cpp
>@@ -106,7 +106,6 @@ StyleRareInheritedData::StyleRareInheritedData()
> #if ENABLE(CSS3_TEXT)
>     , m_textAlignLast(RenderStyle::initialTextAlignLast())
>     , m_textJustify(RenderStyle::initialTextJustify())
>-    , m_textUnderlinePosition(RenderStyle::initialTextUnderlinePosition())
> #endif // CSS3_TEXT
>     , m_rubyPosition(RenderStyle::initialRubyPosition())
>     , hyphenationLimitBefore(-1)
>@@ -181,7 +180,6 @@ StyleRareInheritedData::StyleRareInheritedData(const StyleRareInheritedData& o)
> #if ENABLE(CSS3_TEXT)
>     , m_textAlignLast(o.m_textAlignLast)
>     , m_textJustify(o.m_textJustify)
>-    , m_textUnderlinePosition(o.m_textUnderlinePosition)
> #endif // CSS3_TEXT
>     , m_rubyPosition(o.m_rubyPosition)
>     , hyphenationString(o.hyphenationString)
>@@ -291,7 +289,6 @@ bool StyleRareInheritedData::operator==(const StyleRareInheritedData& o) const
> #if ENABLE(CSS3_TEXT)
>         && m_textAlignLast == o.m_textAlignLast
>         && m_textJustify == o.m_textJustify
>-        && m_textUnderlinePosition == o.m_textUnderlinePosition
> #endif // CSS3_TEXT
>         && m_rubyPosition == o.m_rubyPosition
>         && m_lineSnap == o.m_lineSnap
>diff --git a/Source/WebCore/rendering/style/StyleRareInheritedData.h b/Source/WebCore/rendering/style/StyleRareInheritedData.h
>index 34b80ea525b47c53922699e270405c054e85e906..d8c0da0e84a15b343b4401c90298cb190f99360b 100644
>--- a/Source/WebCore/rendering/style/StyleRareInheritedData.h
>+++ b/Source/WebCore/rendering/style/StyleRareInheritedData.h
>@@ -120,7 +120,6 @@ public:
> #if ENABLE(CSS3_TEXT)
>     unsigned m_textAlignLast : 3; // TextAlignLast
>     unsigned m_textJustify : 3; // TextJustify
>-    unsigned m_textUnderlinePosition : 3; // TextUnderlinePosition
> #endif // CSS3_TEXT
>     unsigned m_rubyPosition : 1; // RubyPosition
> 
>diff --git a/Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp b/Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp
>index f5c6bb828b09e01050372fc3d8038ff5cb775c6c..556c3dd58fd4b0ebebfc6b13c6c8509855c0a342 100644
>--- a/Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp
>+++ b/Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp
>@@ -75,6 +75,7 @@ StyleRareNonInheritedData::StyleRareNonInheritedData()
>     , m_textCombine(RenderStyle::initialTextCombine())
> #if ENABLE(CSS3_TEXT)
>     , m_textDecorationStyle(RenderStyle::initialTextDecorationStyle())
>+    , m_textUnderlinePosition(RenderStyle::initialTextUnderlinePosition())
> #endif // CSS3_TEXT
>     , m_wrapFlow(RenderStyle::initialWrapFlow())
>     , m_wrapThrough(RenderStyle::initialWrapThrough())
>@@ -158,6 +159,7 @@ StyleRareNonInheritedData::StyleRareNonInheritedData(const StyleRareNonInherited
>     , m_textCombine(o.m_textCombine)
> #if ENABLE(CSS3_TEXT)
>     , m_textDecorationStyle(o.m_textDecorationStyle)
>+    , m_textUnderlinePosition(o.m_textUnderlinePosition)
> #endif // CSS3_TEXT
>     , m_wrapFlow(o.m_wrapFlow)
>     , m_wrapThrough(o.m_wrapThrough)
>@@ -247,6 +249,7 @@ bool StyleRareNonInheritedData::operator==(const StyleRareNonInheritedData& o) c
>         && m_textCombine == o.m_textCombine
> #if ENABLE(CSS3_TEXT)
>         && m_textDecorationStyle == o.m_textDecorationStyle
>+        && m_textUnderlinePosition == o.m_textUnderlinePosition
> #endif // CSS3_TEXT
>         && m_wrapFlow == o.m_wrapFlow
>         && m_wrapThrough == o.m_wrapThrough
>diff --git a/Source/WebCore/rendering/style/StyleRareNonInheritedData.h b/Source/WebCore/rendering/style/StyleRareNonInheritedData.h
>index 0a3c7a8cac1bc20a0e445d9733d705a605bd11cc..8edf5216f96c9b8439aaee3eb7ff4dc0eb135a42 100644
>--- a/Source/WebCore/rendering/style/StyleRareNonInheritedData.h
>+++ b/Source/WebCore/rendering/style/StyleRareNonInheritedData.h
>@@ -183,6 +183,7 @@ public:
> 
> #if ENABLE(CSS3_TEXT)
>     unsigned m_textDecorationStyle : 3; // TextDecorationStyle
>+    unsigned m_textUnderlinePosition : 3; // TextUnderlinePosition
> #endif // CSS3_TEXT
>     unsigned m_wrapFlow: 3; // WrapFlow
>     unsigned m_wrapThrough: 1; // WrapThrough
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 8f6fbe4d411534539b133883d0dea6c067f34c74..c759ef4119c4c439a8333bfec3379d1ecc6a8955 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>[phone]  Lamarque V. Souza  <[e-mail]>
>+
>+        -webkit-text-underline-position should not be inherited
>+        https://bugs.webkit.org/show_bug.cgi?id=116363
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Update expected results.
>+
>+        * fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt:
>+        * fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js:
>+
> 2013-05-17  Frédéric Wang  <[e-mail]>
> 
>         Bad spacing inside MathML formulas when text-indent is specified
>diff --git a/LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt b/LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt
>index 89c6f8ced66876f323b40e51738ab21c9a086919..e5b77ee2a4cedf070dc699295b731c5f9f4a9433 100644
>--- a/LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt
>+++ b/LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt
>@@ -47,11 +47,11 @@ PASS computedStyle.webkitTextUnderlinePosition is 'under'
> PASS computedStyle.getPropertyCSSValue('-webkit-text-underline-position').toString() is '[object CSSPrimitiveValue]'
> PASS computedStyle.getPropertyCSSValue('-webkit-text-underline-position').cssText is 'under'
> 
>-Ancestor inherits values from parent:
>+Ancestor does not inherit values from parent:
> PASS e.style.getPropertyCSSValue('-webkit-text-underline-position') is null
>-PASS computedStyle.webkitTextUnderlinePosition is 'under'
>+PASS computedStyle.webkitTextUnderlinePosition is 'auto'
> PASS computedStyle.getPropertyCSSValue('-webkit-text-underline-position').toString() is '[object CSSPrimitiveValue]'
>-PASS computedStyle.getPropertyCSSValue('-webkit-text-underline-position').cssText is 'under'
>+PASS computedStyle.getPropertyCSSValue('-webkit-text-underline-position').cssText is 'auto'
> 
> Value 'auto alphabetic':
> PASS e.style.getPropertyCSSValue('-webkit-text-underline-position') is null
>diff --git a/LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js b/LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js
>index a9e1b15786698a20c995eff96dcf9bae1cf503fd..9bae5b7a11e1a85209ce2520411a138800474c7f 100644
>--- a/LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js
>+++ b/LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js
>@@ -65,10 +65,10 @@ testComputedStyle("webkitTextUnderlinePosition", "-webkit-text-underline-positio
> debug('');
> 
> testContainer.innerHTML = '<div id="test-parent" style="-webkit-text-underline-position: under;">hello <span id="test-ancestor">world</span></div>';
>-debug("Ancestor inherits values from parent:");
>+debug("Ancestor does not inherit values from parent:");
> e = document.getElementById('test-ancestor');
> testElementStyle("webkitTextUnderlinePosition", "-webkit-text-underline-position", null, "");
>-testComputedStyle("webkitTextUnderlinePosition", "-webkit-text-underline-position", "[object CSSPrimitiveValue]", "under");
>+testComputedStyle("webkitTextUnderlinePosition", "-webkit-text-underline-position", "[object CSSPrimitiveValue]", "auto");
> debug('');
> 
> debug("Value 'auto alphabetic':");

[WebKit Commit Bot]

Comment on attachment 202189 [details]
Patch

Clearing flags on attachment: 202189

Committed r150366: <http://trac.webkit.org/changeset/150366>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yuki Sekiguchi]

Hi Lamarque, Joseph and Dean,

I think this should be reverted because the spec which Joseph quoted is very old and new spec says it should be inherited:
Current Working Draft: http://www.w3.org/TR/2013/WD-css-text-decor-3-20130103/#text-underline-position-property
Current Editor's Draft: http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property

If I am wrong, we should fix test cases.
Please look at text-underline-position-under.html:
http://trac.webkit.org/browser/trunk/LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under.html
This patch make all "lorem ipsum"s -webkit-underline-position: auto, and underline doesn't align.

[Joseph Pecoraro]

(In reply to comment #6)
> Hi Lamarque, Joseph and Dean,
> 
> I think this should be reverted because the spec which Joseph quoted is very old and new spec says it should be inherited:
> Current Working Draft: http://www.w3.org/TR/2013/WD-css-text-decor-3-20130103/#text-underline-position-property
> Current Editor's Draft: http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property
> 
> If I am wrong, we should fix test cases.
> Please look at text-underline-position-under.html:
> http://trac.webkit.org/browser/trunk/LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under.html
> This patch make all "lorem ipsum"s -webkit-underline-position: auto, and underline doesn't align.

Doh, if that is the case then yes, this should be reverted. Navigating the different CSS specs is a mess. I had clicked the "Latest Version" link at the top of the spec thinking that would be enough. Apparently not.

[Dean Jackson]

(In reply to comment #7)

> Doh, if that is the case then yes, this should be reverted. Navigating the different CSS specs is a mess. I had clicked the "Latest Version" link at the top of the spec thinking that would be enough. Apparently not.

Indeed. Reverting now.

[Dean Jackson]

Committed r150941: <http://trac.webkit.org/changeset/150941>

[Yuki Sekiguchi]

Thank you!