Closed Bug 882902 Opened 6 years ago Closed 6 years ago

Find a way to attach selection generically to chrome text inputs

Categories

(Firefox for Metro Graveyard :: Input, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Assignee: nobody → jmathies
Attached patch wip (obsolete) — Splinter Review
Attached patch wip (obsolete) — Splinter Review
Attachment #763666 - Attachment is obsolete: true
Remnants of fennec apparently, there are no references to these.
Attachment #763667 - Attachment is obsolete: true
Attachment #763705 - Flags: review?(fyan)
Attached patch chrome sel fix v.1 (obsolete) — Splinter Review
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.
Attachment #763706 - Flags: review?(fyan)
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.)
Attachment #763705 - Flags: review?(fyan) → review+
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.
Attachment #763706 - Flags: review?(fyan)
(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.
updated per comments.
Attachment #763706 - Attachment is obsolete: true
Attachment #764082 - Flags: review?(fyan)
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. :)
Attachment #764082 - Flags: review?(fyan) → review+
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.