Bug 112615

Summary:

[css3-text] Implement support for vertical writing mode in -webkit-text-underline-position

Product:

WebKit

Reporter:

Lamarque V. Souza <lamarque>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED DUPLICATE

Severity:

Enhancement

CC:

50167214, bruno.abinader, commit-queue, enrica, esprehn+autocc, glenn, macpherson, menard, ntim, rniwa, yuki.sekiguchi

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

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

Bug Depends on:

Bug Blocks:

101931

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	beidson: review-

Lamarque V. Souza

Reported 2013-03-18 13:36:03 PDT

The CSS3 property "text-decoration-style" accepts the following values: "auto | alphabetic | [ under || [ left | right ] ]". Support for values "auto | alphabetic | under" was implemented in bugs https://bugs.webkit.org/show_bug.cgi?id=102491 and https://bugs.webkit.org/show_bug.cgi?id=102795. This bug intents to implement the support for values "under left" and "under right", which only applies for vertical writing mode according to the specification: http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property

Attachments
Patch (139.16 KB, patch) 2013-04-15 01:38 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Patch (140.21 KB, patch) 2013-05-24 02:09 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Patch (140.28 KB, patch) 2013-05-24 03:15 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Patch (160.49 KB, patch) 2013-05-27 04:16 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Patch (145.43 KB, patch) 2013-05-28 01:09 PDT, Yuki Sekiguchi	no flags	Details Formatted Diff Diff
Patch (143.94 KB, patch) 2013-06-27 23:23 PDT, Yuki Sekiguchi	beidson: review-	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Yuki Sekiguchi

Comment 1 2013-04-15 01:38:49 PDT

Created attachment 198033 [details] Patch

Lamarque V. Souza

Comment 2 2013-04-15 07:43:30 PDT

View in context: https://bugs.webkit.org/attachment.cgi?id=198033&action=review > Source/WebCore/css/CSSParser.cpp:9340 > // However, values 'left' and 'right' are not implemented yet, so we will parse sintax This comment should be removed. > Source/WebCore/css/CSSParser.cpp:9352 > + case CSSValueRight: The part of the parser is incorret. The only acceptable values are either 'left', 'right', 'under left' or 'under right'. No other combination should be accepted. Here you accept things like 'left right', 'under alphabetic', 'right auto', etc. I created a valid parser in https://bug-102491-attachments.webkit.org/attachment.cgi?id=191578 but I removed since I was not planning to implement the vertical text support so soon. > Source/WebCore/rendering/InlineTextBox.cpp:995 > // and to 'under Left' for vertical text (e.g. japanese). We support only horizontal text for now. Remove the "We support only horizontal text for now." comment. > Source/WebCore/rendering/InlineTextBox.cpp:996 > + bool drawsUnderLeft; You can detect 'under left' and 'under right' values from underlinePosition variable. You do not need those two bools. 'under left' and 'under right' are mutually exclusive by the way. > Source/WebCore/rendering/InlineTextBox.cpp:1022 > + bool isOpposite; What does exactly isOpposite mean? Opposite to what? > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:26 > +left right :p The line should be alphabetic. "left right" is not a valid value for text-underline-position according to the specification. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:29 > +alphabetic right :p The line should be alphabetic. also not a valid value. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:32 > +under right left :p The line should be alphabetic. also not a valid value. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:26 > +left right :p The line should be alphabetic. not valid value. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:29 > +alphabetic right :p The line should be alphabetic. not valid value. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:32 > +under right left :p The line should be alphabetic. not valid value.

Yuki Sekiguchi

Comment 3 2013-05-24 02:09:47 PDT

Created attachment 202789 [details] Patch

Yuki Sekiguchi

Comment 4 2013-05-24 03:15:42 PDT

Created attachment 202797 [details] Patch

Lamarque V. Souza

Comment 5 2013-05-24 08:12:13 PDT

I did not have time to review all the patch, so I will write only the things I spotted in the first glance: View in context: https://bugs.webkit.org/attachment.cgi?id=202797&action=review > Source/WebCore/ChangeLog:12 > + because these line should ignore veritcal-align like under underline in horizontal writing mode. typo: "veritcal-align" > Source/WebCore/rendering/InlineTextBox.cpp:1011 > + default: Do not move this code into the default statement. Read comment https://bugs.webkit.org/show_bug.cgi?id=102795#c63 about this. > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:26 > +left right :p The line should be alphabetic. This is an invalid value. It is ok that your patch defaults to alphabetic here. However, your patch should update LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position.html to check for this and other invalid values. Anything that is not either "auto", "alphabetic", "under", "under left" or "under right" is invalid.

Yuki Sekiguchi

Comment 6 2013-05-27 04:16:12 PDT

