fix text selection and cursor position in lines with rtl text #13194

Merged
merged 7 commits into from Dec 15, 2016

Projects

None yet

7 participants

@farnabaz
Contributor

assume a line contain both english text and persian (rtl) text, like this

Sample Text متن نمونه sample text

atom always look at first rect size of Text Element and calculate position, this cause cursor to place in wrong position
in our example Range.getClientRects() return three rects, look at this pen

http://codepen.io/farnabaz/pen/rWxrGW

before:
before

after:
after

@farnabaz farnabaz fix text selection and cursor position in lines with rtl text
assume a line contain both english text and persian (rtl) text, like this
```
Sample Text متن نمونه sample text
```
atom always look at first rect size of Text Element and calculate position, this cause cursor to  place in wrong position
in our example `Range.getClientRects()` return three rects, look at this pen

http://codepen.io/farnabaz/pen/rWxrGW
a0529cc
@50Wliu
Member
50Wliu commented Nov 11, 2016

Hi @farnabaz, can you please add some specs for this?

/cc @as-cii

@farnabaz
Contributor

atom has problem with selection and cursor position in lines contain rtl-language characters, as you see in first picture when a line has persian characters, cursor act strange after this chars and do not point right place, everything works fine before persian chars but after them cursor position is wrong and selection does not highlight
you can test this bug with this text, copy this text and try to select word World

Hello سلام دنیا World

this is because atom try to locate cursor column using Range.getClientRects() in LinesYardstick class.
in src/lines-yardstick.coffee: 129 text size calculates by position of first rect of its range, this works fine with ltr texts, because they has only one rect in their ranges, but a line with both right-to-left and left-to-right chars has more than one rect, I update this pen so you can see what i'm talking about rect
http://codepen.io/farnabaz/pen/rWxrGW

@50Wliu
Member
50Wliu commented Nov 11, 2016 edited

Sorry, what I meant was adding a test to make sure that this behavior doesn't regress. They go in the spec folder and you can find more information about them at http://flight-manual.atom.io/hacking-atom/sections/writing-specs/.

farnabaz added some commits Nov 12, 2016
@farnabaz farnabaz add test: cursor position in bidirectional lines
bd1919b
@farnabaz farnabaz add test: remove inconsistent indentations
4d88a97
@farnabaz
Contributor

my bad, sorry for misunderstanding
I add a test in lines-yardstick-spec

@50Wliu
Member
50Wliu commented Nov 12, 2016

Thanks! Can I also ask that you use two-space indentation?

@farnabaz
Contributor
farnabaz commented Nov 12, 2016 edited

I used two-space indentation in all files

@50Wliu
Member
50Wliu commented Nov 12, 2016 edited

/cc @as-cii

@as-cii
Member
as-cii commented Nov 17, 2016

Thanks for the pull request, @farnabaz! The reason why we used getClientRects is explained in 244f117: when a range has zero-width, getBoundingClientRect will return a wrong result, and that's why we decided to use getClientRects instead, falling back to getBoundingClientRect only in case the client rect list is empty.

I wonder if we might still be able to use getClientRects by inverting the logic to something like the following:

this.rangeForMeasurement.setStart(textNode, startIndex)
this.rangeForMeasurement.setEnd(textNode, endIndex)
const boundingClientRect = this.rangeForMeasurement.getBoundingClientRect()
if (boundingClientRect.width > 0) {
  return boundingClientRect
} else {
  return this.rangeForMeasurement.getClientRects()[0]
}

@nathansobo: thoughts? 💭

@50Wliu
Member
50Wliu commented Nov 17, 2016

Do we know if getBoundingClientRect() still returns incorrect results, or has that been fixed since April?

@farnabaz
Contributor
farnabaz commented Nov 23, 2016 edited

@as-cii As Nathan mentioned in that commit, sometimes getClientRects() return empty list, I think it may better to change your solution to something like this to avoid empty list and reach our goal

