852619 - Tap off form input to clear caret selection should clear focus

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) click a text input bringing up the caret
2) click off the form to remove focus

result: the caret is cleared but the form maintains focus.

Comment 1

[Jim Mathies [:jimm]]

Similarly, when focus is active in an input and the focus selection monocles are up, taping off the input doesn't clear selection.

Comment 2

[Jim Mathies [:jimm]]

Attachment: tap focus adjustments

Comment 3

[Jim Mathies [:jimm]]

Attachment: tap focus adjustments

Updated post some review changes in dependent bugs.

Comment 4

[Allison Naaktgeboren :ally]

Comment on attachment 727209 [details] [diff] [review]
tap focus adjustments

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

I'm not terribly familiar with the selection code, but this seems solid to me. 
Couple nits, and is it possible to write a test for this?

::: browser/metro/base/content/Util.js
@@ +178,5 @@
>    /*
>     * Rect and nsIDOMRect utilities
>     */
>  
> +  cleanRect: function cleanRect() {

nit - can we call this getCleanRect()? the current name is ambiguous about whether 'clean' is a noun or a verb.

::: browser/metro/base/content/contenthandlers/SelectionHandler.js
@@ +316,5 @@
>  
>    /*
> +   * Selection clear message handler
> +   *
> +   * @param aClearFocus requests that the focus also be cleared. 

looks like you have a trailing whitespace of some sort

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +366,5 @@
>    },
>  
>    /*
>     * closeEditSessionAndClear
>     * 

more different trailing whitespace

@@ +371,5 @@
>     * Closes an active edit session and shuts down. Clears any selection region
>     * associated with the edit session.
> +   *
> +   * @param aClearFocus bool indicating if the selection handler should also
> +   * clear the focus element. optional, the default is false.

Why did you choose false as the default?

@@ +376,4 @@
>     */
> +  closeEditSessionAndClear: function closeEditSessionAndClear(aClearFocus) {
> +    let clearFocus = aClearFocus || false;
> +    this._sendAsyncMessage("Browser:SelectionClear", { clearFocus: clearFocus });

(limitation of reviewer's brain) if clearFocus is false, does the second argument become 'clearFocus:false', or 'false:false'. Might you really want {"clearFocus": clearFocus} ?

Comment 5

[Jim Mathies [:jimm]]

(In reply to :Ally Naaktgeboren from comment #4)
> Comment on attachment 727209 [details] [diff] [review]
> tap focus adjustments
> 
> Review of attachment 727209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not terribly familiar with the selection code, but this seems solid to
> me. Couple nits, and is it possible to write a test for this?

Yes, tests will get covered in bug 852090.


> @@ +371,5 @@
> >     * Closes an active edit session and shuts down. Clears any selection region
> >     * associated with the edit session.
> > +   *
> > +   * @param aClearFocus bool indicating if the selection handler should also
> > +   * clear the focus element. optional, the default is false.
> 
> Why did you choose false as the default?

The code has always depended on the old way of doing things. I added new functionality so I kept the old default.

> 
> @@ +376,4 @@
> >     */
> > +  closeEditSessionAndClear: function closeEditSessionAndClear(aClearFocus) {
> > +    let clearFocus = aClearFocus || false;
> > +    this._sendAsyncMessage("Browser:SelectionClear", { clearFocus: clearFocus });
> 
> (limitation of reviewer's brain) if clearFocus is false, does the second
> argument become 'clearFocus:false', or 'false:false'. Might you really want
> {"clearFocus": clearFocus} ?

Apparently not since this works. :) I'll ask mbrubeck about this today in irc, it's an interesting question.

Comment 6

[Sam Foster [:sfoster] (he/him)]

See https://developer.mozilla.org/en-US/docs/JavaScript/Guide/Working_with_Objects, you would need to do let params = {}; params[clearFocus] to get { false: false }; properties are always strings (only JSON requires them to be quoted).

Comment 7

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51d0ff5dc48e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51d0ff5dc48e