Bug 158602 – A composition underline is placed to wrong position in RTL

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Fujii Hironori]

A composition underline is placed to wrong positoin in RTL

InlineTextBox::paintCompositionUnderline does not take RTL into account.
The position of composition underline should be inverted.

[Fujii Hironori]

Created attachment 280980 [details]
test content

[Fujii Hironori]

Created attachment 280981 [details]
screen shot

I'm just compositing a Japanese text "ほげ".
The underline was placed at the mirrored position.

[Fujii Hironori]

Created attachment 281002 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 281002 [details]
Patch

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

> LayoutTests/editing/input/composition-underline-rtl-expected.html:1
> +<style>

Missing DOCTYPE (triggers quirks mode).

> LayoutTests/editing/input/composition-underline-rtl.html:1
> +<style>

Ditto.

> LayoutTests/editing/input/composition-underline-rtl.html:5
> +    direction:rtl;

Nit: Space after :.

[Build Bot]

Comment on attachment 281002 [details]
Patch

Attachment 281002 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1477594

New failing tests:
editing/input/composition-underline-rtl.html

[Build Bot]

Created attachment 281004 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Fujii Hironori]

Thanks for your review. I'll fix all points.

(In reply to comment #6)
> The attached test failures were seen while running run-webkit-tests on the
> ios-sim-ews.
> Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

This seems eventSender.keyDown("leftArrow") fails to move the caret on iOS.
I'll try EWS again after rewriting with window.getSelection and document.createRange.

[Fujii Hironori]

Created attachment 281005 [details]
Patch

[zalan]

Comment on attachment 281005 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:938
> +        if (!isLeftToRightDirection())
> +            start = m_logicalWidth - width - start;

Do you mind making a helper function for this as now this logic is both at InlineTextBox::paintDecoration and here? (and maybe it's even missing from some of the other painting methods)

There is this 'start += 1;' a few lines below, does it work fine with RTL?

[Fujii Hironori]

zalan,

Thanks for your review.

(In reply to comment #9)
> > Source/WebCore/rendering/InlineTextBox.cpp:938
> > +        if (!isLeftToRightDirection())
> > +            start = m_logicalWidth - width - start;
> 
> Do you mind making a helper function for this as now this logic is both at
> InlineTextBox::paintDecoration and here? (and maybe it's even missing from
> some of the other painting methods)

It has a similar logic.
But, it seems difficult to simplify these codes.
Could you tell me a code you think of.

> There is this 'start += 1;' a few lines below, does it work fine with RTL?

No problem.
Because this code shortens the underline by 1px each from both sides.

> // We need to have some space between underlines of subsequent clauses, because some input methods do not use different underline styles for those.
> // We make each line shorter, which has a harmless side effect of shortening the first and last clauses, too.
> start += 1;
> width -= 2;

[Fujii Hironori]

Created attachment 281162 [details]
WIP patch

How about this utility function?
Does this patch look better?

[Fujii Hironori]

Created attachment 281633 [details]
Patch

Zalan. I created a new patch. Could you review again?

[WebKit Commit Bot]

Comment on attachment 281633 [details]
Patch

Clearing flags on attachment: 281633

Committed r202250: <http://trac.webkit.org/changeset/202250>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.