Keybindings not restored in Highlighter when returning from other tab

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rc, Assigned: paul)

Tracking

unspecified
Firefox 11
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox10-)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Whiteboard: [good-first-bugs][mentor=robcee][lang=js]
(Assignee)

Comment 1

6 years ago
Created attachment 575962 [details] [diff] [review]
patch v1
(Assignee)

Updated

6 years ago
Attachment #575962 - Flags: review?(rcampbell)
(Assignee)

Comment 2

6 years ago
Created attachment 575973 [details] [diff] [review]
patch v1.1 - with comments
(Assignee)

Updated

6 years ago
Attachment #575973 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Attachment #575962 - Attachment is obsolete: true
Attachment #575962 - Flags: review?(rcampbell)
(Reporter)

Comment 3

6 years ago
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.
Attachment #575973 - Flags: review?(rcampbell) → review-
(Assignee)

Comment 4

6 years ago
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`.
(Reporter)

Comment 5

6 years ago
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
Status: NEW → ASSIGNED
tracking-firefox10: --- → ?
(Assignee)

Comment 6

6 years ago
Created attachment 576460 [details] [diff] [review]
patch v1.2

I think this is a better approach
Attachment #576460 - Flags: review?(rcampbell)
(Assignee)

Updated

6 years ago
Attachment #575973 - Attachment is obsolete: true
(Reporter)

Comment 7

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

yeah, that looks much better. Thanks!
Attachment #576460 - Flags: review?(rcampbell) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [good-first-bugs][mentor=robcee][lang=js] → [land-in-fx-team]
(Reporter)

Updated

6 years ago
Attachment #576460 - Attachment is patch: true
(Reporter)

Comment 8

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/1fdc7c54a636
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1fdc7c54a636
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.
tracking-firefox10: ? → -
You need to log in before you can comment on or make changes to this bug.