Remove 'alphabetic' value from text-underline-position

Remove 'alphabetic' value from text-underline-position

According to latest version of CSS3 Text Decoration W3C standard (W3C
Candidate Recommendation 1 August 2013), the text-underline-position
property does not include 'alphabetic' as valid property value, thus the
need to remove it from implementation.

David Baron's feedback on why removing this:
http://lists.w3.org/Archives/Public/www-style/2013Mar/0529.html

Specification link:
http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position

BUG=311335
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161361

[eae, 2013-11-04 15:53:49]

On 2013/11/04 13:19:13, abinader wrote:

Have you checked the usage numbers for alphabetic? If it is widely used we might
want to deprecate it before removing it all together or at the very least
announce the plans to remove it. See the "Intent to remove" type mails on
blink-dev.

[Julien - ping for review, 2013-11-04 16:06:42]

@Bruno, please link to the Editor's Draft when quoting a specification as this
is the most up-to-date version:
http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position

In this case, the value was removed after David Baron's feeback:
http://lists.w3.org/Archives/Public/www-style/2013Mar/0529.html (always good to
include the history)

> Have you checked the usage numbers for alphabetic?

This feature is under a runtime flag disabled by default so that shouldn't
matter. Because of that, we also don't really need an intent to remove :)

https://codereview.chromium.org/57493002/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/57493002/diff/1/LayoutTests/TestExpectations#...
LayoutTests/TestExpectations:489: crbug.com/311335
fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html
[ NeedsRebaseline ]
This will not apply.

https://codereview.chromium.org/57493002/diff/1/LayoutTests/fast/css3-text/cs...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js
(right):

https://codereview.chromium.org/57493002/diff/1/LayoutTests/fast/css3-text/cs...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js:74:
e.style.textUnderlinePosition = 'under auto';
Please don't change the test, this was here to test that the implementation
shouldn't allow multiple under (the original implementation did). It's fine to
add extra testing but I don't see the point in this change.

https://codereview.chromium.org/57493002/diff/1/LayoutTests/fast/css3-text/cs...
File
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html
(left):

https://codereview.chromium.org/57493002/diff/1/LayoutTests/fast/css3-text/cs...
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html:14:
'p' letter. Bugzilla link: http://webkit.org/b/102795.</p>
Let's keep the bugzilla link as it still gives some information.

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSParser-in.cpp
File Source/core/css/CSSParser-in.cpp (left):

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSParser-in....
Source/core/css/CSSParser-in.cpp:9139: // "auto | alphabetic | under" for now.
I would rather keep this comment so that we know what's missing in this
function.

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSParser-in.cpp
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSParser-in....
Source/core/css/CSSParser-in.cpp:2185: // FIXME: Implement support for 'left'
and 'right' values.
This FIXME is too far from the implementation and will likely be forgotten as we
update the code.

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSPrimitiveV...
File Source/core/css/CSSPrimitiveValueMappings.h (left):

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSPrimitiveV...
Source/core/css/CSSPrimitiveValueMappings.h:2533: // FIXME: Implement support
for 'under left' and 'under right' values.
What's wrong with this comment that you need to move it?

https://codereview.chromium.org/57493002/diff/1/Source/core/rendering/InlineT...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/57493002/diff/1/Source/core/rendering/InlineT...
Source/core/rendering/InlineTextBox.cpp:925: // According to the specification
TextUnderlinePositionAuto should default to 'alphabetic' for horizontal text
This comment should be updated as 'alphabetic' was removed. Based on the comment
below about the alphabetic baseline, this part could be safely removed.

https://codereview.chromium.org/57493002/diff/1/Source/core/rendering/style/R...
File Source/core/rendering/style/RenderStyleConstants.h (left):

https://codereview.chromium.org/57493002/diff/1/Source/core/rendering/style/R...
Source/core/rendering/style/RenderStyleConstants.h:381: // FIXME: Implement
support for 'under left' and 'under right' values.
I think the old comment still applies as we need to implement the behavior with
'under'.

[eae, 2013-11-04 16:15:56]

On 2013/11/04 16:06:42, Julien Chaffraix - PST wrote:
> @Bruno, please link to the Editor's Draft when quoting a specification as this
> is the most up-to-date version:
> http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position
> 
> In this case, the value was removed after David Baron's feeback:
> http://lists.w3.org/Archives/Public/www-style/2013Mar/0529.html (always good
to
> include the history)
> 
> > Have you checked the usage numbers for alphabetic?
> 
> This feature is under a runtime flag disabled by default so that shouldn't
> matter. Because of that, we also don't really need an intent to remove :)

Ah, that certainly makes it easier! Thanks.

[abinader, 2013-11-04 16:38:01]

https://codereview.chromium.org/57493002/diff/1/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/57493002/diff/1/LayoutTests/TestExpectations#...
LayoutTests/TestExpectations:489: crbug.com/311335
fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html
[ NeedsRebaseline ]
On 2013/11/04 16:06:43, Julien Chaffraix - PST wrote:
> This will not apply.

