Last Comment Bug 704228 - Keybindings not restored in Highlighter when returning from other tab
: Keybindings not restored in Highlighter when returning from other tab
Status: RESOLVED FIXED
:
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]
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
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 Rob Campbell [:rc] (:robcee) 2011-11-21 11:27:43 PST
STR:

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.

Actual: 

Enter does nothing.
Comment 1 Paul Rouget [:paul] 2011-11-21 13:46:20 PST
Created attachment 575962 [details] [diff] [review]
patch v1
Comment 2 Paul Rouget [:paul] 2011-11-21 14:14:55 PST
Created attachment 575973 [details] [diff] [review]
patch v1.1 - with comments
Comment 3 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 = this.store.getValue(aWinID, "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 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 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 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 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 Rob Campbell [:rc] (:robcee) 2011-11-29 11:12:36 PST
https://hg.mozilla.org/integration/fx-team/rev/1fdc7c54a636
Comment 9 Tim Taubert [:ttaubert] 2011-11-29 22:05:12 PST
https://hg.mozilla.org/mozilla-central/rev/1fdc7c54a636
Comment 10 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.