1084293 - Selection handle displayed at an incorrect position after Select All button tapped

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Cornel Ionce [:noni] [Hubs QA]]

Attachment: text_selection_handle.png

Environment: 
Device: Asus Transformer Tab (Android 4.2.1);
Build: Nightly 36.0a1 (2014-10-16);

Steps to reproduce:
1. Load http://en.wikipedia.org/wiki/mozilla
2. Long tap on a word to select it.
3. Tab on "Select all" from the action bar

Expected result:
The entire text is selected and the selection handles are either displayed at the start and end of the text or not displayed at all.

Actual result:
A single selection handle (right one) is displayed at an incorrect position.

Notes:
Please check the attached screenshot.

Comment 1

[Mark Capella [:capella]]

Curious ... had you "Requested Desktop Site"? I don't Wikipedia displayed that way otherwise, on either my Phone or Tablet (old or new style UI).

I can't repro this ... do you see this consistently? Are you long-tapping a particular word I might try?

Comment 2

[Cornel Ionce [:noni] [Hubs QA]]

I did not select "Request Desktop Site". 
I can see this consistently, in 100% of the cases and it reproduces with any long-tapped word.

I'm also experiencing this with a Nexus 4 (Android 4.4.4) device without "Requested Desktop Site".

Comment 3

[Mike Taylor [:miketaylr]]

I can also reproduce, from a mobile wikipedia page. Will add a screenshot. 

(Oddly not from a few other random URLs I tried, time.com, some reddit.com comment thread, mozhacks blog post).

Comment 4

[Mike Taylor [:miketaylr]]

Attachment: 2014-11-10 16.16.20.png

Comment 5

[Mark Capella [:capella]]

The text selection handle positions are determined starting here: [1]

In this case, where the entire page text is selected including complex expandable/collapsible elements, we return from .getRangeAt(0).getClientRects() the following rects table as results:

   rect[ 0] [{"l":"    0","t":"    0","w":"    0","h":"    0"}]
   rect[ 1] [{"l":"    0","t":"    0","w":"    0","h":"    0"}]
   rect[ 2] [{"l":"    0","t":"    0","w":"  600","h":"  887"}]
   rect[ 3] [{"l":"  319","t":"  887","w":"    0","h":"    0"}]
   rect[ 4] [{"l":"  319","t":"  887","w":"    0","h":"    0"}]
   rect[ 5] [{"l":"  319","t":"  887","w":"    0","h":"    0"}]

And update our internal cache with anchor/focus handle positions determined as:
   anchorHandle [{"x":0,"y":0}], focusHandle [{"x":318.8500061035156,"y":887}]

Our handle positions are determined based on the first and last rect[] in that table, and we don't ignore empty rects[].

Based on "getClientRects()" [2], I wonder if we're interpreting / employing this return data properly (Microsoft TextRectangle object vs Mozilla ClientRect's)?

Either that or we're getting wrong results from core which seems icky also.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=120ddf617de1&mark=1053-1053#1050
[2] https://developer.mozilla.org/en-US/docs/Web/API/element.getClientRects#Returns


researching ...

Comment 6

[Mark Capella [:capella]]

I think the operative phrase from link [2] above is:

... Rectangles are returned even for CSS boxes that have empty border-boxes...
"The left, top, right and bottom coordinates can still be meaningful."

But in the context of our selection handle end-point determination logic, they aren't.

Filtering rect.isEmpty() results here [3] will fix this.

This is a pretty simple code change, and I think it's ok to label it a "good first bug", but you'll need average knowledge of Javascript, own an Android device, and be able to build and test the Android version of Firefox for Mobile devices. (This is easier if you already have a source repo and can build Firefox for desktop).

A skilled first-time contributor should be able to finish this in under two weeks. Others may take longer, as creating your local build and test environments the first time is a challenge of itself. See [4] for overall details, and the robocop section [5] for testing specifics.

Leave comments / questions here, or look for me in IRC as nick:capella, in #introduction and #mobile channels.


[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=260eb4adf6c6&mark=1087-1087#1084

Comment 7

[Mark Capella [:capella]]

Attahcing the below from email:

Hi Mantaroh, you can comment here in the bugzilla instead of emailing directly. I'll assign this to you for now. Create your patch, and when you think it works, attach it here and we'll have a look at it :-)


Hi mark,
I'm Mantaroh Yoshinaga.
(Software engineer in Japan)

