860248 - Defect - Tapping on content while selection is active clears the selection but leaves grippers behind

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Jim Mathies [:jimm]]

STR:

1) press-hold on some text in content (not in an input) to select it
2) expand out the markers a bit
3) tap once on the selected text

result: selection is cleared but the monocles are left behind

Not sure what the correct behavior is. In the past we've prevented clicks from clearing selection. But maybe this is more desired behavior. If we keep this we need to figure out a way to turn selection monocles off when the frame modifies selected content underneath us. 

What we might do is pull bug 851521 up and work on it. We could add a mode in the frame (via selection controller maybe?) that disables default click behavior. Then the front end could control what a tap on selection does.

MarcoM, we need clarification here on what the behavior should be. From there we can figure out how best to solve this.

Tentatively marking this as dependent on bug 851521.

Comment 1

[Marco Mucci [:MarcoM]]

Jim's point estimate=5

Comment 2

[Marco Mucci [:MarcoM]]

Hi Asa, please see Jim's question in Comment #0 about clarification on what the correct behavior should be.

Comment 3

[Asa Dotzler [:asa]]

I'd like Yuan's input here. Also, what do IE and Chrome do?

Comment 4

[Jim Mathies [:jimm]]

(In reply to Asa Dotzler [:asa] from comment #3)
> I'd like Yuan's input here. Also, what do IE and Chrome do?

IE is a little different. A single tap on any selectable content will invoke selection. Then another single tap brings up the context menu. Clicking off any selection clears the selection.

We rely on press-hold for context menus, which is akin to right-clicking on desktop. What we do when the users single or double taps on selection is open for discussion. Tapping off selection will clear like in IE.

Comment 5

[Allison Naaktgeboren :ally]

Asa asked for yuan's input in comment 3, let's get that. :)

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Just noting that this behavior occurs even if you tap outside the selection.

1) Double-tap to select a word
2) Tap anywhere in content
3) Note that the grippers have not gone away, but selection is cleared

Once you get into this state, the grippers don't act right:
  - Double-tapping to select another word will not cause the grippers' positions to update, though it will cause the word to become selected
  - Attempting to move the grippers with touch input will cause them to jump to wherever you tapped
  - Any mouse or keyboard input will cause the grippers to disappear

Comment 7

[Jim Mathies [:jimm]]

Attachment: fix plus tests

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 761650 [details] [diff] [review]
fix plus tests

More touch up work on the click handler code when selection is active. This correctly clears content selection when you tap anywhere in content.

Comment 9

[Allison Naaktgeboren :ally]

Comment on attachment 761650 [details] [diff] [review]
fix plus tests

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

Looks mostly good. One nit & two caveats. The first caveat is the test, the second is that I don't see the feedback that Asa requests from Yuan on the bug itself. I assume since you sent it to review that you've worked it out offline.

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +831,5 @@
>        this.closeEditSession(false);
>        return;
>      }
>  
> +    if (this._hitTestSelection(aEvent) && this._targetIsEditable) {

I'm pleased to see a patch removing more code than it is adding. :)

@@ +850,4 @@
>      }
>  
> +    // Close when we get a single tap in content.
> +    this.closeEditSession(false);

nit: humor me & switch this to an if/else rather than an if block with an early return

::: browser/metro/base/tests/mochitest/browser_selection_basic.js
@@ -311,2 @@
>  gTests.push({
> -  desc: "double-tap copy text in content",

So, the test changes look sane, but I don't have your experience with the test framework. If you are confident in them, go ahead; if not you will need to seek a second reviewer.

Comment 10

[Allison Naaktgeboren :ally]

We discussed the desired behavior in #windev, the behavior of the patch is what yuan has requested.

Comment 12

[Jim Mathies [:jimm]]

> @@ +850,4 @@
> >      }
> >  
> > +    // Close when we get a single tap in content.
> > +    this.closeEditSession(false);
> 
> nit: humor me & switch this to an if/else rather than an if block with an
> early return

Hmmmm. :) I disagree with using this code pattern in our code, so I'm not sure I can. Although if this is common in our front end code I would switch to it. Will try to find the answer.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #12)
> > @@ +850,4 @@
> > >      }
> > >  
> > > +    // Close when we get a single tap in content.
> > > +    this.closeEditSession(false);
> > 
> > nit: humor me & switch this to an if/else rather than an if block with an
> > early return
> 
> Hmmmm. :) I disagree with using this code pattern in our code, so I'm not
> sure I can. Although if this is common in our front end code I would switch
> to it. Will try to find the answer.

Our coding standard doesn't mention this explicitly, but it definitely leans toward early return in most cases - 

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

Comment 14

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fcf27567179

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fcf27567179

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #9 testing without any issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-07-10-03-02-05-mozilla-central/

- Went through the original test case from comment 0 without any issues (grippers are being cleared)
- Went through the problems reported in comment 6 without any issues (selected several words, selected more text after selecting words)
- Selected some text and then created a tab using "+" at the top and the overlay on the right side, went back to the website and ensured the text was still selected (monocle grippers where not visible but this matches Firefox Desktop behavior)
- Ensured that both monocle grippers disappeared when scrolling through the webpage
- Ensured that mouse selections still worked in combination with touch selection without any issues (going between them)
- Ensured that all of the above test cases worked in "Filled" view without issues

Comment 17

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #10 testing without any issues. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-07-18-03-02-01-mozilla-central/

- Went through the original test case from comment 0 without any issues (grippers are being cleared)
- Went through all the test cases added in comment 16 without any issues

Comment 18

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130806104538
Built from http://hg.mozilla.org/mozilla-central/rev/1e381c91885d

Didn't WFM
Tested on windows 8 using latest nightly  for iteration-11. 

STR:

1) press-hold on some text in content (not in an input) to select it
2) expand out the markers a bit
3) tap once on the selected text

result: selection is cleared but the monocles are left behind

Comment 19

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130816030205
Built from http://hg.mozilla.org/mozilla-central/rev/1ed5a88cd4d0

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.

Comment 20

[Samvedana (:Samvedana)]

Sometimes I see this issue, not on every site. See bug 904957.