Closed Bug 690361 Opened 8 years ago Closed 8 years ago

Switching tabs re-enables the highlighter

Categories

(DevTools :: General, defect)

9 Branch
x86
macOS
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 10

People

(Reporter: dcamp, Assigned: rcampbell)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 files, 1 obsolete file)

1. Highlight a node
2. Use the X to close the highlighter
3. Switch to a different tab
4. Switch back to the original tab
5. The highlighter has been re-enabled
regression?

Maybe related: Closing the highlighter after selecting a node and reopening the highlighter opens with that node selected.
Attached patch close fix (obsolete) — Splinter Review
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attachment #563432 - Flags: review?(mihai.sucan)
Comment on attachment 563432 [details] [diff] [review]
close fix

Don't you need to remove this event listener?
Comment on attachment 563432 [details] [diff] [review]
close fix

Review of attachment 563432 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks fine!

Shouldn't we have a test for this? It's a regression we should have caught during our work.

::: browser/devtools/highlighter/inspector.jsm
@@ +292,5 @@
>  
> +    let iui = this.IUI;
> +    closeButton.addEventListener("click", function() {
> +      iui.closeInspectorUI(false);
> +    }, false);

Wouldn't the following work?

closeButton.addEventListener("click",
  this.IUI.closeInspectorUI.bind(this.IUI, false),
  false);
Attachment #563432 - Flags: review?(mihai.sucan) → review+
(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 563432 [details] [diff] [review] [diff] [details] [review]
> close fix
> 
> Don't you need to remove this event listener?

Not sure. The node gets destroyed during cleanup, shouldn't it get automagically taken care of? If not I can cache the method somewhere.
(In reply to Mihai Sucan [:msucan] from comment #4)
> ::: browser/devtools/highlighter/inspector.jsm
> @@ +292,5 @@
> >  
> > +    let iui = this.IUI;
> > +    closeButton.addEventListener("click", function() {
> > +      iui.closeInspectorUI(false);
> > +    }, false);
> 
> Wouldn't the following work?
> 
> closeButton.addEventListener("click",
>   this.IUI.closeInspectorUI.bind(this.IUI, false),
>   false);

it should. I'm a little unclear on the preference of closure vs. use of bind though. If removing the listener is an actual problem, I'll probably use the binding approach and cache the method on the highlighter for later removal.
hopefully improved close fix that removes the listener and explicitly removes the close button from the DOM.
Attachment #563432 - Attachment is obsolete: true
Attachment #563441 - Flags: review?(mihai.sucan)
Attachment #563441 - Flags: review?(dao)
Comment on attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

Review of attachment 563441 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #563441 - Flags: review?(mihai.sucan) → review+
Comment on attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

needs no extra review
Attachment #563441 - Flags: review?(dao)
here's some test code to go along with this. Verified it fails without the close button fix and passes with it.
Attachment #563517 - Flags: review?(mihai.sucan)
Comment on attachment 563517 [details] [diff] [review]
[in-fx-team] and a test

yoink
Attachment #563517 - Flags: review?(mihai.sucan) → review+
Duplicate of this bug: 690804
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment on attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

https://hg.mozilla.org/integration/fx-team/rev/4307d6d3df01
Attachment #563441 - Attachment description: better close fix → [in-fx-team] better close fix
Comment on attachment 563517 [details] [diff] [review]
[in-fx-team] and a test

https://hg.mozilla.org/integration/fx-team/rev/e249ca88cad7
Attachment #563517 - Attachment description: and a test → [in-fx-team] and a test
Duplicate of this bug: 691566
https://hg.mozilla.org/mozilla-central/rev/4307d6d3df01

https://hg.mozilla.org/mozilla-central/rev/e249ca88cad7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Comment on attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

bug 695433 was opened because of this bug on beta. Because of the attention the highlighter's been receiving, it's expected that more people will turn this pref on. I'd like to try landing this on aurora and beta to at least fix this annoying bug.
Attachment #563441 - Flags: approval-mozilla-beta?
Attachment #563441 - Flags: approval-mozilla-aurora?
Attachment #563441 - Flags: approval-mozilla-beta?
Comment on attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

If I understand this, the feature is off by default because it's not ready yet. This is one more not ready issue, but I don't think we need to worry since the feature is disabled.
Attachment #563441 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Verified Fixed using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a1) Gecko/20111020 Firefox/10.0a1
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.