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.
Created attachment 280980 [details] test content
Created attachment 280981 [details] screen shot I'm just compositing a Japanese text "ほげ". The underline was placed at the mirrored position.
Created attachment 281002 [details] Patch
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 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
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
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.
Created attachment 281005 [details] Patch
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?
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;
Created attachment 281162 [details] WIP patch How about this utility function? Does this patch look better?
Created attachment 281633 [details] Patch Zalan. I created a new patch. Could you review again?
Comment on attachment 281633 [details] Patch Clearing flags on attachment: 281633 Committed r202250: <http://trac.webkit.org/changeset/202250>
All reviewed patches have been landed. Closing bug.