1292904 - Can't double click to select text in text fields when AccessibleCaret is enabled

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Matt Cosentino]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160805030444

Steps to reproduce:

Double click in text field to select a word.


Actual results:

An empty selection is created.


Expected results:

The word should be selected.

Comment 1

[Tooru Fujisawa [:arai]]

1. Where does the issue happen?  which page (URL) and which text field?
2. does the issue happen with safe mode and clean profile?

Comment 2

[Matt Cosentino]

Happens everywhere, in every text field, including this one. Yes it happens in safe mode. My guess is it is related to these new text selection handles.

Comment 3

[Tooru Fujisawa [:arai]]

(In reply to Matt Cosentino from comment #2)
> Happens everywhere, in every text field, including this one.
I cannot reproduce this on Firefox build 20160805030444 (both 32bit and 64bit) on Windows 10, with clean profile

> Yes it happens in safe mode.
How about clean profile?

> My guess is it is related to these new text selection handles.
which bug/change do you refer with "these new text selection handles" ?  do you have bug number?

Comment 4

[Matt Cosentino]

It's the new "accessible caret", which is supposed to be for only mobile devices but it seems to be enabled on Windows 10 with a touch screen. I turned off "layout.accessiblecaret.enabled_on_touch" and I can select text normally again.

Comment 5

[Tooru Fujisawa [:arai]]

thanks
confirmed the issue by setting layout.accessiblecaret.enabled to true.

layout.accessiblecaret.enabled_on_touch was introduced by bug 1195722

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

AccessibleCaret is designed for mobile device on a touch screen, so it doesn't consider mouse usage.  The touch area of the AccessibleCaret also covers the blinking caret, so any click events on the AccessibleCaret itself is ignored. 

I think we might need a way to distinguish whether a "Windows with a touch screen" is a desktop or a tablet, and enable AccessibleCaret only on the tablet (mobile). kats, what do you think?

Comment 7

[Matt Cosentino]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #6)
> I think we might need a way to distinguish whether a "Windows with a touch
> screen" is a desktop or a tablet, and enable AccessibleCaret only on the
> tablet (mobile). kats, what do you think?

Could this help?

https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.viewmanagement.userinteractionmode

Comment 8

[Botond Ballo [:botond]]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #6)
> I think we might need a way to distinguish whether a "Windows with a touch
> screen" is a desktop or a tablet, and enable AccessibleCaret only on the
> tablet (mobile). kats, what do you think?

Kats is away this week so I'll chime in: I think that's reasonable. Alternatively, would it be feasible to have the caret appear on a desktop device as well, but only in response to touch events and not in response to mouse events?

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Botond Ballo [:botond] from comment #8)

> Alternatively, would it be feasible to have the caret appear on a desktop
> device as well, but only in response to touch events and not in response to
> mouse events?

Technically, this is easy to let AccessibleCaret ignore mouse events. However I feel this would be counterintuitive since the user might get frustrating that the blue carets cannot be interact with mouse.

Comment 10

[Botond Ballo [:botond]]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #9)
> (In reply to Botond Ballo [:botond] from comment #8)
> 
> > Alternatively, would it be feasible to have the caret appear on a desktop
> > device as well, but only in response to touch events and not in response to
> > mouse events?
> 
> Technically, this is easy to let AccessibleCaret ignore mouse events.
> However I feel this would be counterintuitive since the user might get
> frustrating that the blue carets cannot be interact with mouse.

I'm not suggesting that the carets, once shown, cannot be moved with the mouse - only that they don't appear in the first place in response to mouse events such as a double-click.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

See the patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1221947 for an example of how to differentiate based on the source of the events (touch, mouse, etc).

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> I'm not suggesting that the carets, once shown, cannot be moved with the
> mouse - only that they don't appear in the first place in response to mouse
> events such as a double-click.

I see :jaws already filed bug 1293483 for this.

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 12:
> I see :jaws already filed bug 1293483 for this.
We can discuss whether to enable AccessibleCaret on touch event only platforms in bug 1293483.

> I'm not suggesting that the carets, once shown, cannot be moved with the
> mouse - only that they don't appear in the first place in response to mouse
> events such as a double-click.

I see. This should already work on non-editable text. The issue here is in editable fields like <input>, <textarea> or contenteditable. When double-clicking in an editable text field, the first click makes the blinking cursor and the single AccessibleCaret show due to a collapsed selection established, and the second click is handled by the logic of pressing on the AccessibleCaret itself, which doesn't perform a standard double-click to select a word.

Although the standard action to select a word when AccessibleCaret enabled is to *long press* on a word, I can investigate a bit more to support double click to select a word for AccessibleCaret. NI myself for this.

Comment 14

[Andrew Overholt [:overholt]]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #13)
> I see. This should already work on non-editable text.

It does.

> The issue here is in
> editable fields like <input>, <textarea> or contenteditable.

Yes, that's correct.

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1292904 Part 2 - Split touch area into image and text overlay.

The touch area of the caret element includes the caret image and the
text above the image. This patch split these two areas, and call the
area covering the text "text overlay".

Also, the text overlay, image, and selection bar elements are created by
ourselves and are guaranteed to be dom::Element. That's why we can get
them and convert them to dom::Element* by using AsElement().

Review commit: https://reviewboard.mozilla.org/r/71466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71466/

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1292904 Part 3 - Use only caret image for touch area if event is a mouse type.

That is, exclude the text overlay to fix the double clicking and triple
clicking on a word in editable text area.

Review commit: https://reviewboard.mozilla.org/r/71468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71468/

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: text-overlay-and-image.png

The idea to fix double-click and triple-click is to split the touch area into the blue "text overlay" and the red "image", and exclude the "text overlay" from the touch area if the event is of mouse type.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #6)
> I think we might need a way to distinguish whether a "Windows with a touch
> screen" is a desktop or a tablet, and enable AccessibleCaret only on the
> tablet (mobile). kats, what do you think?

In general I don't like approaches that try to distinguish between "mobile" devices and "desktop" devices because there is no clear distinction between them any more. There are devices with properties of both desktop and mobile. I think the carets should be usable by either touch or mouse, or whatever other input mechanisms the platform provides.

(In reply to Botond Ballo [:botond] from comment #10)
> I'm not suggesting that the carets, once shown, cannot be moved with the
> mouse - only that they don't appear in the first place in response to mouse
> events such as a double-click.

This sounds reasonable to me. If I understand comment 13 correctly it sounds like we basically need to reimplement this double-click handling behaviour in AccessibleCaret such that when the second click happens shortly after the caret is activated, we expand the selection to the whole word. Presumably we will also need to implement the triple-click behaviour as well. I'm not sure if that's what the patches are doing but conceptually that seems like the desired behaviour to me.

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8780999 [details]
Bug 1292904 Part 3 - Use only caret image for touch area if event is a mouse type.

https://reviewboard.mozilla.org/r/71468/#review70118

Comment 21

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1292904 Part 1 - Use element id to get caret child element.

The children of the caret element already have unique class name like
"image" or "bar", we could change the class name to be id, and use id to
locate them.

Review commit: https://reviewboard.mozilla.org/r/72534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72534/

Comment 22

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8780998 [details]
Bug 1292904 Part 2 - Split touch area into image and text overlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71466/diff/1-2/

Comment 23

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8780999 [details]
Bug 1292904 Part 3 - Use only caret image for touch area if event is a mouse type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71468/diff/1-2/

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8782327 [details]
Bug 1292904 Part 1 - Use element id to get caret child element.

https://reviewboard.mozilla.org/r/72534/#review70178

::: layout/base/AccessibleCaret.cpp:227
(Diff revision 1)
>  AccessibleCaret::CreateCaretElement(nsIDocument* aDocument) const
>  {
>    // Content structure of AccessibleCaret
>    // <div class="moz-accessiblecaret">  <- CaretElement()
>    //   <div class="image">              <- CaretImageElement()
>    //   <div class="bar">                <- SelectionBarElement()

You should update this comment as well.

Comment 25

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8780998 [details]
Bug 1292904 Part 2 - Split touch area into image and text overlay.

https://reviewboard.mozilla.org/r/71466/#review70180

Comment 26

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8782327 [details]
Bug 1292904 Part 1 - Use element id to get caret child element.

https://reviewboard.mozilla.org/r/72534/#review70178

> You should update this comment as well.

Indeed. Will change class to id in next patchset.

Comment 27

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8782327 [details]
Bug 1292904 Part 1 - Use element id to get caret child element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72534/diff/1-2/

Comment 28

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8780998 [details]
Bug 1292904 Part 2 - Split touch area into image and text overlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71466/diff/2-3/

Comment 29

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8780999 [details]
Bug 1292904 Part 3 - Use only caret image for touch area if event is a mouse type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71468/diff/2-3/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d225bb9b4261
Part 1 - Use element id to get caret child element. r=mtseng
https://hg.mozilla.org/integration/autoland/rev/ce482fb4d62b
Part 2 - Split touch area into image and text overlay. r=mtseng
https://hg.mozilla.org/integration/autoland/rev/0552273a68a1
Part 3 - Use only caret image for touch area if event is a mouse type. r=mtseng

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d225bb9b4261
https://hg.mozilla.org/mozilla-central/rev/ce482fb4d62b
https://hg.mozilla.org/mozilla-central/rev/0552273a68a1