Closed
Bug 669658
Opened 13 years ago
Closed 13 years ago
[highlighter] Improve the keybindings
Categories
(DevTools :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 10
People
(Reporter: paul, Assigned: past)
References
Details
(Whiteboard: [minotaur][fixed-in-fx-team])
Attachments
(1 file, 5 obsolete files)
10.19 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•13 years ago
|
||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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 4•13 years ago
|
||
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•13 years ago
|
Whiteboard: [minotaur]
Reporter | ||
Comment 5•13 years ago
|
||
Addressed Rob's comments.
Attachment #546053 -
Attachment is obsolete: true
Attachment #547714 -
Flags: review?(rcampbell)
Updated•13 years ago
|
Attachment #547714 -
Flags: review?(rcampbell) → review+
Reporter | ||
Updated•13 years ago
|
Attachment #547714 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Priority: -- → P1
Whiteboard: [minotaur] → [minotaur][has-patch]
Comment 6•13 years ago
|
||
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•13 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 8•13 years ago
|
||
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)
Comment 9•13 years ago
|
||
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•13 years ago
|
||
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)
Comment 11•13 years ago
|
||
(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•13 years ago
|
||
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.
Updated•13 years ago
|
Whiteboard: [minotaur][has-patch] → [minotaur][has-patch][needs-update]
Assignee | ||
Comment 13•13 years ago
|
||
I'll be working on this, since Paul has enough on his plate already.
Assignee: paul → past
Assignee | ||
Comment 14•13 years ago
|
||
Rebased, inlined attach/detachPageListeners and fixed a bug probably caused by bitrot.
Attachment #547714 -
Attachment is obsolete: true
Attachment #556881 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 15•13 years ago
|
||
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)
Assignee | ||
Comment 16•13 years ago
|
||
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 17•13 years ago
|
||
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 18•13 years ago
|
||
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);
Assignee | ||
Comment 19•13 years ago
|
||
Thank you both, I changed the listener registration to mimic the one in inspector.jsm, like you suggested.
Attachment #564163 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Whiteboard: [minotaur][has-patch][needs-update] → [minotaur][land-in-fx-team]
Comment 20•13 years ago
|
||
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
Comment 21•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•