788000 - Work - Implement selection for chrome content

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Jim Mathies [:jimm]]

+++ This bug was initially created as a clone of Bug #781487 +++

Bug 781487 overhauled selection code for content. Currently we don't have equal selection support in chrome.

Comment 1

[Allison Naaktgeboren :ally]

blocks something on the mvp list. sending to triage

Comment 2

[Erin Lancaster [:elan]]

nom'ing for IT2 to help decrease risk around Bug 791914

Comment 3

[Jim Mathies [:jimm]]

Attachment: wip

Comment 4

[Jim Mathies [:jimm]]

Attachment: wip

Comment 5

[Jim Mathies [:jimm]]

Attachment: chrome sel v.1

Comment 6

[Jim Mathies [:jimm]]

Attachment: tests v.1

Comment 7

[Jim Mathies [:jimm]]

Attachment: tests v.2

https://tbpl.mozilla.org/?tree=Try&showall=1&rev=1a27031e3947

Comment 8

[Jim Mathies [:jimm]]

Attachment: chrome sel v.2

Comment 9

[Jim Mathies [:jimm]]

Attachment: chrome sel v.2

Comment 10

[Jim Mathies [:jimm]]

Attachment: tests v.2

Comment 11

[Jim Mathies [:jimm]]

Attachment: tests v.2

basic tests for the nav bar.

Comment 12

[Jim Mathies [:jimm]]

green on try:
https://tbpl.mozilla.org/?tree=Try&showall=1&rev=c75712636007

Comment 13

[Jim Mathies [:jimm]]

Attachment: deps rollup

Everything merged fine with tip, so you should have any trouble.

Comment 14

[Matt Brubeck (:mbrubeck)]

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

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

::: browser/metro/base/content/browser-ui.js
@@ +662,5 @@
> +    if (Elements.urlbarState.getAttribute("mode") != "edit") {
> +      this._editURI(true, touchEvent);
> +      if (touchEvent) {
> +        SelectionHelperUI.attachEditSession(ChromeSelectionHandler,
> +                                            aEvent.clientX, aEvent.clientY);

I don't like how this is special-cased just for the urlbar, though it's a good start (especially since we have very few other chrome text inputs - the only ones I know are related to Sync).  Can we get a follow-up bug to extend the same behavior to all chrome text boxes?

::: browser/metro/base/content/helperui/ChromeSelectionHandler.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please add "use strict"; to the top of the file if possible.  (I've been trying to do this for all new files, when I remember, because it helps catch certain bugs sooner.)

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +895,5 @@
> +   * Detects when the nav bar hides or shows, so we can enable
> +   * selection at the appropriate location once the transition is
> +   * complete, or shutdown selection down when the nav bar is hidden.
> +   */
> +  _onNavBarTransitionEvent: function _onNavBarTransitionEvent(aEvent) {

It's sort of an organizational nit, but it feels like this belongs more in browser-ui.js than here.  Maybe we can move it out when we do the follow-up work to extend this to other chrome UI.

Comment 15

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #14)
> I don't like how this is special-cased just for the urlbar, though it's a
> good start (especially since we have very few other chrome text inputs - the
> only ones I know are related to Sync).  Can we get a follow-up bug to extend
> the same behavior to all chrome text boxes?

I'd like to make it global, but I'm not sure how to go about it. A global click capture listener that looks at the target for text edits maybe? Seems like that would have a lot of overhead.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #15)
> (In reply to Matt Brubeck (:mbrubeck) from comment #14)
> > I don't like how this is special-cased just for the urlbar, though it's a
> > good start (especially since we have very few other chrome text inputs - the
> > only ones I know are related to Sync).  Can we get a follow-up bug to extend
> > the same behavior to all chrome text boxes?
> 
> I'd like to make it global, but I'm not sure how to go about it. A global
> click capture listener that looks at the target for text edits maybe? Seems
> like that would have a lot of overhead.

Filed follow up bug 882902.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5c5cb384162
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b519589630b2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bc16dc2a866f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb17ac2733d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/186cf0ac8efc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6aadcaa65d40
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9c39cd5ddbc5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6aadcaa65d40
https://hg.mozilla.org/mozilla-central/rev/9c39cd5ddbc5