The default bug view has changed. See this FAQ.

Switching tabs re-enables the highlighter

VERIFIED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: dcamp, Assigned: rc)

Tracking

9 Branch
Firefox 10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

6 years ago
regression?

Maybe related: Closing the highlighter after selecting a node and reopening the highlighter opens with that node selected.
(Assignee)

Comment 2

6 years ago
Created attachment 563432 [details] [diff] [review]
close fix
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+
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

6 years ago
Created attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

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)
(Assignee)

Comment 10

6 years ago
Created attachment 563517 [details] [diff] [review]
[in-fx-team] and a test

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)
(Reporter)

Comment 11

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

Updated

6 years ago
Whiteboard: [land-in-fx-team]
(Assignee)

Updated

6 years ago
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
(Assignee)

Comment 13

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

Comment 14

6 years ago
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

Updated

6 years ago
Duplicate of this bug: 691566
(Assignee)

Comment 16

6 years ago
https://hg.mozilla.org/mozilla-central/rev/4307d6d3df01

https://hg.mozilla.org/mozilla-central/rev/e249ca88cad7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
(Assignee)

Comment 17

6 years ago
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?
(Assignee)

Updated

6 years ago
Attachment #563441 - Flags: approval-mozilla-beta?

Comment 18

6 years ago
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-

Comment 19

6 years ago
Verified Fixed using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a1) Gecko/20111020 Firefox/10.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.