Bug 158602 - A composition underline is placed to wrong position in RTL
Summary: A composition underline is placed to wrong position in RTL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-09 20:10 PDT by Fujii Hironori
Modified: 2016-06-20 16:23 PDT (History)
11 users (show)

See Also:


Attachments
test content (152 bytes, text/html)
2016-06-09 20:11 PDT, Fujii Hironori
no flags Details
screen shot (7.21 KB, image/png)
2016-06-09 20:14 PDT, Fujii Hironori
no flags Details
Patch (3.90 KB, patch)
2016-06-10 01:34 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (767.33 KB, application/zip)
2016-06-10 02:31 PDT, Build Bot
no flags Details
Patch (4.20 KB, patch)
2016-06-10 03:18 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (2.51 KB, patch)
2016-06-13 01:40 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (6.10 KB, patch)
2016-06-19 23:22 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2016-06-09 20:10:53 PDT
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.
Comment 1 Fujii Hironori 2016-06-09 20:11:52 PDT
Created attachment 280980 [details]
test content
Comment 2 Fujii Hironori 2016-06-09 20:14:03 PDT
Created attachment 280981 [details]
screen shot

I'm just compositing a Japanese text "ほげ".
The underline was placed at the mirrored position.
Comment 3 Fujii Hironori 2016-06-10 01:34:56 PDT
Created attachment 281002 [details]
Patch
Comment 4 Ryosuke Niwa 2016-06-10 01:44:44 PDT
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 :.
Comment 5 Build Bot 2016-06-10 02:31:08 PDT
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
Comment 6 Build Bot 2016-06-10 02:31:13 PDT
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
Comment 7 Fujii Hironori 2016-06-10 03:16:53 PDT
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.
Comment 8 Fujii Hironori 2016-06-10 03:18:52 PDT
Created attachment 281005 [details]
Patch
Comment 9 zalan 2016-06-10 07:00:12 PDT
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?
Comment 10 Fujii Hironori 2016-06-12 22:48:47 PDT
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;
Comment 11 Fujii Hironori 2016-06-13 01:40:06 PDT
Created attachment 281162 [details]
WIP patch

How about this utility function?
Does this patch look better?
Comment 12 Fujii Hironori 2016-06-19 23:22:19 PDT
Created attachment 281633 [details]
Patch

Zalan. I created a new patch. Could you review again?
Comment 13 WebKit Commit Bot 2016-06-20 16:23:24 PDT
Comment on attachment 281633 [details]
Patch

Clearing flags on attachment: 281633

Committed r202250: <http://trac.webkit.org/changeset/202250>
Comment 14 WebKit Commit Bot 2016-06-20 16:23:30 PDT
All reviewed patches have been landed.  Closing bug.