Closed Bug 956697 Opened 6 years ago Closed 2 years ago

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

Categories

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

28 Branch
x86
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: karsten.schilder, Unassigned)

References

Details

(Whiteboard: p=2 )

Attachments

(3 files, 1 obsolete file)

Attached image 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.
OS: All → Windows 8.1
Hardware: All → x86
Attached image IE.png
Added screenshot how it looks in Internet Explorer
Whiteboard: [triage]
Whiteboard: [triage] → [triage] [defect] p=0
Status: UNCONFIRMED → NEW
Component: General → Input
Ever confirmed: true
Summary: Touch drag points missing at text marking by finger touch → Selection monocles do not appear when selecting text with double-tap
Whiteboard: [triage] [defect] p=0 → [selection] [triage] [defect] p=0
Blocks: 957244
str:

1) double tap on text in content

result: text selected but monocles do not display.

needs a test too.
Whiteboard: [selection] [triage] [defect] p=0 → [defect] p=2
Hey Jim, should this defect be marked as [beta28] or [release28]
Flags: needinfo?(jmathies)
Blocks: metrobacklog
No longer blocks: metrov1backlog
Flags: needinfo?(jmathies)
Priority: -- → P1
Target Milestone: --- → Firefox 30
Whiteboard: [defect] p=2 → p=2 r=ff30
Target Milestone: Firefox 30 → ---
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
QA Contact: kamiljoz
Whiteboard: p=2 r=ff30 → p=2 s=it-30c-29a-28b.2 r=ff30
Priority: P1 → P2
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.
(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.
Ah! That's why I can't observe DoubleTap messages in input.js (?)
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?
(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
(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.
(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.
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.
Attached patch wip.diff (obsolete) — Splinter Review
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/azasypkin@mozilla.com-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?
Attachment #8379803 - Flags: feedback?(kamiljoz)
Attachment #8379803 - Flags: feedback?(jmathies)
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?
Attachment #8379803 - Flags: feedback?(jmathies) → feedback+
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.
Attachment #8379803 - Flags: feedback?(kamiljoz) → feedback+
Attached patch wip v2.diffSplinter Review
(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.
Attachment #8379803 - Attachment is obsolete: true
(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.
(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.
(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.
Status: ASSIGNED → NEW
Whiteboard: p=2 s=it-30c-29a-28b.2 r=ff30 → p=2
Summary: Selection monocles do not appear when selecting text with double-tap → Selection monocles do not appear when selecting text with double or single tap
Assignee: azasypkin → nobody
Mass close of bugs in obsolete product https://bugzilla.mozilla.org/show_bug.cgi?id=1350354
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.