852087 - Get basic caret manipulation working in text inputs

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Jim Mathies [:jimm]]

todo:

- display a selection monocle at the caret position when a text input has focus
- allow the user to drag the caret monocle to move the caret
- tap in other areas of the text input to move the caret
- tap off the text input (change focus) to clear selection mode

Comment 1

[Jim Mathies [:jimm]]

Attachment: basics - moving the caret around (wip)

Comment 2

[Jim Mathies [:jimm]]

Attachment: basics - moving the caret around v.1

Comment 3

[Jim Mathies [:jimm]]

Attachment: basics - moving the caret around v.2

I'll land this with the work in bug 852088.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 726818 [details] [diff] [review]
basics - moving the caret around v.2

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

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +59,5 @@
>      addMessageListener("Browser:SelectionDebug", this);
> +    addMessageListener("Browser:CaretAttach", this);
> +    addMessageListener("Browser:CaretPositionStart", this);
> +    addMessageListener("Browser:CaretPosition", this);
> +    addMessageListener("Browser:CaretPositionEnd", this);

"Browser:CaretPositionStart" is confusingly named -- I would expect it to be the start of a message sequence that ends with CaretPositionEnd, but it is not.

Could we rename these to something like:

CaretPositionStart -> CaretUpdate
CaretPosition      -> CaretDragMove
CaretPositionEnd   -> CaretDragEnd

@@ +432,5 @@
>      this._updateUIMarkerRects(selection);
>  
>      this._cache.updateStart = aUpdateStart;
>      this._cache.updateEnd = aUpdateEnd;
> +    this._cache.updateCaret = (aUpdateCaret == undefined ? false : aUpdateCaret);

I'd prefer "!!aUpdateCaret" or "aUpdateCaret || false" as more idiomatic ways to "cast to bool" here.

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +712,5 @@
>    },
>  
>    markerDragStart: function markerDragStart(aMarker) {
>      let json = this._getMarkerBaseMessage();
>      json.change = aMarker.tag;

json.change is not used for caret messages, so we can move this line below the "if (aMarker.tag == "caret")" block.  (The same comment applies to markerDragMove and markerDragStop.)

@@ +716,5 @@
>      json.change = aMarker.tag;
> +    if (aMarker.tag == "caret") {
> +      this._sendAsyncMessage("Browser:CaretPosition", json);
> +      return;
> +    }

Is it useful to send a message on dragstart, or can we skip this?

::: browser/metro/theme/browser.css
@@ +952,5 @@
>  }
>  
> +#selectionhandle-caret {
> +  list-style-image: url("chrome://browser/skin/images/selection-monocle.png");
> +}

Please move this (and the other two selection-monocle.png styles) into the shared ruleset above.

Comment 5

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> > +    addMessageListener("Browser:CaretAttach", this);
> > +    addMessageListener("Browser:CaretPositionStart", this);
> > +    addMessageListener("Browser:CaretPosition", this);
> > +    addMessageListener("Browser:CaretPositionEnd", this);
> 
> "Browser:CaretPositionStart" is confusingly named -- I would expect it to be
> the start of a message sequence that ends with CaretPositionEnd, but it is
> not.
> 
> Could we rename these to something like:
> 
> CaretPositionStart -> CaretUpdate
> CaretPosition      -> CaretDragMove
> CaretPositionEnd   -> CaretDragEnd

Sure, sounds good to me. These started off at matching Selection drag events, but morphed over time. I should have updated them to better reflect what they do.

Note drag support works with this patch (and the code will continue to support it if we want) but in the follow up patch dragging the caret around will not be used, we'll switch to selection mode the second a drag starts. (See bug 852088)

> @@ +432,5 @@
> >      this._updateUIMarkerRects(selection);
> >  
> >      this._cache.updateStart = aUpdateStart;
> >      this._cache.updateEnd = aUpdateEnd;
> > +    this._cache.updateCaret = (aUpdateCaret == undefined ? false : aUpdateCaret);
> 
> I'd prefer "!!aUpdateCaret" or "aUpdateCaret || false" as more idiomatic
> ways to "cast to bool" here.

Not a problem. This actually reminds me of a bug I need to file. The old fennec code cached all the positional information it sent over from some reason, but we don't need to do that anymore, so I think this._cache in SelectionHandler can go away. 

> >    markerDragStart: function markerDragStart(aMarker) {
> >      let json = this._getMarkerBaseMessage();
> >      json.change = aMarker.tag;
> 
> json.change is not used for caret messages, so we can move this line below
> the "if (aMarker.tag == "caret")" block.  (The same comment applies to
> markerDragMove and markerDragStop.)

Actually, I may not be using it, but I'd like the message to reflect what marker changed for completeness. We might look at that in the future and I'd like it to be up to date if we do.

> 
> @@ +716,5 @@
> >      json.change = aMarker.tag;
> > +    if (aMarker.tag == "caret") {
> > +      this._sendAsyncMessage("Browser:CaretPosition", json);
> > +      return;
> > +    }
> 
> Is it useful to send a message on dragstart, or can we skip this?

Not sure, I can take a look to see if it can be skipped.

> > +#selectionhandle-caret {
> > +  list-style-image: url("chrome://browser/skin/images/selection-monocle.png");
> > +}
> 
> Please move this (and the other two selection-monocle.png styles) into the
> shared ruleset above.

ah, yes. good idea.

Comment 6

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1407288d820b

Comment 7

[Jim Mathies [:jimm]]

oops, sorry for the spam.

Comment 8

[Jim Mathies [:jimm]]

trivial update to this landing - register for the right messages in SelectionHandler.

https://hg.mozilla.org/integration/mozilla-inbound/rev/853489b1046b

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1407288d820b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/853489b1046b