Created attachment 202967 [details] Patch

WebKit Commit Bot

Comment 7 2013-05-27 04:18:32 PDT

Attachment 202967 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/style.css', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-double.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right.html', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal-expected.txt', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h']" exit_code: 1 Source/WebCore/rendering/InlineTextBox.cpp:984: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yuki Sekiguchi

Comment 8 2013-05-27 04:20:46 PDT

Thank you for your advice. (In reply to comment #2) > View in context: https://bugs.webkit.org/attachment.cgi?id=198033&action=review > > > Source/WebCore/css/CSSParser.cpp:9340 > > // However, values 'left' and 'right' are not implemented yet, so we will parse sintax > > This comment should be removed. Removed. > > Source/WebCore/css/CSSParser.cpp:9352 > > + case CSSValueRight: > > The part of the parser is incorret. The only acceptable values are either 'left', 'right', 'under left' or 'under right'. No other combination should be accepted. Here you accept things like 'left right', 'under alphabetic', 'right auto', etc. I created a valid parser in https://bug-102491-attachments.webkit.org/attachment.cgi?id=191578 but I removed since I was not planning to implement the vertical text support so soon. Fixed. > > Source/WebCore/rendering/InlineTextBox.cpp:995 > > // and to 'under Left' for vertical text (e.g. japanese). We support only horizontal text for now. > > Remove the "We support only horizontal text for now." comment. Removed. > > Source/WebCore/rendering/InlineTextBox.cpp:996 > > + bool drawsUnderLeft; > > You can detect 'under left' and 'under right' values from underlinePosition variable. You do not need those two bools. 'under left' and 'under right' are mutually exclusive by the way. Changed to use one value. > > Source/WebCore/rendering/InlineTextBox.cpp:1022 > > + bool isOpposite; > > What does exactly isOpposite mean? Opposite to what? Just removed. > > +left right :p The line should be alphabetic. > > "left right" is not a valid value for text-underline-position according to the specification. > > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:29 > > +alphabetic right :p The line should be alphabetic. > > also not a valid value. > > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:32 > > +under right left :p The line should be alphabetic. > > also not a valid value. > > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:26 > > +left right :p The line should be alphabetic. > > not valid value. > > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:29 > > +alphabetic right :p The line should be alphabetic. > > not valid value. > > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html:32 > > +under right left :p The line should be alphabetic. > > not valid value. Yes, these are not valid value. We should check invalid case. These test cases found my bugs, so I think adding this to trunk is valuable.

Yuki Sekiguchi

Comment 9 2013-05-27 04:27:04 PDT

(In reply to comment #5) > I did not have time to review all the patch, so I will write only the things I spotted in the first glance: > > View in context: https://bugs.webkit.org/attachment.cgi?id=202797&action=review > > > Source/WebCore/ChangeLog:12 > > + because these line should ignore veritcal-align like under underline in horizontal writing mode. > > typo: "veritcal-align" Fixed. > > Source/WebCore/rendering/InlineTextBox.cpp:1011 > > + default: > > Do not move this code into the default statement. Read comment https://bugs.webkit.org/show_bug.cgi?id=102795#c63 about this. Fixed. > > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html:26 > > +left right :p The line should be alphabetic. > > This is an invalid value. It is ok that your patch defaults to alphabetic here. However, your patch should update LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position.html to check for this and other invalid values. Added test cases to getComputedStyle-text-underline-position.html. I found that I should implement CSSValueList behavior, so I implemented it. > Anything that is not either "auto", "alphabetic", "under", "under left" or "under right" is invalid. This is not correct. "left under" and "right under" are also valid value. There is the definition of || in CSS2.1[1] > A double bar (||) separates two or more options: one or more of them must occur, in any order. This says "in any order", so we can write "left under" or "right under". [1]: http://www.w3.org/TR/CSS21/about.html#property-defs

Yuki Sekiguchi

Comment 10 2013-05-27 04:29:09 PDT

(In reply to comment #7) > Attachment 202967 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/style.css', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-double.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right.html', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-left-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-horizontal-expected.txt', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical-expected.png', u'LayoutTests/platform/mac/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-right-left-vertical-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h']" exit_code: 1 > Source/WebCore/rendering/InlineTextBox.cpp:984: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] > Total errors found: 1 in 22 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. The outer "if" dose not conclude with a return. This may be bug of style checker.

Lamarque V. Souza

Comment 11 2013-05-27 06:14:01 PDT

(In reply to comment #8) > Thank you for your advice. You are welcome :-) > (In reply to comment #2) > Yes, these are not valid value. > We should check invalid case. > These test cases found my bugs, so I think adding this to trunk is valuable. I agree, just that you added them to the wrong file. You should have used getComputedStyle-text-underline-position.html for checking the parser of this patch. The other unit tests are for the rendering part. Keep in mind that getComputedStyle-text-underline-position.html is text only test and the others are pixel tests. You do not need pixel tests to test the parser. (In reply to comment #9) > Added test cases to getComputedStyle-text-underline-position.html. > I found that I should implement CSSValueList behavior, so I implemented it. Good. > > Anything that is not either "auto", "alphabetic", "under", "under left" or "under right" is invalid. > > This is not correct. > "left under" and "right under" are also valid value. > There is the definition of || in CSS2.1[1] > > A double bar (||) separates two or more options: one or more of them must occur, in any order. > > This says "in any order", so we can write "left under" or "right under". > > [1]: http://www.w3.org/TR/CSS21/about.html#property-defs Ok about that then.(In reply to comment #10) > (In reply to comment #7) > > Attachment 202967 [details] [details] did not pass style-queue: > > > > Source/WebCore/rendering/InlineTextBox.cpp:984: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] > > Total errors found: 1 in 22 files > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > The outer "if" dose not conclude with a return. > This may be bug of style checker. What the error message want to say is that you should do: if (...) return <something>; if (...) return <something>; instead of if (...) return <something>; else if (...) return <something>; You just need to remove the "else"'s in your source code. They are not necessary if there is a return inside the if statement.

Lamarque V. Souza

Comment 12 2013-05-27 08:17:59 PDT

View in context: https://bugs.webkit.org/attachment.cgi?id=202967&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1389 > + return list; You should add an assert here, list must never be empty. Besides, I do not see a need to replace CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) with this function. This just makes your patch bigger. I see that you used one of my draft patch for text-underline-position to implement this function but since WebKit already uses CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) you should implement your patch using it unless there is a really good reason for not to. > Source/WebCore/css/CSSPrimitiveValueMappings.h:2468 > // FIXME: Implement support for 'under left' and 'under right' values. Remove obsolete comment. > Source/WebCore/rendering/InlineTextBox.cpp:1251 > + FloatPoint start(localOrigin.x(), localOrigin.y() + underlineOffset + (isTopSide ? -doubleOffset : doubleOffset)); We should add tests similar to LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style.html to check for this change. Maybe a text-decoration-style-underline-left.html and text-decoration-style-underline-right.html

Yuki Sekiguchi

Comment 13 2013-05-28 01:09:08 PDT

Created attachment 203027 [details] Patch

Yuki Sekiguchi

Comment 14 2013-05-28 01:40:31 PDT

(In reply to comment #11) > I agree, just that you added them to the wrong file. You should have used getComputedStyle-text-underline-position.html for checking the parser of this patch. The other unit tests are for the rendering part. Keep in mind that getComputedStyle-text-underline-position.html is text only test and the others are pixel tests. You do not need pixel tests to test the parser. OK. I removed rendering part of invalid value test. > > > Attachment 202967 [details] [details] [details] did not pass style-queue: > > > > > > Source/WebCore/rendering/InlineTextBox.cpp:984: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] > > > Total errors found: 1 in 22 files > > > > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > > > The outer "if" dose not conclude with a return. > > This may be bug of style checker. > > What the error message want to say is that you should do: > > if (...) > return <something>; > if (...) > return <something>; > > instead of > > if (...) > return <something>; > else if (...) > return <something>; > > You just need to remove the "else"'s in your source code. They are not necessary if there is a return inside the if statement. I understand it. Thank you. Fixed. (In reply to comment #12) > View in context: https://bugs.webkit.org/attachment.cgi?id=202967&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1389 > > + return list; > > You should add an assert here, list must never be empty. I don't think so. I think your draft patch needs assert because it is not clear whether list is empty or not. However, my implementation is copied from renderTextDecorationFlagsToCSSValue(). Since "if" at 2nd line above the return line assures that list must not be empty, and there is no assert in renderTextDecorationFlagsToCSSValue(). I think the following assert is too duplicate. > if (condition) > return foo; > ASSERT(!condition); > return bar; > Besides, I do not see a need to replace CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) with this function. This just makes your patch bigger. I see that you used one of my draft patch for text-underline-position to implement this function but since WebKit already uses CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) you should implement your patch using it unless there is a really good reason for not to. Hmm. How to use CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e)? Should we renderTextUnderlinePositionFlagsToCSSValue() like the following? > for (int flag = 1; flag <= TextUnderlinePositionRight; flag <<= 1) { > if (style->textUnderlinePosition() & flag) > list->append(cssValuePool().createValue(flag)) > } or At first, we change CSSPrimitiveValue(TextUnderlinePosition e) to read flag. After that we change renderTextUnderlinePositionFlagsToCSSValue() like the following: > TextUnderlinePosition position = style->textUnderlinePosition(); > while (position) { > RefPtr<CSSPrimitiveValue> newValue = cssValuePool().createValue(position); > TextUnderlinePosition usedPosition = newValue; > position &= ~usedPosition; > list->append(newValue.release()); > } I think neither of them are clear code. Reducing diff is good, but writing clear code is also good. > > Source/WebCore/css/CSSPrimitiveValueMappings.h:2468 > > // FIXME: Implement support for 'under left' and 'under right' values. > > Remove obsolete comment. Removed. > > Source/WebCore/rendering/InlineTextBox.cpp:1251 > > + FloatPoint start(localOrigin.x(), localOrigin.y() + underlineOffset + (isTopSide ? -doubleOffset : doubleOffset)); > > We should add tests similar to LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style.html to check for this change. Maybe a text-decoration-style-underline-left.html and text-decoration-style-underline-right.html text-underline-position-double.html will cover this change. I changed this to reftest.

Lamarque V. Souza

Comment 15 2013-05-28 18:05:09 PDT

(In reply to comment #14) > (In reply to comment #12) > > View in context: https://bugs.webkit.org/attachment.cgi?id=202967&action=review > > > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1389 > > > + return list; > > > > You should add an assert here, list must never be empty. > > I don't think so. > I think your draft patch needs assert because it is not clear whether list is empty or not. > However, my implementation is copied from renderTextDecorationFlagsToCSSValue(). > Since "if" at 2nd line above the return line assures that list must not be empty, and there is no assert in renderTextDecorationFlagsToCSSValue(). "none" is a valid value for TextDecoration (http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-property), that is why defaulting to CSSValueNone when the list is empty is Ok there and no assert is needed there. However, "none" is not a valid value for text-underline-position, so the list must not be empty, an assert is needed and we should not default to CSSValueNone. > I think the following assert is too duplicate. > > if (condition) > > return foo; > > ASSERT(!condition); > > return bar; asserts are used to detect bugs not to make code more readable. Besides, they are not compiled if debugging is disabled, so they do not cause any runtime overhead in release mode. In our case the code above should be something like: if (condition) list->append(...); ASSERT(!list->isEmpty()) return list; See? No return inside the if statement. > > Besides, I do not see a need to replace CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) with this function. This just makes your patch bigger. I see that you used one of my draft patch for text-underline-position to implement this function but since WebKit already uses CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) you should implement your patch using it unless there is a really good reason for not to. > > Hmm. How to use CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e)? Ok, I tested it here and trying to use CSSPrimitiveValue::CSSPrimitiveValue() for a list is a big pain. I am ok with using renderTextUnderlinePositionFlagsToCSSValue() instead of CSSPrimitiveValue::CSSPrimitiveValue(). > > > Source/WebCore/rendering/InlineTextBox.cpp:1251 > > > + FloatPoint start(localOrigin.x(), localOrigin.y() + underlineOffset + (isTopSide ? -doubleOffset : doubleOffset)); > > > > We should add tests similar to LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style.html to check for this change. Maybe a text-decoration-style-underline-left.html and text-decoration-style-underline-right.html > > text-underline-position-double.html will cover this change. > I changed this to reftest. Ok, that's better than using pixel test :-)

Yuki Sekiguchi

Comment 16 2013-06-27 23:23:35 PDT

Created attachment 205669 [details] Patch

Yuki Sekiguchi

Comment 17 2013-06-27 23:30:25 PDT

(In reply to comment #15) > > I don't think so. > > I think your draft patch needs assert because it is not clear whether list is empty or not. > > However, my implementation is copied from renderTextDecorationFlagsToCSSValue(). > > Since "if" at 2nd line above the return line assures that list must not be empty, and there is no assert in renderTextDecorationFlagsToCSSValue(). > > "none" is a valid value for TextDecoration (http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-property), that is why defaulting to CSSValueNone when the list is empty is Ok there and no assert is needed there. However, "none" is not a valid value for text-underline-position, so the list must not be empty, an assert is needed and we should not default to CSSValueNone. Changed to use "auto", and changed definition of TextUnderlinePosition constants. It make underline-position's code more like text-decoration's.

yisibl

Comment 18 2015-12-14 23:28:56 PST

Blink support.

Brady Eidson

Comment 19 2016-05-24 22:01:33 PDT

Comment on attachment 205669 [details] Patch Assuming that patches for review since 2013 are stale, r-

Tim Nguyen (:ntim)

Comment 20 2024-07-22 23:47:53 PDT

*** This bug has been marked as a duplicate of bug 245559 ***

Note You need to log in before you can comment on or make changes to this bug.