Bug 126611

Summary: text-emphasis-position CSS property doesn't recognize 'left' and 'right'
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, darin, eflews.bot, enrica, esprehn+autocc, glenn, gyuyoung.kim, jonlee, kondapallykalyan, macpherson, menard, mitz, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch simon.fraser: review+

Myles C. Maxfield
Reported 2014-01-07 18:21:38 PST
text-emphasis-position CSS property doesn't recognize 'left' and 'right'
Attachments
Patch (36.61 KB, patch)
2014-01-07 18:36 PST, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Patch (36.61 KB, patch)
2014-01-07 18:54 PST, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1 2014-01-07 18:36:30 PST
Created attachment 220580 [details] Patch
EFL EWS Bot
Comment 2 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
Myles C. Maxfield
Comment 3 2014-01-07 18:54:44 PST
Created attachment 220581 [details] Patch
Myles C. Maxfield
Comment 4 2014-01-07 18:58:34 PST
<rdar://problem/15466935>
Build Bot
Comment 5 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
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Build Bot
Comment 8 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
Myles C. Maxfield
Comment 9 2014-01-07 20:54:16 PST
Created attachment 220589 [details] Patch
Simon Fraser (smfr)
Comment 10 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?
Myles C. Maxfield
Comment 11 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.
Note You need to log in before you can comment on or make changes to this bug.