Closed Bug 1293483 Opened 8 years ago Closed 8 years ago

Don't show the accessible / touch caret when the mouse is the source of the event

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: jaws, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

photo167683873108502585.jpg
(deleted), image/jpeg
Details
Bug 1293483 - Keep the carets hidden when the user is using a mouse to modify the selection.
(deleted), text/x-review-board-request
TYLin
: review+
Details 
We should only show the touch caret when a touch event was used. See https://bugzilla.mozilla.org/show_bug.cgi?id=1221947 for details on how this can be done.
I actually find this really useful even when not using touch to select things.
Does AccessibleCaret worth having a product switch in preference UI so that the user could enable/disable it easily?
Blocks: AccessibleCaret
(In reply to Dave Townsend [:mossop] from comment #1)
> I actually find this really useful even when not using touch to select
> things.

Well, it breaks double-clicking and triple-clicking to select text in the textarea so in that sense it's less than useful. Does the mouse cursor not provide enough of a handle for making a selection?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> (In reply to Dave Townsend [:mossop] from comment #1)
> > I actually find this really useful even when not using touch to select
> > things.
> 
> Well, it breaks double-clicking and triple-clicking to select text in the
> textarea so in that sense it's less than useful. Does the mouse cursor not
> provide enough of a handle for making a selection?

I'm assuming we can fix those bugs.

What mouse cursor are you referring to?
Yes, if those bugs are fixed then this would be fine. I'm referring to cursor:text, which shows up on textareas and input[type=text].
Attached image photo167683873108502585.jpg (deleted) — 
¡Hola Jared!

Is this bug pictured on the attachment? If so, this just bite me on Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0 ID:20160810030202 CSet: 6cf0089510fad8deb866136f5b92bbced9498447

Could you please confirm?

¡Gracias!
Alex
Flags: needinfo?(jaws)
Yes, that is this bug.
Flags: needinfo?(jaws)
status-firefox51: --- → affected
Version: unspecified → 51 Branch
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Well, it breaks double-clicking and triple-clicking to select text in the
> textarea so in that sense it's less than useful.

Is this still an issue for you? I just tested in the latest nightly and double/triple-clicking seems to work as expected with e10s force-enabled (which enables the accessible caret). I tried both on regular text and textareas.
Flags: needinfo?(jaws)
(It was fixed in bug 1292904.)

I propose to close this bug as wontfix because I personally agree with :mossop and feel that the teardrop handles are nice and make it easier to drag around the selection. It also allows the user to seamlessly switch between touch and mouse when creating a selection which is nice.

Does anybody have any objections?
I'm fine with closing as wontfix.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(jaws)
Resolution: --- → WONTFIX
Yes, I do have objections ;)

At the very least, always showing the handles is big deviation from the platform standard, so we should have a solid reason to do it. Making selections easier to change can be interesting, but these kinds of »accidental« solutions are rarely the best ones.
It is currently also questionable that the benefit introduced by always showing the handles is worth the cost of change (cost of change in terms of user impact).
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I have a WIP for this at https://github.com/staktrace/gecko-dev/commit/99e235571454a44b4360f26aafae0f48e4efc67c which I'll probably finish off tomorrow.
Assignee: nobody → bugmail
I got this working, but there's some ugliness involved. TYLin, do you have thoughts on how to do this differently that might be better? My solution currently consists of three patches: [1][2][3]. The first one is the main fix, which simply keeps the carets hidden if the selection is changed by a mouse input. The second patch is some test updating.

However, with just those two patches, touch-tapping in a textfield to reposition the cursor keeps the caret hidden - this is because it's the synthetic mouse events from the tap that reposition the selection, and so it gets picked up as a "mouse" input which keeps the caret hidden. The third patch tries to annotate this so we know that it's coming from a touch input - it works but it's really ugly and I'd rather find another solution. Any thoughts?

