[highlighter] Improve the keybindings

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: past)

Tracking

unspecified
Firefox 10
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur][fixed-in-fx-team])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Right now, we can lock a node with Enter or Escape. We can't unlock a node with a key binding.

Proposal:
While inspecting, Enter and Escape lock the node.
When the node is locked, Enter and Escape unlock the node.
(Reporter)

Updated

6 years ago
No longer depends on: 669652
(Reporter)

Comment 1

6 years ago
Created attachment 544821 [details] [diff] [review]
patch v1
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #544821 - Flags: review?(rcampbell)
Comment on attachment 544821 [details] [diff] [review]
patch v1

this needs a unittest.

Some slight rewordings:
+    // We still want to be notified if the user press "ESCAPE" to unlock
+    // the node, se we don't remove the "keypress" event.

// We still want to be notified if the user presses "ESCAPE" to
// unlock the node, we don't remove the "keypress" event until
// the highlighter is removed.

Does this also work of the HTML panel is focused?
(Reporter)

Comment 3

6 years ago
Created attachment 546053 [details] [diff] [review]
patch v1.2

Now works in the HTML tree as well.
Added a test.
Attachment #544821 - Attachment is obsolete: true
Attachment #546053 - Flags: review?(rcampbell)
Attachment #544821 - Flags: review?(rcampbell)
Comment on attachment 546053 [details] [diff] [review]
patch v1.2


+  let utils =
+    window.QueryInterface(Ci.nsIInterfaceRequestor).
+      getInterface(Ci.nsIDOMWindowUtils);

