Get basic caret manipulation working in text inputs

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [selection])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Assignee: nobody → jmathies
(Assignee)

Updated

6 years ago
Summary: Get basic caret manipulation working in text unputs → Get basic caret manipulation working in text inputs
(Assignee)

Comment 1

6 years ago
Created attachment 726337 [details] [diff] [review]
basics - moving the caret around (wip)
(Assignee)

Comment 2

6 years ago
Created attachment 726716 [details] [diff] [review]
basics - moving the caret around v.1
Attachment #726337 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 726818 [details] [diff] [review]
basics - moving the caret around v.2

I'll land this with the work in bug 852088.
Attachment #726716 - Attachment is obsolete: true
Attachment #726818 - Flags: review?(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.
Attachment #726818 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 852090
No longer blocks: 851388
(Assignee)

Comment 7

6 years ago
oops, sorry for the spam.
Blocks: 852088
No longer blocks: 852090
(Assignee)

Comment 8

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

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

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1407288d820b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.