882902 - Find a way to attach selection generically to chrome text inputs

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Jim Mathies [:jimm]]

The work in bug 788000 attaches selection handling to the navbar directly. This doesn't cover other chrome text inputs. To attach generically we should investigate some options:

1) setup a global click listener and do some inspection work to make sure we only attach to chrome text inputs.

2) mbrubeck suggested looking at extending the xbl binding for text inputs. open question here is can the binding tell whether or not it's content or chrome.

Comment 1

[Jim Mathies [:jimm]]

Attachment: wip

Comment 2

[Jim Mathies [:jimm]]

Attachment: wip

Comment 3

[Jim Mathies [:jimm]]

Attachment: remove old bindings

Remnants of fennec apparently, there are no references to these.

Comment 4

[Jim Mathies [:jimm]]

Attachment: chrome sel fix v.1

I took mbrubeck's 2nd suggestion and hooked this up to the bindings. The short delay on turning on selection insures any processing (like select() on the navbar) has happened already.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 763706 [details] [diff] [review]
chrome sel fix v.1

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

::: browser/metro/profile/metro.js
@@ +18,5 @@
>  pref("metro.debug.treatmouseastouch", false);
>  pref("metro.debug.colorizeInputOverlay", false);
>  pref("metro.debug.selection.displayRanges", false);
>  pref("metro.debug.selection.dumpRanges", false);
> +pref("metro.debug.selection.dumpEvents", true);

(removed from my local patch.)

Comment 6

[Frank Yan (:fryn)]

Comment on attachment 763706 [details] [diff] [review]
chrome sel fix v.1

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

::: browser/metro/base/content/bindings/bindings.xml
@@ +196,5 @@
> +        if (event.mozInputSource == Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH) {
> +          setTimeout(function () {
> +            SelectionHelperUI.attachEditSession(ChromeSelectionHandler,
> +                                                event.clientX, event.clientY);
> +          }, 50);

Why 50 instead of any other value?
Is the problem that this listener is phase="capturing" while the #urlbar-edit has an onclick handler (_urlbarClicked) that runs after this?
If that's the case, we should first try to make that handler run also in phase="capturing". One way to do that is to move the _urlbarClicked call from the onclick attribute into autocomplete.xml#autocomplete. Firefox desktop's autocomplete.xml is a general binding, whereas Firefox metro's autocomplete.xml is already tightly bound to the the URL bar only as a sort of singleton, so I think it's fine to move the code into there.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Frank Yan (:fryn) from comment #6)
> Why 50 instead of any other value?

The navbar was the genesis of this but really it's more general since this connect to any text box, I wanted to be sure selection didn't trigger until other handlers (for whatever edit we happen to be working with) had a chance to fire. 

I can try your suggestion though for the nav bar. We'll have to remember that whenever we work with edits we need to be sure the selection handler here is the very last to be called.

Comment 8

[Jim Mathies [:jimm]]

Attachment: chrome sel fix v.2

updated per comments.

Comment 9

[Frank Yan (:fryn)]

Comment on attachment 764082 [details] [diff] [review]
chrome sel fix v.2

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

r+, assuming that the patch works. I forgot to test it out locally.

(In reply to Jim Mathies [:jimm] from comment #7)
> I can try your suggestion though for the nav bar. We'll have to remember
> that whenever we work with edits we need to be sure the selection handler
> here is the very last to be called.

True. It would be ideal if we could do this without making the code non-deterministic though.
One way would be to use a listener-managing helper function that maintains an ordered list of listeners that enables inserting listeners pinned to the end of that list.
If there is no maintainable deterministic approach, setTimeout with a delay of 0 would be better than an arbitrary non-zero delay.
Let's try landing this as-is. If anything breaks, I'll take the responsibility of looking for a better approach.

Thanks for working on this. :)

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08f0b84d5e62
https://hg.mozilla.org/mozilla-central/rev/754436cbbc10