this.rangeForMeasurement.setStart(textNode, startIndex)
this.rangeForMeasurement.setEnd(textNode, endIndex)
const clientRects = this.rangeForMeasurement.getClientRects()
if (clientRects.length == 1) {
  return clientRects[0]
} else {
  return this.rangeForMeasurement.getBoundingClientRect()
}
@farnabaz farnabaz Handle ranges with multiple rect in LineYardstick
95edf2f
@nathansobo
Contributor

@ungb this looks like it could be helpful. Could you test it when you get a chance?

@ungb
ungb commented Dec 13, 2016 edited

@nathansobo I was able to test this and see that the behavior is better than before for the given scenario, I also went through and spot check a few cases and didn't see any issues with find-and-replace or the overlay for snippets

All are tested with atom built from this PR:
After change(before is describe above):
changes

NOTE: the one thing I noticed is when I want to move between the persian (rtl) text, I have to click left a lot to go to the english text, and once to go right of it, but can never go in the middle

Notice the row and column count as I click right and left and where the cursor is.
cursorcount

Overlay(looks good):
overlay

Find and Replace(looks good):
findandreplace

I'm not sure why the build is failing on x64 bit windows, but looks related to the new test you added @farnabaz
https://ci.appveyor.com/project/Atom/atom/build/4490/job/8xa7kuw9g618p8my#L484. It passed on x86 and on travisci.

@ungb
ungb commented Dec 14, 2016

@nathansobo @as-cii I was able to also test the folding behavior.

I didn't see any issues with it
foldafter

spec/lines-yardstick-spec.coffee
+ expect(linesYardstick.pixelPositionForScreenPosition(Point(0, 58))).toEqual({left: 487, top: 0})
+ expect(linesYardstick.pixelPositionForScreenPosition(Point(0, Infinity))).toEqual({left: 873.625, top: 0})
+
+ describe "::screenPositionForPixelPosition(pixelPosition)", ->
@nathansobo
nathansobo Dec 14, 2016 edited Contributor

I noticed this describe block was already indented incorrectly, so I'm just going to fix it here.

@nathansobo nathansobo Merge branch 'master' into farnabaz-master
feb60ca
@nathansobo
Contributor

Made some adjustments to the tests and merged master. Waiting on a green build.

@farnabaz
Contributor

@ungb it seems monospace font size is different on Windows x64, this picture shows same line in Mac and Windows x64 (both lines has font-size:14px), font size on Windows is a little smaller than other

screen shot 2016-12-15 at 2 35 11 am

@nathansobo nathansobo Disable measurement tests for RTL text on Windows
@damieng This is unlike the other tests in this file in that none of the
assertions pass. Maybe you could take a look at some point to enable at
least some coverage on Windows for this?
b087ab2
@lee-dohm
Member

@farnabaz Have you tested the same lines in Chrome on Mac and Windows x64 to ensure that it isn't a difference in the Chrome font rendering on the two platforms?

@nathansobo nathansobo merged commit f9dfabf into atom:master Dec 15, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nathansobo
Contributor

@farnabaz Thanks for taking this on! It will be great to handle RTL languages a bit better, though it seems like we still have a bit of room for improvement based on @ungb's testing. Still, this is way better than it was.

@farnabaz
Contributor

@lee-dohm On mac Chrome act exactly like Atom, but on Windows x64 line is different
white line shows same line in Chrome 55 on Windows x64
06f04416-c272-11e6-84de-bf3358b064b4

Here is my test page: http://codepen.io/farnabaz/pen/dOQEGE

@farnabaz
Contributor

@nathansobo It's a bit difficult to fix cursor position and selection in RTL languages, one of things that need to be fixed is screenPositionForPixelPosition function in LinesYardstick. I'll try to explain how and why this function return wrong value in RTL languages.
Should I need to create an Issue for this? or simply start to change on my fork and then explain it on Pull Request?

@nathansobo
Contributor

@farnabaz Explaining it in a pull request would be great. And yeah, the better you can help us understand what you're learning about this problem the better. Thanks so much for your interest in this!

@mohamadrezax
mohamadrezax commented Jan 11, 2017 edited

in new beta version 1.14.0 you released a fix for this issue but the problem still exsists. May the selection result being different but it is still incorrect !!
hope this issue will be fixed finally
tnx atom team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment