Closed Bug 1128187 Opened 6 years ago Closed 6 years ago

Allow SelectionHandles in mixed LTR-RTL content

Categories

(Firefox for Android :: Text Selection, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(2 files)

While testing the correction for bug 1081742, I realized that SelectionHandler will have to be made smarter following it's completion.

Currently, the selection handles take their RTL/LTR cue from the document, but pages (in and out of RM) can display a mix of both. Handles can start and end in elements with different text directions ... testcase [1]

[1] https://www.dropbox.com/s/ik1ks4mhhe0js6k/test_bug1081742.html?dl=0
Attached patch bug1128187.diffSplinter Review
The fix isn't too hard, and while exposed by bug 1081742, exists w/o it based on design of the underlying webpage.

Mixed content selection w/o fix:
https://www.dropbox.com/s/ysrd0278wmjj0x0/bug1128187_Incorrect.png?dl=0

Mixed content selection w/the fix:
https://www.dropbox.com/s/0nmd5lg0lg9odhy/bug1128187_Correct.png?dl=0
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8557517 - Flags: review?(margaret.leibovic)
Comment on attachment 8557517 [details] [diff] [review]
bug1128187.diff

Review of attachment 8557517 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, nice job with the code comments! Sorry for the delayed review, I'm getting back on top of things now!

::: mobile/android/chrome/content/SelectionHandler.js
@@ +1202,5 @@
> +   * Return true if text associated with a node is RTL.
> +   */
> +  _isNodeRTL: function(node) {
> +    // Find containing node that supports .direction attribute (needed
> +    // when target node is #text for example).

Nice comment.
Attachment #8557517 - Flags: review?(margaret.leibovic) → review+
Drats ... dropped a piece for the "TextSelection:PositionHandles" generated for Caret Handle ... not used, but needed to avoid "org.json.JSONException: No value for rtl" in TextSelection.java
https://hg.mozilla.org/mozilla-central/rev/0a92f400101e
https://hg.mozilla.org/mozilla-central/rev/946346db74bb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.