Closed Bug 891805 Opened 7 years ago Closed 7 years ago

Initial selection off caret monocle drag has poor bounds targeting

Categories

(Firefox for Metro Graveyard :: Input, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [selection] p=1)

Attachments

(3 files, 1 obsolete file)

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.
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attached image coordinate example
Attachment #773520 - Attachment is patch: false
Attachment #773520 - Attachment mime type: text/plain → image/png
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
Attached patch fixSplinter Review
A smallish bug fix that improves caret selection accuracy and a touchup for that content editable test.
Attachment #773521 - Flags: review?(sfoster)
Attachment #773519 - Attachment is obsolete: true
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.
Attachment #773521 - Flags: review?(sfoster) → review?(rsilveira)
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.
Attachment #773521 - Flags: review?(rsilveira) → review+
(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.
(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.)
Whiteboard: [selection] → [selection] p=1
https://hg.mozilla.org/mozilla-central/rev/80f127b3950f
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.