[1] https://github.com/staktrace/gecko-dev/commit/7bf23cb803ccdf714bab76eb51cd351ce1deb92e
[2] https://github.com/staktrace/gecko-dev/commit/68f79a998da8810ea0873c3ad99ff2bb3051557e
[3] https://github.com/staktrace/gecko-dev/commit/7bb3e31f072974b3a86e03d6fe525bf6a3849647
Flags: needinfo?(tlin)
These patches also break the scenario you described in bug 1300905 comment 10 and possibly other things. I have another idea that I'll try out.
Flags: needinfo?(tlin)
I think about this a bit today. The synthetic mouse event is from APZCCallbackHelper::FireSingleTapEvent() [1], right?

I see the problem is that AccessibleCaretManager::OnSelectionChanged cannot tell whether the selection is being changed due to mouse event or touch event from the aReason. Showing the single caret after single tapping doesn't involve interaction of the caret itself, so the state transition of AccessibleCaretEventHub could not offer much help directly. You might notice that when OnSelectionChanged() is called with the mouse up reason, AccessibleCaretEventHub is at NoActionState.

I have a vague idea that we might transit to a new state after receving eTocuhEnd, and observing the three synthetic mouse events eMouseMove, eMouseDown, and eMouseUp. Then this state could pass a parameter so that OnSelectionChanged() knows it should update carets. However this idea seems fragile.

It's late today. Hope you've got a better idea :)

[1] http://searchfox.org/mozilla-central/rev/d1276b5b84e6cf7991c8e640b5e0ffffd54575a6/gfx/layers/apz/util/APZCCallbackHelper.cpp#526
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #16)
> I have a vague idea that we might transit to a new state after receving
> eTocuhEnd, and observing the three synthetic mouse events eMouseMove,
> eMouseDown, and eMouseUp. Then this state could pass a parameter so that
> OnSelectionChanged() knows it should update carets. However this idea seems
> fragile.

This is pretty close to what I was thinking. Instead of adding a new state though, I just have the EventHub tell the Manager what was the last input type it saw. Then the Manager can show or hide the carets based on that. It seems to work pretty well and I think it's more robust/cleaner than my previous approach. I think it's also more in line with what we're conceptually trying to accomplish - that is, if the user is doing stuff with the mouse, then hide the carets, but if we discover the user is actively using touch input (either by long-pressing on an existing selection, creating a new selection via touch, or even just by touch-panning the page) then we can go ahead and show the carets.
Comment on attachment 8796565 [details]
Bug 1293483 - Keep the carets hidden when the user is using a mouse to modify the selection.

https://reviewboard.mozilla.org/r/82382/#review81292

I guess this solution is the simplest one we can get and is pref-controlled, so r=me.

After this landed, I think the only possible action to trigger carets show by mouse is mouse long-tapping because we still have long tap injector turned on. Luckily, bug 1304263 will cover that.

::: layout/base/AccessibleCaretEventHub.cpp:523
(Diff revision 2)
> +      aEvent->mMessage == eMouseUp ||
> +      aEvent->mMessage == eMouseClick ||
> +      aEvent->mMessage == eMouseDoubleClick ||
> +      aEvent->mMessage == eMouseLongTap) {
> +    // Don't reset the source on mouse movement since that can
> +    // happen anytime, even randomly during a touch sequence

Nit: Period at the end of the comment, please.
Attachment #8796565 - Flags: review?(tlin) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5cc5227236
Keep the carets hidden when the user is using a mouse to modify the selection. r=TYLin
https://hg.mozilla.org/mozilla-central/rev/2b5cc5227236
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Had these markers appear when using a Wacom graphics tablet (non touch) with a stylus in Firefox Developer version 52.0a2 for the first time today.

A link was clicked from outside the browser, opening the page in the current tab of FF which had a text field and some text in it. I canceled the link from loading before it had changed the visibility of the previous page with the text field in it (ie: the browser considered it a new page but the contents of the old page were still visible). And this is when these markers appeared in the text field, where the previous page's text was still selectable. Clicking back and reloading the page made the markers disappear.

Probably a rare instance but thought I'd mention it as it apparently will load in some circumstances for graphics tablet users.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: