891805 - Initial selection off caret monocle drag has poor bounds targeting

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Jim Mathies [:jimm]]

This has been annoying me lately, so I'd like to try and find a fix. It's also the cause of tests bug 891462.

Basically the selection bound should always start from the point at which the caret monocle is position when you start a drag. But it is often offset a character or two in the direction you drag the monocle.

Comment 1

[Jim Mathies [:jimm]]

Attachment: example screenshot off test slaves

Comment 2

[Jim Mathies [:jimm]]

Attachment: fix

Comment 3

[Jim Mathies [:jimm]]

Attachment: coordinate example

Comment 4

[Jim Mathies [:jimm]]

try run with fix, without the coordinate change:
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=af34d995d074

try run with fix with coordinate change:
https://tbpl.mozilla.org/?tree=Try&showall=0&rev=5412f7f2c785

Comment 5

[Jim Mathies [:jimm]]

Attachment: fix

A smallish bug fix that improves caret selection accuracy and a touchup for that content editable test.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 773521 [details] [diff] [review]
fix

Rodrigo, sfoster just got hit with a big autocomplete review so curious if you might have a spare cycle to take this smallish change.

Comment 7

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 773521 [details] [diff] [review]
fix

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

I applied the patch and found it really hard to select text when the caret was at the end or beginning of a textbox. When the caret was on the middle of a word, dragging would select one extra letter too. Code looks good.

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +1106,5 @@
>  
>    markerDragStart: function markerDragStart(aMarker) {
>      let json = this._getMarkerBaseMessage(aMarker.tag);
>      if (aMarker.tag == "caret") {
> +      this._cachedCaretPos = null;

I'm curious to why the caching happens on dragMove instead of here.

@@ +1135,5 @@
>          return false;
>        }
> +      // Cache for when we start the drag in _transitionFromCaretToSelection.
> +      if (!this._cachedCaretPos) {
> +        this._cachedCaretPos = this._getMarkerBaseMessage(aMarker.tag);

You caching more then you need, consider caching only caret pos.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Rodrigo Silveira [:rsilveira] from comment #7)
> Comment on attachment 773521 [details] [diff] [review]
> fix
> 
> Review of attachment 773521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I applied the patch and found it really hard to select text when the caret
> was at the end or beginning of a textbox. When the caret was on the middle
> of a word, dragging would select one extra letter too. Code looks good.

Yeah I get that extra letter sometimes. Looks like:

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/contenthandlers/SelectionHandler.js#109

should not use selectAtPoint for text inputs, since the caret usually sits between two characters, and selectAtPoint is 'smart' in that it'll grab each character on either side of the coordinate. I'll file a follow up. Not sure I'd block roll out on it though.

> 
> ::: browser/metro/base/content/helperui/SelectionHelperUI.js
> @@ +1106,5 @@
> >  
> >    markerDragStart: function markerDragStart(aMarker) {
> >      let json = this._getMarkerBaseMessage(aMarker.tag);
> >      if (aMarker.tag == "caret") {
> > +      this._cachedCaretPos = null;
> 
> I'm curious to why the caching happens on dragMove instead of here.

We could. We don't know the user is dragging until the move, so this would cache on taps as well. In practice the two seem to work identically.

> 
> @@ +1135,5 @@
> >          return false;
> >        }
> > +      // Cache for when we start the drag in _transitionFromCaretToSelection.
> > +      if (!this._cachedCaretPos) {
> > +        this._cachedCaretPos = this._getMarkerBaseMessage(aMarker.tag);
> 
> You caching more then you need, consider caching only caret pos.

Good catch, will update.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #8)
> (In reply to Rodrigo Silveira [:rsilveira] from comment #7)
> > Comment on attachment 773521 [details] [diff] [review]
> > fix
> > 
> > Review of attachment 773521 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I applied the patch and found it really hard to select text when the caret
> > was at the end or beginning of a textbox. When the caret was on the middle
> > of a word, dragging would select one extra letter too. Code looks good.
> 
> Yeah I get that extra letter sometimes. Looks like:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> contenthandlers/SelectionHandler.js#109
> 
> should not use selectAtPoint for text inputs, since the caret usually sits
> between two characters, and selectAtPoint is 'smart' in that it'll grab each
> character on either side of the coordinate. I'll file a follow up. Not sure
> I'd block roll out on it though.

(I would normally try and fix this here, but it may be involved, and I'd like to get rid of that failing test.)

Comment 10

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80f127b3950f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80f127b3950f