not really needed. (you'll see why in a few lines...)

+  gBrowser.selectedBrowser.addEventListener("load", function() {
+    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);

good practice to get used to naming your functions and removing the listener explicitly. "arguments.callee" is going away.

+  function lockNode()
+  {
+    Services.obs.removeObserver(lockNode,
+      INSPECTOR_NOTIFICATIONS.HIGHLIGHTING);
+
+    utils.sendKeyEvent("keypress", KeyEvent.DOM_VK_ESCAPE, 0, 0);

you can use EventUtils.synthesizeKey() here. See:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_visibleFindSelection.js#34

and here:

+  function unlockNode() {
+    utils.sendKeyEvent("keypress", KeyEvent.DOM_VK_RETURN, 0, 0);

looks good! r+ with these fixed.
Attachment #546053 - Flags: review?(rcampbell) → review+
(Reporter)

Updated

6 years ago
Whiteboard: [minotaur]
(Reporter)

Comment 5

6 years ago
Created attachment 547714 [details] [diff] [review]
patch v1.3

Addressed Rob's comments.
Attachment #546053 - Attachment is obsolete: true
Attachment #547714 - Flags: review?(rcampbell)
Attachment #547714 - Flags: review?(rcampbell) → review+
(Reporter)

Updated

6 years ago
Attachment #547714 - Flags: review?(gavin.sharp)
Priority: -- → P1
Whiteboard: [minotaur] → [minotaur][has-patch]
Comment on attachment 547714 [details] [diff] [review]
patch v1.3

>diff -r 3f9ccfb84896 browser/base/content/inspector.js

>     this.stopInspecting();
>+    this.browser.removeEventListener("keypress", this, true);

>   detachPageListeners: function IUI_detachPageListeners()
>   {
>-    this.browser.removeEventListener("keypress", this, true);
>+    // We still want to be notified if the user presses "ESCAPE" to
>+    // unlock the node, we don't remove the "keypress" event until
>+    // the highlighter is removed.

I don't understand this change. detachPageListeners is only called once, from stopInspecting(), so this change looks like a no-op. Am I missing something?
Attachment #547714 - Flags: review?(gavin.sharp) → review-
(Reporter)

Comment 7

6 years ago
Not sure to understand what the problem is.

stopInspecting is called several times. We need to remove the keypress listener just after one of this call, not every time.

Does it make sense?
Comment on attachment 547714 [details] [diff] [review]
patch v1.3

Sorry, yes, that makes sense. I just got confused.
Attachment #547714 - Flags: review- → review?(gavin.sharp)
Just a note for the future, it would be useful if patches were attached with at least 8 lines of context (Hg configuration instructions for that are at https://developer.mozilla.org/en/Installing_Mercurial#Configuration ).
Comment on attachment 547714 [details] [diff] [review]
patch v1.3

If I understand things correctly, this means that Enter/Esc can be used to toggle inspection once inspection has been started once (i.e. attachPageListeners has been called), but not before that. Also Enter/Esc are both global toggles for inspection at all times. That seems kind of odd.

I think things would be a lot easier to follow if attach/detachPageListeners were just inlined in startInspecting/stopInspecting. But to address the inconsistency above, it seems like you should add the keypress listeners in openInspectorUI rather than attachPageListeners. Also, is it really necessary to add the listeners to each of the browser/treeIframe/highlighterContainer? It seems odd that all of those can end up receiving key events (i.e. having focus).
Attachment #547714 - Flags: review?(gavin.sharp)
(In reply to Gavin Sharp from comment #9)
> Just a note for the future, it would be useful if patches were attached with
> at least 8 lines of context (Hg configuration instructions for that are at
> https://developer.mozilla.org/en/Installing_Mercurial#Configuration ).

funny, but mq likes smaller contexts for applying. Makes it easier to rebase them. The downside is that it makes these patches harder to review (and often import at checkin time).

We might have to start using explicit -U 8 on our qdiffs for review.

(In reply to Gavin Sharp from comment #10)
> Comment on attachment 547714 [details] [diff] [review]
> patch v1.3
> 
> If I understand things correctly, this means that Enter/Esc can be used to
> toggle inspection once inspection has been started once (i.e.
> attachPageListeners has been called), but not before that. Also Enter/Esc
> are both global toggles for inspection at all times. That seems kind of odd.

Not really. Starting and Stopping the inspector (active inspection, highlighting?) is probably the most frequent action. It should be easy to do. I'm not sure I like ESC starting inspection, that seems weird, but I'd like to have a single, easy to hit key that can do this. I'd suggest spacebar, but that's used for page scrolling.

> I think things would be a lot easier to follow if attach/detachPageListeners
> were just inlined in startInspecting/stopInspecting. But to address the
> inconsistency above, it seems like you should add the keypress listeners in
> openInspectorUI rather than attachPageListeners.

That's a good suggestion.

> Also, is it really
> necessary to add the listeners to each of the
> browser/treeIframe/highlighterContainer? It seems odd that all of those can
> end up receiving key events (i.e. having focus).

Unfortunately, I think it is. Unless we can add them to the browser document and have them override whatever's going on in the tree panel's iframe. I could see that having unwanted side effects when we introduce the attribute editing patch.

What do you think?
I've never had much trouble keeping my MQ diffs at 8 lines of context, for what it's worth, but then maybe I don't bitrot myself enough for it to matter.

You don't need to make any changes based on my comments if you don't think they're necessary - feel free to re-request review with whatever you want to do. I don't really have any ideas about what shortcuts keys should be used for what.
Blocks: 672902
Whiteboard: [minotaur][has-patch] → [minotaur][has-patch][needs-update]
I'll be working on this, since Paul has enough on his plate already.
Assignee: paul → past
Created attachment 556881 [details] [diff] [review]
Patch v1.4

Rebased, inlined attach/detachPageListeners and fixed a bug probably caused by bitrot.
Attachment #547714 - Attachment is obsolete: true
Attachment #556881 - Flags: review?(gavin.sharp)
Created attachment 564163 [details] [diff] [review]
Patch v1.5

Rebased and updated after the recent move of inspector.js to the devtools module. Gavin, there is no need for another review now, but if you've already started it or want to do it anyway, give me a shout.

Try run results at:

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=958847d0b8b8
Attachment #556881 - Attachment is obsolete: true
Attachment #556881 - Flags: review?(gavin.sharp)
Comment on attachment 564163 [details] [diff] [review]
Patch v1.5

This patch could use another look, mainly because of the inspector.js -> inspector.jsm + TreePanel.jsm refactoring.
Attachment #564163 - Flags: review?(rcampbell)
Comment on attachment 564163 [details] [diff] [review]
Patch v1.5

in TreePanel.jsm:

+    this.treeIFrame.addEventListener("keypress",
+                                     this.IUI.handleEvent.bind(this.IUI),
+                                     false);
...
+      this.treeIFrame.removeEventListener("keypress",
+                                          this.IUI.handleEvent,
+                                          false);

you're not really removing the same bound event. You'll need to cache the bound event so you can later remove it correctly like this:

this.boundIUIHandleEvent = this.IUI.handleEvent.bind(this.IUI);
addEventListener...

and then in your removeEventListener:

+      this.treeIFrame.removeEventListener("keypress",
+                                          this.boundIUIHandleEvent,
+                                          false);

and delete the bound event at removal.

Alternatively, since IUI implements handleEvent, can you just submit "this.IUI" as the event handler as you do in inspector.jsm?

test looks fine, r+ with above addressed.
Attachment #564163 - Flags: review?(rcampbell) → review+
Comment on attachment 564163 [details] [diff] [review]
Patch v1.5

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

A quick look...

::: browser/devtools/highlighter/TreePanel.jsm
@@ +140,5 @@
>      this.treeIFrame.addEventListener("click", this.onTreeClick.bind(this), false);
>      this.treeIFrame.addEventListener("dblclick", this.onTreeDblClick.bind(this), false);
> +    this.treeIFrame.addEventListener("keypress",
> +                                     this.IUI.handleEvent.bind(this.IUI),
> +                                     false);

You do not need to bind the function, nor store the bound function. Just do:

this.treeIFrame.addEventListener("keypress", this.IUI, false);

later...

this.treeIFrame.removeEventListener("keypress", this.IUI, false);
Created attachment 565929 [details] [diff] [review]
Patch v1.6

Thank you both, I changed the listener registration to mimic the one in inspector.jsm, like you suggested.
Attachment #564163 - Attachment is obsolete: true
Whiteboard: [minotaur][has-patch][needs-update] → [minotaur][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/9d697ecaf161
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9d697ecaf161
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
You need to log in before you can comment on or make changes to this bug.