Closed Bug 852087 Opened 8 years ago Closed 8 years ago

Get basic caret manipulation working in text inputs

Categories

(Firefox for Metro Graveyard :: Input, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [selection])

Attachments

(1 file, 2 obsolete files)

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: nobody → jmathies
Summary: Get basic caret manipulation working in text unputs → Get basic caret manipulation working in text inputs
Attachment #726337 - Attachment is obsolete: true
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+
(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.
Blocks: sel-tests
No longer blocks: caret-sel
oops, sorry for the spam.
Blocks: 852088
No longer blocks: sel-tests
trivial update to this landing - register for the right messages in SelectionHandler.

https://hg.mozilla.org/integration/mozilla-inbound/rev/853489b1046b
https://hg.mozilla.org/mozilla-central/rev/1407288d820b
Status: NEW → RESOLVED
Closed: 8 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.