704228 - Keybindings not restored in Highlighter when returning from other tab

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

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

Attachment: patch v1

Comment 2

[Paul Rouget [:paul]]

Attachment: patch v1.1 - with comments

Comment 3

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

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

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

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

Attachment: patch v1.2

I think this is a better approach

Comment 7

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

Comment on attachment 576460 [details] [diff] [review]
patch v1.2

yeah, that looks much better. Thanks!

Comment 8

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

https://hg.mozilla.org/integration/fx-team/rev/1fdc7c54a636

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1fdc7c54a636

Comment 10

[Alex Keybl [:akeybl]]

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.