Last Comment Bug 749974 - Items in Developer tools menu have inconsistent check state
: Items in Developer tools menu have inconsistent check state
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All Windows 7
: -- normal (vote)
: Firefox 15
Assigned To: Paul Rouget [:paul]
:
Mentors:
Depends on: 756759
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-28 12:50 PDT by Girish Sharma [:Optimizer]
Modified: 2012-06-15 00:05 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (3.46 KB, patch)
2012-05-02 23:10 PDT, Paul Rouget [:paul]
dcamp: review+
Details | Diff | Review
patch v2 (10.94 KB, patch)
2012-05-15 05:28 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v2.1 (12.16 KB, patch)
2012-05-16 01:21 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v2.2 (11.43 KB, patch)
2012-05-21 05:28 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review
patch v2.3 (11.43 KB, patch)
2012-05-21 06:23 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Review
patch v2.4 (16.24 KB, patch)
2012-05-25 04:48 PDT, Paul Rouget [:paul]
no flags Details | Diff | Review

Description Girish Sharma [:Optimizer] 2012-04-28 12:50:59 PDT
1) Open any developer tool via any method. Be it Menu entry from Developer tool menu, Shortcut, or context menu.
2) The checkbox for the corresponding item is checked in the developer tools menu
3) Now close the developer tool using a method other than the menu entry under developer tools menu.
4) The checkbox in (2) is still checked. Clicking on it will open that developer tool again.
Comment 1 Kevin Dangoor 2012-04-30 09:14:30 PDT
It works fine for me on the Mac. I tried a variety of tools with a variety of means for opening or closing the tool. Perhaps this is Windows-only?
Comment 2 Girish Sharma [:Optimizer] 2012-04-30 09:18:50 PDT
Yeah, I basically tested it for Firefox Menu, that is a windows only thing
Comment 3 Rob Campbell [:rc] (:robcee) 2012-05-01 09:28:10 PDT
and presumably on linux as well with the app menu enabled.
Comment 4 Girish Sharma [:Optimizer] 2012-05-01 09:34:29 PDT
Yes, but I cannot confirm this behavior on Linux as I do not use one.
Comment 5 Paul Rouget [:paul] 2012-05-01 09:40:21 PDT
I can reproduce on Linux.
Comment 6 Paul Rouget [:paul] 2012-05-02 23:10:08 PDT
Created attachment 620597 [details] [diff] [review]
patch v1
Comment 7 Dave Camp (:dcamp) 2012-05-03 08:37:27 PDT
Comment on attachment 620597 [details] [diff] [review]
patch v1

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

::: browser/devtools/highlighter/inspector.jsm
@@ +464,1 @@
>  

Shouldn't we be toggling the checked state of the underlying <command>?
Comment 8 Paul Rouget [:paul] 2012-05-03 09:13:40 PDT
(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 620597 [details] [diff] [review]
> patch v1
> 
> Review of attachment 620597 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/highlighter/inspector.jsm
> @@ +464,1 @@
> >  
> 
> Shouldn't we be toggling the checked state of the underlying <command>?

I just tried. It doesn't work. When you click on the menuitem, because its type is checkbox, it will get its own "checked" value, that will override the broadcasted "checked" value.
Comment 9 Dão Gottwald [:dao] 2012-05-14 13:45:51 PDT
Comment on attachment 620597 [details] [diff] [review]
patch v1

>+    this.inspectMenuitem.setAttribute("checked", "true");

So why do you keep setting this?

>+    if (this.inspectAppMenuitem) this.inspectAppMenuitem.setAttribute("checked", "true");

new line after if (...)
Comment 10 Paul Rouget [:paul] 2012-05-15 03:53:06 PDT
So I figured out that setting the attribute to false instead of removing the attribute works. I'll switch to a <command> approach (I'll need to update the CSS as we only check the presence of the checked attribute).
Comment 11 Paul Rouget [:paul] 2012-05-15 05:28:08 PDT
Created attachment 624003 [details] [diff] [review]
patch v2

Using <command>. Before asking for review, I need to test on Windows and Mac.
Comment 12 Paul Rouget [:paul] 2012-05-16 01:21:25 PDT
Created attachment 624320 [details] [diff] [review]
patch v2.1

windows and mac fix
Comment 13 Dão Gottwald [:dao] 2012-05-17 07:34:59 PDT
Comment on attachment 624320 [details] [diff] [review]
patch v2.1

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

> .highlighter-nodeinfobar-button {
>   -moz-appearance: none;
>   border: 0 solid hsla(210,8%,5%,.45);
>   padding: 0;
>   width: 26px;
>   min-height: 26px;
>+  background-image: none;
>+}
>+
>+.highlighter-nodeinfobar-button[checked=true] {
>+  background-color: transparent !important;
>+  border-color: hsla(210,8%,5%,.45) !important;
> }