I'm interested in Fennec's bug(#1084293).
https://bugzilla.mozilla.org/show_bug.cgi?id=1084293

I've finished build development environment, and reproduced this phenomenon on my Nexus5.

I think that we should modify end point calculate logic.
example: calculate using loop for rect array.

How do you think about my suggestion?

I want to try this bug for my first bug.

Best regard,
Mantroh Yoshinaga 〈[e-mail]〉

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Hi Mark,

I trying to fix this bug now.

My solution is using loop and determine min position and max position.
when determining, I ignore element which width or height is zero.

This phenomenon was never shown when using this solution on my debug environment.

But, I have several question about this bug.

1. start cursor didn't shown when calculated start position is (0,0).
Probably, cursor was rendered before start position.

2. end cursor didn't shown when calculated end position is max.
Probably, this is same problem no.1.

We should consider cursor size.
If start position is zero or end position is max, should add cursor size.

How do you think about this cursor size problem?

Comment 9

[Mark Capella [:capella]]

The issue of handles starting/ending "offscreen" on selectAll is a different issue, and is handled differently by various other commercial browsers. I'd like to address that in a different bug later.

For this bug, I'd just like to filter out empty rects, as mentioned in comment #6, which will at least avoid displaying the handle in a completely wrong position.

You should be able to reproduce this fairly simply (I can) by :

1) navigating to http://en.wikipedia.org/wiki/mozilla
2) long pressing the word "exclusively" in the first paragraph to select it.
3) Pressing the selectAll icon in the action bar
4) Swipe/scroll the page up to observe the selection handle to the right of the "Contents" box
   as pictured above in the attachment https://bugzilla.mozilla.org/attachment.cgi?id=8506773

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: 1084293.patch

Hi mark,

I created the patch for this bug.
and, I confirmed that fixed this bug using my patch.
So, Could you please review this patch?

Comment 11

[Mark Capella [:capella]]

Comment on attachment 8558366 [details] [diff] [review]
1084293.patch

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

This looks good in general. Can you re-generate the patch following the directions here?
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

If you format it properly, it should look something like this one. 
https://bugzilla.mozilla.org/attachment.cgi?id=8558275&action=edit

In that example, I'd used r=bnicholson on the commit message, but for yours, please use r=margaret.

When you repost the patch, instead of checking review+ in the flags section, check review? and insert :margaret as the reviewer.

After that I'll retest the patch locally again, and get you a push to the TRY server.

