Last Comment Bug 704228 - Keybindings not restored in Highlighter when returning from other tab
: Keybindings not restored in Highlighter when returning from other tab
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: Firefox 11
Assigned To: Paul Rouget [:paul]
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Depends on:
  Show dependency treegraph
Reported: 2011-11-21 11:27 PST by Rob Campbell [:rc] (:robcee)
Modified: 2012-01-05 13:46 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1 (4.71 KB, patch)
2011-11-21 13:46 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.1 - with comments (4.81 KB, patch)
2011-11-21 14:14 PST, Paul Rouget [:paul]
rcampbell: review-
Details | Diff | Splinter Review
patch v1.2 (3.96 KB, patch)
2011-11-23 05:34 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review

Description User image Rob Campbell [:rc] (:robcee) 2011-11-21 11:27:43 PST

1. In a webpage, open the highlighter and lock a node by clicking on it.
2. Open a new tab.
3. Switch back to tab from step 1.

Expected Results:

Pressing Enter (key) should restart inspection.


Enter does nothing.
Comment 1 User image Paul Rouget [:paul] 2011-11-21 13:46:20 PST
Created attachment 575962 [details] [diff] [review]
patch v1
Comment 2 User image Paul Rouget [:paul] 2011-11-21 14:14:55 PST
Created attachment 575973 [details] [diff] [review]
patch v1.1 - with comments
Comment 3 User image Rob Campbell [:rc] (:robcee) 2011-11-22 12:38:09 PST
Comment on attachment 575973 [details] [diff] [review]
patch v1.1 - with comments

I think we should set the state of "locked" on the highlighter depending on whether InspectorUI.inspecting is true or false. That property is set in start and stopInspecting.

That'll do away with the unnecessary additional storage property.

+    // We start inspecting, even if the node is locked, to initiate the
+    // key event listeners.
+    let locked =, "locked");
+    this.startInspecting();
+    if (locked) {
+      this.stopInspecting();
+    }

maybe rather than do this, you could call startInspecting() in highlighterReady and if !this.inspecting was set (before the startInspecting() call, obviously), call stopInspecting();

Although, that feels kind of messy.

Maybe move the this.browser.addEventListener("keypress", this, true); out of startInspecting into somewhere earlier in the InspectorUI's startup path?

Test looks like it should catch the problem.
Comment 4 User image Paul Rouget [:paul] 2011-11-22 14:47:15 PST
You're right. I have better version coming.

I also realize that we add keypress event listeners on `startInpsecting`, but we don't remove them on `stopInspecting`. Every time we toggle the inspection, we add extra listeners.

I'll just add the listeners in `initializeHighlighter`.
Comment 5 User image Rob Campbell [:rc] (:robcee) 2011-11-23 02:49:59 PST
yeah, that'd be best.

Was also thinking about the location of the "lock" setting. You'll probably want to move them into a method in the Highlighter prototype when you're doing your refactoring.
Comment 6 User image Paul Rouget [:paul] 2011-11-23 05:34:51 PST
Created attachment 576460 [details] [diff] [review]
patch v1.2

I think this is a better approach
Comment 7 User image Rob Campbell [:rc] (:robcee) 2011-11-24 08:36:56 PST
Comment on attachment 576460 [details] [diff] [review]
patch v1.2

yeah, that looks much better. Thanks!
Comment 8 User image Rob Campbell [:rc] (:robcee) 2011-11-29 11:12:36 PST
Comment 9 User image Tim Taubert [:ttaubert] 2011-11-29 22:05:12 PST
Comment 10 User image Alex Keybl [:akeybl] 2012-01-05 13:46:46 PST
Not tracking for FF10 since the user effect does not seem great to a majority of our users (and it's unclear how many keybindings are in the highlighter). That being said, if this is still deemed highly desirable and low risk, please nominate for Beta 10 approval.

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