Can you explain this change?
Comment 14 Paul Rouget [:paul] 2012-05-18 02:31:01 PDT
(In reply to Dão Gottwald [:dao] from comment #13)
> Comment on attachment 624320 [details] [diff] [review]
> patch v2.1
> 
> >--- a/browser/themes/gnomestripe/browser.css
> >+++ b/browser/themes/gnomestripe/browser.css
> 
> > .highlighter-nodeinfobar-button {
> >   -moz-appearance: none;
> >   border: 0 solid hsla(210,8%,5%,.45);
> >   padding: 0;
> >   width: 26px;
> >   min-height: 26px;
> >+  background-image: none;
> >+}
> >+
> >+.highlighter-nodeinfobar-button[checked=true] {
> >+  background-color: transparent !important;
> >+  border-color: hsla(210,8%,5%,.45) !important;
> > }
> 
> Can you explain this change?

The Inspect button in the infobar now uses the checked attribute broadcasted by the <command> element,
so this style is also used, and need to be overridden:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/global/toolbarbutton.css#112
Comment 15 Dão Gottwald [:dao] 2012-05-19 03:09:31 PDT
(In reply to Paul Rouget [:paul] from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > Comment on attachment 624320 [details] [diff] [review]
> > patch v2.1
> > 
> > >--- a/browser/themes/gnomestripe/browser.css
> > >+++ b/browser/themes/gnomestripe/browser.css
> > 
> > > .highlighter-nodeinfobar-button {
> > >   -moz-appearance: none;
> > >   border: 0 solid hsla(210,8%,5%,.45);
> > >   padding: 0;
> > >   width: 26px;
> > >   min-height: 26px;
> > >+  background-image: none;
> > >+}
> > >+
> > >+.highlighter-nodeinfobar-button[checked=true] {
> > >+  background-color: transparent !important;
> > >+  border-color: hsla(210,8%,5%,.45) !important;
> > > }
> > 
> > Can you explain this change?
> 
> The Inspect button in the infobar now uses the checked attribute broadcasted
> by the <command> element,
> so this style is also used, and need to be overridden:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/
> global/toolbarbutton.css#112

Judging by <http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/toolbarbutton.css#106>, it looks like the !important flags shouldn't really be needed...
Comment 16 Paul Rouget [:paul] 2012-05-21 05:06:16 PDT
(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to Paul Rouget [:paul] from comment #14)
> > (In reply to Dão Gottwald [:dao] from comment #13)
> > > Comment on attachment 624320 [details] [diff] [review]
> Judging by
> <http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/
> global/toolbarbutton.css#106>, it looks like the !important flags shouldn't
> really be needed...

I don't use the important flag for winstripe. Only for gnomestripe.
Comment 17 Paul Rouget [:paul] 2012-05-21 05:08:45 PDT
Oh, I see (just saw bug 756759). The !important flags shouldn't really be needed in toolkit.
I'll update my patch.
Comment 18 Paul Rouget [:paul] 2012-05-21 05:28:15 PDT
Created attachment 625615 [details] [diff] [review]
patch v2.2
Comment 19 Dão Gottwald [:dao] 2012-05-21 05:47:59 PDT
Comment on attachment 625615 [details] [diff] [review]
patch v2.2

>--- a/browser/themes/winstripe/browser.css
>+++ b/browser/themes/winstripe/browser.css

> .highlighter-nodeinfobar-button {
>   -moz-appearance: none;
>   border: 0 solid hsla(210,8%,5%,.45);
>   padding: 0;
>   width: 26px;
>   min-height: 26px;
>+  background-image: none;
> }

This looks like it needs an update for the patch in bug 756759.
Comment 20 Paul Rouget [:paul] 2012-05-21 06:23:40 PDT
Created attachment 625623 [details] [diff] [review]
patch v2.3
Comment 21 Paul Rouget [:paul] 2012-05-21 06:25:12 PDT
Comment on attachment 625623 [details] [diff] [review]
patch v2.3

Hmm, I didn't see the change in winstripe.
Comment 23 Ed Morley [:emorley] 2012-05-22 09:53:33 PDT
Sorry, backed out for failures in browser_toolbar_basic.js:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=f789661fed75

{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_basic.js | inspector button state 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_basic.js | inspector button state 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_basic.js | inspector button state 7
}

https://hg.mozilla.org/integration/fx-team/rev/512b928b22e7
Comment 24 Paul Rouget [:paul] 2012-05-25 04:48:42 PDT
Created attachment 627183 [details] [diff] [review]
patch v2.4

fixed tests
Comment 26 Rob Campbell [:rc] (:robcee) 2012-05-26 10:28:53 PDT
https://hg.mozilla.org/mozilla-central/rev/5ddd48e50e8c
Comment 27 Girish Sharma [:Optimizer] 2012-06-14 15:02:24 PDT
Can I reopen this bug for Debugger ?
Debugger has the same problem right now.
I open the debugger via Developer tools menu, close it via close button, check still there in developer tools menu.
Comment 28 Panos Astithas [:past] 2012-06-14 23:53:08 PDT
(In reply to Girish Sharma [:Optimizer] from comment #27)
> Can I reopen this bug for Debugger ?
> Debugger has the same problem right now.
> I open the debugger via Developer tools menu, close it via close button,
> check still there in developer tools menu.

There is already bug 760839 for that.
Comment 29 Girish Sharma [:Optimizer] 2012-06-15 00:05:59 PDT
Okay. may be mark that bug blocking this one ?

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