::: mobile/android/chrome/content/SelectionHandler.js
@@ +1170,5 @@
>        throw "Failed to update cache for invalid selection";
>      }
>  
> +    let newRects = [];
> +    for(var i=0,len=rects.length;i<len;i++) {

Please use 'let' instead of 'var'. Also, using moz spaces styling this line should be like:
   for (let i = 0, len = rects.length; i < len; i++) {

@@ +1172,5 @@
>  
> +    let newRects = [];
> +    for(var i=0,len=rects.length;i<len;i++) {
> +      if(rects[i].width > 0 && rects[i].height > 0) {
> +        newRects.push(rects[i]);

add a space between the |if (|

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: 1084293_2.patch

Hi mark,

Thank you for confirm my patch.
I modified my patch.
(Whitespace rule / patch comment and others..)

Could you please see this patch?

Comment 13

[Mark Capella [:capella]]

Comment on attachment 8558979 [details] [diff] [review]
1084293_2.patch

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

Getting close mantaroh!

   This time is looks like you posted a diff to your earlier diff. Can you re-post the entire patch please as a single, complete patch? And just flag me for ?feedback.

Comment 14

[Mark Capella [:capella]]

mantaroh: I've been so busy I completely missed something here. When you refactor your patch, can you make sure to move rects.length check after the part where you filter out empty rects?

Something like:

*** The existing first part
     let rects = this._getSelection().getRangeAt(0).getClientRects();

*** Your new code
     let newRects = [];
     for (...) {
        ...
     }

*** The existing length check, but on the newRects
     if (newRects.length == 0) {
       ...
     }
 

This way of course, if we filter out *everything*, we throw that exception.

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: 1084293_20150205.patch

Hi mark,

I'm sorry for delay reply.
I've finished to modify feedback, and move to array size check.

Could you please check it?

Comment 16

[Mark Capella [:capella]]

Comment on attachment 8559658 [details] [diff] [review]
1084293_20150205.patch

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

I just noticed the patch commit message mentions |Bug 1084239| instead of |Bug 1084293|. That's a minor change we can fix :-)

You patch does exactly what I asked for, and I thank you for it. Unfortunately, I've found an issue where it seems to create a problem that didn't exist before.

If you click here for example:
http://thehill.com/homenews/senate/231861-senate-dems-block-immigration-bill-for-third-day-in-a-row

Then switch into readermode (tap the icon that looks like a book in the addressbar)

Longpress the word "Homeland" in the first paragraph, and then tap the SelectAll icon in the actionmode when it opens, do you notice the focus handle in the wrong place?
https://www.dropbox.com/s/a9f0qiqjkt04ouc/bug1084293_regression.png?dl=0

Without this patch applied I don't see that happen, and we can't move forward here unless we can explain this and correct it.

It seems that document.selectAll actions return different-ish results in general from the getClientRects() call in that function.

Any ideas? (I'm thinking too ....)

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: SelectAll_ReaderMode.png

Hi capella,

Thank you for confirming my patch, and fix bug number.

I confirmed reader mode problem..
And I tryed remote debug(inspector).

I think reader mode contain 4 elements.
Header, Content, Message, Toolbar.

When use SelectAll, getClientRects()  returned such 4 elements.
And, Header element contained domain and title elements,
but returned element is header element when we calculate position.

So, current logic calculated start position which header element's lower left.

I think should modify calculating start position to higher left.

What do you think about this?

Comment 18

[Mark Capella [:capella]]

Comment on attachment 8561030 [details]
SelectAll_ReaderMode.png

Unfortunately, what I thought might be an easy solution doesn't work in all cases. This is my fault for not fully understanding the getClientRects() mechanism in the case of selections spanning multiple child nodes.

I'm afraid we're going to have to sit on this for awhile. The issue is low-priority, and while new approaches I've briefly looked aren't yet worth the investment of time for the expected payback, I'll let you know if anything promising comes up.

I appreciate the help you've provided so far. I wonder, are you working on anything else in the meantime? We have several current good-first-bugs I could point you to.

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Hi Capella,

Thank you for your comment.

> I appreciate the help you've provided so far. I wonder, are you working on
> anything else in the meantime? We have several current good-first-bugs I
> could point you to.
That sounds good.
I was just looking for new bugs.
If you can, Could you please tell me a bug?

Comment 20

[Mark Capella [:capella]]

The easies thing to do is to use "bugsahoy" to find good-first bugs ... for example simple / mentored Mobile bugs involving Java or javascript are here:

http://www.joshmatthews.net/bugsahoy/?mobileandroid=1&java=1&js=1&unowned=1&simple=1

Comment 21

[Mark Capella [:capella]]

Back to analysis

Comment 22

[Mantaroh Yoshinaga[:mantaroh]]

Hi capella,
I'm back to this bug..

I though that there are another solution for this bug.
This bug's problem is 2 point as follow.
1) included invisible element when calculate cursor position.
 getClientRects() included zero-size element.
 So, calculated position is wrong.
 It's fixed previous patche.

2) calculated start position is wrong.
 Current calculated most left-bottom position as start position.
 So, start position is wrong when some pages loaded.

My solution which will fixed problem 2) is calculating start position which most left-top when LTR.
(RTL is calculating start position which most right-top and end position which most left-bottom).

How do you think about this solution?

Comment 23

[Mark Capella [:capella]]

I'm not sure I follow your idea. Please feel free to submit a patch that you think helps here, and I'll take a closer look. I suspect that you'll wind up trying to special-case a solution that won't be generally applicable; ie: will cause more problems than this minor issue is worth solving.

I touched on the heart of the issue in our current implementation back in comment #5.

Depending on results returned from:
https://developer.mozilla.org/en-US/docs/Web/API/Element/getClientRects

Works for the vast majority of our cases, but the results returned from the selectAll() method don't simply iterate a line-by-line clientRect of displayed text.

See: "Originally, Microsoft intended this method to return a TextRectangle object for each line of text. However, the CSSOM working draft specifies that it returns a ClientRect for each border box. For an inline element, the two definitions are the same. But for a block element, Mozilla will return only a single rectangle."

selectAll() results in block elements being included in the selection range, not necessarily the underlying individual text elements (those that support "dir" or "direction" attributes.). This breaks our expectations that we receive TextRectangle objects, and prevents accurate handle positioning.

This is a minor UI issue, as the entire text has been correctly "selected", and most users after performing a selectAll() will do a "Share", "Copy", and etc action by tapping an ActionBar icon, versus immediately needing to grab aSelectionHandle to modify the selection.


There is a way to solve this, if one has the time and inclination, involving code that adjusts the handles dynamically to the correct UI locations *after a selectAll action* only. Subsequent code in the block we've been addressing comes in a later path and will work.

I actually spent some time tinkering with this but moved on to higher priority patches.

The bigger issue is that with FF/OS (or Boot-To-Gecko as it used to be called) mozilla implemented built-in Selection/Caret support tied more directly to Gecko. See bug 988143, where we're looking at replacing the current Android TextSelection logic you've been looking at (SelectionHandler.js) almost entirely with new code.

I don't have a project plan with an estimated completion date for that yet, but I'd hate to see you waste too much time pursuing something that may wind up obviated in short time.

Comment 24

[Mark Capella [:capella]]

Pruning some old bugs (part 2) either:
   ) obviated by new Core/Layout AccessibleCaret implementation in m-c and stable, targeted for 47.
   ) Or no longer being observable.

If you still see what you consider incorrect behaviour with the new version, please file a new bug targeted there, and attach a test-case or example link to help things :-)