Closed Bug 704228 Opened 12 years ago Closed 12 years ago

Keybindings not restored in Highlighter when returning from other tab


(DevTools :: Inspector, defect)

Not set



Firefox 11
Tracking Status
firefox10 - ---


(Reporter: rcampbell, Assigned: paul)



(1 file, 2 obsolete files)


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.
Whiteboard: [good-first-bugs][mentor=robcee][lang=js]
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #575962 - Flags: review?(rcampbell)
Attached patch patch v1.1 - with comments (obsolete) — Splinter Review
Attachment #575973 - Flags: review?(rcampbell)
Attachment #575962 - Attachment is obsolete: true
Attachment #575962 - Flags: review?(rcampbell)
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.
Attachment #575973 - Flags: review?(rcampbell) → review-
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`.
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.
Assignee: nobody → paul
Attached patch patch v1.2Splinter Review
I think this is a better approach
Attachment #576460 - Flags: review?(rcampbell)
Attachment #575973 - Attachment is obsolete: true
Comment on attachment 576460 [details] [diff] [review]
patch v1.2

yeah, that looks much better. Thanks!
Attachment #576460 - Flags: review?(rcampbell) → review+
Whiteboard: [good-first-bugs][mentor=robcee][lang=js] → [land-in-fx-team]
Attachment #576460 - Attachment is patch: true
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 11
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.