Closed Bug 788000 Opened 7 years ago Closed 7 years ago

Work - Implement selection for chrome content

Categories

(Firefox for Metro Graveyard :: Input, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [selection] feature=work)

Attachments

(3 files, 7 obsolete files)

+++ 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.
Whiteboard: completed-elm
Blocks: 791914, 790090
Product: Firefox → Firefox for Metro
blocks something on the mvp list. sending to triage
Whiteboard: [metro-mvp?]
Whiteboard: [metro-mvp?] → [metro-mvp] [LOE:2]
nom'ing for IT2 to help decrease risk around Bug 791914
Blocks: 831985
No longer blocks: 831985
Blocks: 831908, 831909
Whiteboard: [metro-mvp] [LOE:2] → [metro-mvp] [LOE:2] feature=work
Summary: Implement selection for chrome content → Work - Implement selection for chrome content
Whiteboard: [metro-mvp] [LOE:2] feature=work → feature=work
Depends on: 812529
No longer blocks: 831908
No longer blocks: 791914
No longer depends on: 812529
Component: General → Input
Whiteboard: feature=work → [selection] feature=work
Depends on: 858358
Depends on: 869437
No longer depends on: 858358
Assignee: nobody → jmathies
Depends on: 879454
No longer depends on: 869437
Depends on: 879883
Depends on: 879887
Attached patch wip (obsolete) — Splinter Review
Attached patch wip (obsolete) — Splinter Review
Attachment #758686 - Attachment is obsolete: true
Attached patch chrome sel v.1 (obsolete) — Splinter Review
Attachment #759244 - Attachment is obsolete: true
Attached patch tests v.1 (obsolete) — Splinter Review
Attached patch tests v.2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&showall=1&rev=1a27031e3947
Attachment #760561 - Attachment is obsolete: true
Attached patch chrome sel v.2 (obsolete) — Splinter Review
Attachment #760560 - Attachment is obsolete: true
Attachment #760971 - Attachment is obsolete: true
Attached patch chrome sel v.2Splinter Review
Attachment #761756 - Attachment is obsolete: true
Attachment #761760 - Flags: review?(mbrubeck)
Attached patch tests v.2 (obsolete) — Splinter Review
Attached patch tests v.2Splinter Review
basic tests for the nav bar.
Attachment #761762 - Attachment is obsolete: true
Attachment #761766 - Flags: review?(mbrubeck)
Attachment #761766 - Flags: review?(mbrubeck) → review+
Attached patch deps rollupSplinter Review
Everything merged fine with tip, so you should have any trouble.
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.
Attachment #761760 - Flags: review?(mbrubeck) → review+
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/6aadcaa65d40
https://hg.mozilla.org/mozilla-central/rev/9c39cd5ddbc5
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.