956697 - Selection monocles do not appear when selecting text with double or single tap

Status: RESOLVED INCOMPLETE

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

Description

[KarstenS]

Attachment: Firefox.png

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

Doubble tap a word and try to resize the area of marked text.


Actual results:

Resizing of the marked area by finger touch is not possible as the drag points are missing to do that.

Looks like the marked area is in mouse mode instead in finger mode.


Expected results:

Dragpoints to change the marked area accessibble.

Comment 1

[KarstenS]

Attachment: IE.png

Added screenshot how it looks in Internet Explorer

Comment 2

[Jim Mathies [:jimm]]

str:

1) double tap on text in content

result: text selected but monocles do not display.

needs a test too.

Comment 3

[Marco Mucci [:MarcoM]]

Hey Jim, should this defect be marked as [beta28] or [release28]

Comment 4

[Kamil Jozwiak [:kjozwiak]]

I personally think we should fix this in the next iteration or ASAP. This affects anyone that is using a touch device like a surface pro without a mouse. You won't be able to select any text from a website like Wikipedia. This also makes it difficult for QA when attempting to test monocles/grippers.

We've lost major functionality due to this issue.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #4)
> I personally think we should fix this in the next iteration or ASAP. This
> affects anyone that is using a touch device like a surface pro without a
> mouse. You won't be able to select any text from a website like Wikipedia.
> This also makes it difficult for QA when attempting to test
> monocles/grippers.
> 
> We've lost major functionality due to this issue.

You can still initiate selection using press-hold over the text in content, which technically is the common path we've always supported. Double tap was originally captured for zoom, but we disabled that. We need to bring double-tap to select functionality back, which shouldn't be too hard to do.

Comment 6

[Mark Capella [:capella]]

Ah! That's why I can't observe DoubleTap messages in input.js (?)

Comment 7

[Oleg Zasypkin [:azasypkin]]

Btw, I see that IE on my Surface Pro selects text on single tap, and just zoom-in\out on double tap without any selection. Do we want to do both things on double tap or we can go IE way?

Comment 8

[Jim Mathies [:jimm]]

