Bug 126611 - text-emphasis-position CSS property doesn't recognize 'left' and 'right'
Summary: text-emphasis-position CSS property doesn't recognize 'left' and 'right'
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-07 18:21 PST by Myles C. Maxfield
Modified: 2014-01-16 10:44 PST (History)
15 users (show)

See Also:


Attachments
Patch (36.61 KB, patch)
2014-01-07 18:36 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (36.61 KB, patch)
2014-01-07 18:54 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (470.12 KB, application/zip)
2014-01-07 20:06 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (504.38 KB, application/zip)
2014-01-07 20:33 PST, Build Bot
no flags Details
Patch (38.69 KB, patch)
2014-01-07 20:54 PST, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-01-07 18:21:38 PST
text-emphasis-position CSS property doesn't recognize 'left' and 'right'
Comment 1 Myles C. Maxfield 2014-01-07 18:36:30 PST
Created attachment 220580 [details]
Patch
Comment 2 EFL EWS Bot 2014-01-07 18:44:21 PST
Comment on attachment 220580 [details]
Patch

Attachment 220580 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6020766759911424
Comment 3 Myles C. Maxfield 2014-01-07 18:54:44 PST
Created attachment 220581 [details]
Patch
Comment 4 Myles C. Maxfield 2014-01-07 18:58:34 PST
<rdar://problem/15466935>
Comment 5 Build Bot 2014-01-07 20:05:57 PST
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
Comment 6 Build Bot 2014-01-07 20:06:00 PST
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
Comment 7 Build Bot 2014-01-07 20:33:44 PST
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
Comment 8 Build Bot 2014-01-07 20:33:48 PST
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
Comment 9 Myles C. Maxfield 2014-01-07 20:54:16 PST
Created attachment 220589 [details]
Patch
Comment 10 Simon Fraser (smfr) 2014-01-15 17:08:22 PST
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?
Comment 11 Myles C. Maxfield 2014-01-15 21:31:55 PST
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.