text-emphasis-position CSS property doesn't recognize 'left' and 'right'

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

text-emphasis-position CSS property doesn't recognize 'left' and 'right'

[Myles C. Maxfield]

Created attachment 220580 [details]
Patch

[EFL EWS Bot]

Comment on attachment 220580 [details]
Patch

Attachment 220580 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6020766759911424

[Myles C. Maxfield]

Created attachment 220581 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/15466935>

[Build Bot]

Comment on attachment 220581 [details]
Patch

Attachment 220581 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5108068090118144

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 220585 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 220581 [details]
Patch

Attachment 220581 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6442979224977408

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 220586 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Myles C. Maxfield]

Created attachment 220589 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 220589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220589&action=review

> Source/WebCore/ChangeLog:15
> +        are specified, we should draw as if the appropriate value was

if neither "left" nor "right" *is* specified, we should draw as if the appropriate value *were*

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1469
> +    ASSERT(!((textEmphasisPosition & TextEmphasisPositionOver) && (textEmphasisPosition & TextEmphasisPositionUnder)));
> +    ASSERT(!((textEmphasisPosition & TextEmphasisPositionLeft) && (textEmphasisPosition & TextEmphasisPositionRight)));

Shame the types can't enforce this for us.

> Source/WebCore/rendering/InlineFlowBox.cpp:872
> +        if (emphasisMarkIsAbove == (!lineStyle.isFlippedLinesWritingMode()))

No need for parens around !lineStyle.isFlippedLinesWritingMode()

> Source/WebCore/rendering/InlineTextBox.cpp:475
> +    if (!(emphasisPosition & TextEmphasisPositionLeft) && !(emphasisPosition & TextEmphasisPositionRight))

An inline function would make this condition easier to understand.

> Source/WebCore/rendering/InlineTextBox.h:92
> +    bool emphasisMarkIsAbove(const RenderStyle&, bool& above) const;

Very confusing to have two bools returned. What do they each mean?

> Source/WebCore/rendering/style/RenderStyleConstants.h:508
> +enum TextEmphasisPositionItems {

TextEmphasisPositions?

[Myles C. Maxfield]

Comment on attachment 220589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220589&action=review

>> Source/WebCore/ChangeLog:15
>> +        are specified, we should draw as if the appropriate value was
> 
> if neither "left" nor "right" *is* specified, we should draw as if the appropriate value *were*

Done.

>> Source/WebCore/rendering/InlineFlowBox.cpp:872
>> +        if (emphasisMarkIsAbove == (!lineStyle.isFlippedLinesWritingMode()))
> 
> No need for parens around !lineStyle.isFlippedLinesWritingMode()

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:475
>> +    if (!(emphasisPosition & TextEmphasisPositionLeft) && !(emphasisPosition & TextEmphasisPositionRight))
> 
> An inline function would make this condition easier to understand.

Done.

>> Source/WebCore/rendering/InlineTextBox.h:92
>> +    bool emphasisMarkIsAbove(const RenderStyle&, bool& above) const;
> 
> Very confusing to have two bools returned. What do they each mean?

Done.

>> Source/WebCore/rendering/style/RenderStyleConstants.h:508
>> +enum TextEmphasisPositionItems {
> 
> TextEmphasisPositions?

Done.