669658 - [highlighter] Improve the keybindings

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Paul Rouget [:paul]]

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]]

Attachment: patch v1

Comment 2

[Rob Campbell [:rc] (:robcee)]

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]]

Attachment: patch v1.2

Now works in the HTML tree as well.
Added a test.

Comment 4

[Rob Campbell [:rc] (:robcee)]

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]]

Attachment: patch v1.3

Addressed Rob's comments.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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]]

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: [e-mail]]]

Comment on attachment 547714 [details] [diff] [review]
patch v1.3

Sorry, yes, that makes sense. I just got confused.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

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)]

(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: [e-mail]]]

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 (he/him) [:past] (please ni?)]

I'll be working on this, since Paul has enough on his plate already.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1.4

Rebased, inlined attach/detachPageListeners and fixed a bug probably caused by bitrot.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: 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 (he/him) [:past] (please ni?)]

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)]

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]]

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 (he/him) [:past] (please ni?)]

Attachment: 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)]

https://hg.mozilla.org/integration/fx-team/rev/9d697ecaf161

Comment 21

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/9d697ecaf161