Last Comment Bug 690361 - Switching tabs re-enables the highlighter
: Switching tabs re-enables the highlighter
Status: VERIFIED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: 9 Branch
: x86 Mac OS X
: -- normal (vote)
: Firefox 10
Assigned To: Rob Campbell [:rc] (:robcee)
:
Mentors:
: 690804 691566 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-29 08:31 PDT by Dave Camp (:dcamp)
Modified: 2011-10-21 06:09 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
close fix (920 bytes, patch)
2011-09-29 09:00 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
Details | Diff | Splinter Review
[in-fx-team] better close fix (1.61 KB, patch)
2011-09-29 09:43 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
asa: approval‑mozilla‑aurora-
Details | Diff | Splinter Review
[in-fx-team] and a test (6.29 KB, patch)
2011-09-29 12:19 PDT, Rob Campbell [:rc] (:robcee)
dcamp: review+
Details | Diff | Splinter Review

Description Dave Camp (:dcamp) 2011-09-29 08:31:34 PDT
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
Comment 1 Rob Campbell [:rc] (:robcee) 2011-09-29 08:46:20 PDT
regression?

Maybe related: Closing the highlighter after selecting a node and reopening the highlighter opens with that node selected.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-09-29 09:00:56 PDT
Created attachment 563432 [details] [diff] [review]
close fix
Comment 3 Dão Gottwald [:dao] 2011-09-29 09:06:10 PDT
Comment on attachment 563432 [details] [diff] [review]
close fix

Don't you need to remove this event listener?
Comment 4 Mihai Sucan [:msucan] 2011-09-29 09:08:20 PDT
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);
Comment 5 Rob Campbell [:rc] (:robcee) 2011-09-29 09:09:15 PDT
(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.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-09-29 09:12:28 PDT
(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.
Comment 7 Rob Campbell [:rc] (:robcee) 2011-09-29 09:43:20 PDT
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.
Comment 8 Mihai Sucan [:msucan] 2011-09-29 10:33:21 PDT
Comment on attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

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

Looks good!
Comment 9 Dão Gottwald [:dao] 2011-09-29 10:53:06 PDT
Comment on attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

needs no extra review
Comment 10 Rob Campbell [:rc] (:robcee) 2011-09-29 12:19:58 PDT
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.
Comment 11 Dave Camp (:dcamp) 2011-09-29 13:43:56 PDT
Comment on attachment 563517 [details] [diff] [review]
[in-fx-team] and a test

yoink
Comment 12 Panos Astithas [:past] (away until 7/21) 2011-10-01 01:37:35 PDT
*** Bug 690804 has been marked as a duplicate of this bug. ***
Comment 13 Rob Campbell [:rc] (:robcee) 2011-10-03 08:19:35 PDT
Comment on attachment 563441 [details] [diff] [review]
[in-fx-team] better close fix

https://hg.mozilla.org/integration/fx-team/rev/4307d6d3df01
Comment 14 Rob Campbell [:rc] (:robcee) 2011-10-03 08:20:19 PDT
Comment on attachment 563517 [details] [diff] [review]
[in-fx-team] and a test

https://hg.mozilla.org/integration/fx-team/rev/e249ca88cad7
Comment 15 Kevin Dangoor 2011-10-03 19:40:43 PDT
*** Bug 691566 has been marked as a duplicate of this bug. ***
Comment 17 Rob Campbell [:rc] (:robcee) 2011-10-18 12:55:08 PDT
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.
Comment 18 Asa Dotzler [:asa] 2011-10-20 14:34:27 PDT
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.
Comment 19 Teodosia Pop 2011-10-21 06:09:17 PDT
Verified Fixed using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a1) Gecko/20111020 Firefox/10.0a1

Note You need to log in before you can comment on or make changes to this bug.