Last Comment Bug 669658 - [highlighter] Improve the keybindings
: [highlighter] Improve the keybindings
Status: RESOLVED FIXED
[minotaur][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Linux
: P1 normal (vote)
: Firefox 10
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on:
Blocks: 663830 672902
  Show dependency treegraph
 
Reported: 2011-07-06 08:56 PDT by Paul Rouget [:paul]
Modified: 2011-10-11 13:02 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (1.56 KB, patch)
2011-07-08 07:58 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v1.2 (4.77 KB, patch)
2011-07-14 17:29 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
patch v1.3 (4.61 KB, patch)
2011-07-22 09:24 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
Patch v1.4 (8.28 KB, patch)
2011-08-30 09:37 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
Patch v1.5 (10.00 KB, patch)
2011-10-03 05:33 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Review
Patch v1.6 (10.19 KB, patch)
2011-10-10 07:01 PDT, Panos Astithas [:past]
no flags Details | Diff | Review

Description Paul Rouget [:paul] 2011-07-06 08:56:05 PDT
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.
Comment 1 Paul Rouget [:paul] 2011-07-08 07:58:12 PDT
Created attachment 544821 [details] [diff] [review]
patch v1
Comment 2 Rob Campbell [:rc] (:robcee) 2011-07-13 11:34:03 PDT
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?
Comment 3 Paul Rouget [:paul] 2011-07-14 17:29:42 PDT
Created attachment 546053 [details] [diff] [review]
patch v1.2

Now works in the HTML tree as well.
Added a test.
Comment 4 Rob Campbell [:rc] (:robcee) 2011-07-18 15:15:48 PDT
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.
Comment 5 Paul Rouget [:paul] 2011-07-22 09:24:29 PDT
Created attachment 547714 [details] [diff] [review]
patch v1.3

Addressed Rob's comments.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-02 13:10:06 PDT
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?
Comment 7 Paul Rouget [:paul] 2011-08-02 13:32:48 PDT
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 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-02 15:25:28 PDT
Comment on attachment 547714 [details] [diff] [review]
patch v1.3

Sorry, yes, that makes sense. I just got confused.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-10 11:10:14 PDT
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 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-10 11:51:26 PDT
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).
Comment 11 Rob Campbell [:rc] (:robcee) 2011-08-11 06:19:45 PDT
(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?
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-11 14:26:04 PDT
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.
Comment 13 Panos Astithas [:past] 2011-08-29 11:18:43 PDT
I'll be working on this, since Paul has enough on his plate already.
Comment 14 Panos Astithas [:past] 2011-08-30 09:37:03 PDT
Created attachment 556881 [details] [diff] [review]
Patch v1.4

Rebased, inlined attach/detachPageListeners and fixed a bug probably caused by bitrot.
Comment 15 Panos Astithas [:past] 2011-10-03 05:33:13 PDT
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
Comment 16 Panos Astithas [:past] 2011-10-03 05:59:19 PDT
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.
Comment 17 Rob Campbell [:rc] (:robcee) 2011-10-07 08:54:03 PDT
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.
Comment 18 Mihai Sucan [:msucan] 2011-10-07 09:04:05 PDT
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);
Comment 19 Panos Astithas [:past] 2011-10-10 07:01:58 PDT
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.
Comment 20 Rob Campbell [:rc] (:robcee) 2011-10-11 06:24:25 PDT
https://hg.mozilla.org/integration/fx-team/rev/9d697ecaf161
Comment 21 Rob Campbell [:rc] (:robcee) 2011-10-11 13:02:07 PDT
https://hg.mozilla.org/mozilla-central/rev/9d697ecaf161

Note You need to log in before you can comment on or make changes to this bug.