(In reply to Mark Capella [:capella] from comment #6)
> Ah! That's why I can't observe DoubleTap messages in input.js (?)

If you mean the apzc event, we don't send that. we use normal dom events, so in input.js, dblclick. We used to support selection this way, see the patch in bug 895581 where we removed it trying to get double tap to zoom work right -  
https://bugzilla.mozilla.org/attachment.cgi?id=8334136&action=diff

Comment 9

[Jim Mathies [:jimm]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #7)
> Btw, I see that IE on my Surface Pro selects text on single tap, and just
> zoom-in\out on double tap without any selection. Do we want to do both
> things on double tap or we can go IE way?

We're not going to support double tap to zoom (bug 950832) for performance reasons. Single tap to select might be interesting, I think you could trigger this by handling click events down in Content.js assuming there isn't a valid click target at the coordinates.

Comment 10

[Oleg Zasypkin [:azasypkin]]

(In reply to Jim Mathies [:jimm] from comment #9)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #7)
> > Btw, I see that IE on my Surface Pro selects text on single tap, and just
> > zoom-in\out on double tap without any selection. Do we want to do both
> > things on double tap or we can go IE way?
> 
> We're not going to support double tap to zoom (bug 950832) for performance
> reasons. Single tap to select might be interesting, I think you could
> trigger this by handling click events down in Content.js assuming there
> isn't a valid click target at the coordinates.

Yeah, I have wip patch for this, just testing it to figure out potential problems. But currently it looks fine, I personally like single-tap-to-select approach.

Comment 11

[Marco Mucci [:MarcoM]]

Hey Kamil, this is currently assigned for this iteration.

(In reply to Kamil Jozwiak [:kjozwiak] from comment #4)
> I personally think we should fix this in the next iteration or ASAP. This
> affects anyone that is using a touch device like a surface pro without a
> mouse. You won't be able to select any text from a website like Wikipedia.
> This also makes it difficult for QA when attempting to test
> monocles/grippers.
> 
> We've lost major functionality due to this issue.

Comment 12

[Oleg Zasypkin [:azasypkin]]

Attachment: wip.diff

Hey guys, I've pushed single-tap-to-select patch to TRY for you to check it out.

Note: Currently we dismiss selection once user taps anywhere including selection itself while IE doesn't dismiss selection if user taps on it. If we do the same then selection will be preserved on double tap.

TRY link: https://tbpl.mozilla.org/?tree=Try&rev=fae8d64134ed

Package will be available here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fae8d64134ed/

Code is still in unfinished state, but enough to demonstrate. I was thinking about moving "IsElementClickable" method from http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp#156 to nsIDOMWindowUtils to reuse it at both cpp and js code base and remove code duplication from our Util.js, but not sure if it's good way to go. Also I'll add a bunch of tests to cover main cases.

What do you think about it?

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8379803 [details] [diff] [review]
wip.diff

In a page of text where you are clicking on text content this works really great. 

If you're just tapping on a page, particularly white space areas, you end up triggering bug 854072 a lot. I think we would want to find a fix for that before this lands. Also, when I tap to stop apzc scroll, I trigger selection. I wonder if there's a way to prevent that?

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Comment on attachment 8379803 [details] [diff] [review]
wip.diff

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

Another thing that I've noticed is that this will make it very difficult to select text near links because of ui.touch.radius. The current build is using the same values but it's a lot less noticeable as the users need to tap/hold to retrieve the grippers. With this build, users will trigger the links as soon they try to select some text via the single tap.

We should look into this problem and come up with a solution before landing this.

Comment 15

[Oleg Zasypkin [:azasypkin]]

Attachment: wip v2.diff

(In reply to Jim Mathies [:jimm] from comment #13)
> Comment on attachment 8379803 [details] [diff] [review]
> wip.diff
> 
> In a page of text where you are clicking on text content this works really
> great. 
> 
> If you're just tapping on a page, particularly white space areas, you end up
> triggering bug 854072 a lot. I think we would want to find a fix for that
> before this lands. Also, when I tap to stop apzc scroll, I trigger
> selection. I wonder if there's a way to prevent that?

1. Yeah, bug 854072 should be resolved first, the only thing that comes in mind and can reduce number of false triggering is to activate selection only if target is text node (via caretPositionFromPoint or some different way). Application is limited but valuable for text.

2. Unfortunately apzc-transform-end is called before our click handler, but we can track apzc-transform sessions (+ small timeout) to avoid false triggering. I've added "isApzcTransformSessionWasActiveRecently" to Content.js to demonstrate.

Comment 16

[Oleg Zasypkin [:azasypkin]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #14)
> Comment on attachment 8379803 [details] [diff] [review]
> wip.diff
> 
> Review of attachment 8379803 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Another thing that I've noticed is that this will make it very difficult to
> select text near links because of ui.touch.radius. The current build is
> using the same values but it's a lot less noticeable as the users need to
> tap/hold to retrieve the grippers. With this build, users will trigger the
> links as soon they try to select some text via the single tap.
> 
> We should look into this problem and come up with a solution before landing
> this.

Yes, it's true for double and single tap, we need to discuss how to approach this. Probably we can do it in a way as it's done on some (all?) mobile devices: if clickable element became click target only due to fluffing it's kind of "conflict" for user that he probably should resolve by himself: we can zoom only this conflicting area (that doesn't allow fluffing) for user to make final choice. But here we should understand whether we have something to select or click nearby and let user choose only if it's true. That sounds like we need similar functionality as for bug 854072.

Comment 17

[Jim Mathies [:jimm]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #15)

> 2. Unfortunately apzc-transform-end is called before our click handler, but
> we can track apzc-transform sessions (+ small timeout) to avoid false
> triggering. I've added "isApzcTransformSessionWasActiveRecently" to
> Content.js to demonstrate.

Hm, I'd expect the apzc to consume these touch events completely. Lets chat with kats about this when he get on irc.

Comment 18

[Oleg Zasypkin [:azasypkin]]

(In reply to Jim Mathies [:jimm] from comment #17)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #15)
> 
> > 2. Unfortunately apzc-transform-end is called before our click handler, but
> > we can track apzc-transform sessions (+ small timeout) to avoid false
> > triggering. I've added "isApzcTransformSessionWasActiveRecently" to
> > Content.js to demonstrate.
> 
> Hm, I'd expect the apzc to consume these touch events completely. Lets chat
> with kats about this when he get on irc.

As per kats, it's expected behavior, so we need to somehow deal with it.

Comment 19

[Firefox Bug Husbandry Bot]

Mass close of bugs in obsolete product https://bugzilla.mozilla.org/show_bug.cgi?id=1350354