I thought so... what do you suggest? NeedsManualRebaseline?

https://codereview.chromium.org/57493002/diff/1/LayoutTests/fast/css3-text/cs...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js
(right):

https://codereview.chromium.org/57493002/diff/1/LayoutTests/fast/css3-text/cs...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js:74:
e.style.textUnderlinePosition = 'under auto';
On 2013/11/04 16:06:43, Julien Chaffraix - PST wrote:
> Please don't change the test, this was here to test that the implementation
> shouldn't allow multiple under (the original implementation did). It's fine to
> add extra testing but I don't see the point in this change.

Got that, will undo.

https://codereview.chromium.org/57493002/diff/1/LayoutTests/fast/css3-text/cs...
File
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html
(left):

https://codereview.chromium.org/57493002/diff/1/LayoutTests/fast/css3-text/cs...
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html:14:
'p' letter. Bugzilla link: http://webkit.org/b/102795.</p>
On 2013/11/04 16:06:43, Julien Chaffraix - PST wrote:
> Let's keep the bugzilla link as it still gives some information.

Ack!

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSParser-in.cpp
File Source/core/css/CSSParser-in.cpp (left):

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSParser-in....
Source/core/css/CSSParser-in.cpp:9139: // "auto | alphabetic | under" for now.
On 2013/11/04 16:06:43, Julien Chaffraix - PST wrote:
> I would rather keep this comment so that we know what's missing in this
> function.

Ack!

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSParser-in.cpp
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSParser-in....
Source/core/css/CSSParser-in.cpp:2185: // FIXME: Implement support for 'left'
and 'right' values.
On 2013/11/04 16:06:43, Julien Chaffraix - PST wrote:
> This FIXME is too far from the implementation and will likely be forgotten as
we
> update the code.

OK, will remove that.

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSPrimitiveV...
File Source/core/css/CSSPrimitiveValueMappings.h (left):

https://codereview.chromium.org/57493002/diff/1/Source/core/css/CSSPrimitiveV...
Source/core/css/CSSPrimitiveValueMappings.h:2533: // FIXME: Implement support
for 'under left' and 'under right' values.
On 2013/11/04 16:06:43, Julien Chaffraix - PST wrote:
> What's wrong with this comment that you need to move it?

I thought it would make more sense by having the comment before the switch case,
but that's cosmetics, indeed. I'll undo this.

https://codereview.chromium.org/57493002/diff/1/Source/core/rendering/InlineT...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/57493002/diff/1/Source/core/rendering/InlineT...
Source/core/rendering/InlineTextBox.cpp:925: // According to the specification
TextUnderlinePositionAuto should default to 'alphabetic' for horizontal text
On 2013/11/04 16:06:43, Julien Chaffraix - PST wrote:
> This comment should be updated as 'alphabetic' was removed. Based on the
comment
> below about the alphabetic baseline, this part could be safely removed.

Ack!

https://codereview.chromium.org/57493002/diff/1/Source/core/rendering/style/R...
File Source/core/rendering/style/RenderStyleConstants.h (left):

https://codereview.chromium.org/57493002/diff/1/Source/core/rendering/style/R...
Source/core/rendering/style/RenderStyleConstants.h:381: // FIXME: Implement
support for 'under left' and 'under right' values.
On 2013/11/04 16:06:43, Julien Chaffraix - PST wrote:
> I think the old comment still applies as we need to implement the behavior
with
> 'under'.

Ack!

[Julien - ping for review, 2013-11-05 17:03:57]

lgtm

https://codereview.chromium.org/57493002/diff/160001/LayoutTests/fast/css3-te...
File
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html
(right):

https://codereview.chromium.org/57493002/diff/160001/LayoutTests/fast/css3-te...
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html:13:
cross the 'pii' letter. Bugzilla link: http://webkit.org/b/102795.</p>
is 'pii' a new letter? :)

[abinader, 2013-11-05 17:07:40]

On 2013/11/05 17:03:57, Julien Chaffraix - PST wrote:
> lgtm
> 
>
https://codereview.chromium.org/57493002/diff/160001/LayoutTests/fast/css3-te...
> File
>
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html
> (right):
> 
>
https://codereview.chromium.org/57493002/diff/160001/LayoutTests/fast/css3-te...
>
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html:13:
> cross the 'pii' letter. Bugzilla link: http://webkit.org/b/102795.%3C/p>
> is 'pii' a new letter? :)

w00t? I could blame Vi's "insert mode" with lagged console output but I guess
that wouldn't count ;) Fixing now.

[commit-bot: I haz the power, 2013-11-05 17:12:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bruno.d@partner.samsung.com/57493002/2...

[commit-bot: I haz the power, 2013-11-05 20:38:04]

Change committed as 161361