749974 - Items in Developer tools menu have inconsistent check state

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Girish Sharma [:Optimizer]]

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]

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]]

Yeah, I basically tested it for Firefox Menu, that is a windows only thing

Comment 3

[Rob Campbell [:rc] (:robcee)]

and presumably on linux as well with the app menu enabled.

Comment 4

[Girish Sharma [:Optimizer]]

Yes, but I cannot confirm this behavior on Linux as I do not use one.

Comment 5

[Paul Rouget [:paul]]

I can reproduce on Linux.

Comment 6

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 7

[Dave Camp (:dcamp)]

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]]

(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]]

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]]

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]]

Attachment: patch v2

Using <command>. Before asking for review, I need to test on Windows and Mac.

Comment 12

[Paul Rouget [:paul]]

Attachment: patch v2.1

windows and mac fix

Comment 13

[Dão Gottwald [:dao]]

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]]

(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]]

(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]]

(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]]

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]]

Attachment: patch v2.2

Comment 19

[Dão Gottwald [:dao]]

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]]

Attachment: patch v2.3

Comment 21

[Paul Rouget [:paul]]

Comment on attachment 625623 [details] [diff] [review]
patch v2.3

Hmm, I didn't see the change in winstripe.

Comment 22

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/f789661fed75

Comment 23

[Ed Morley [:emorley]]

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]]

Attachment: patch v2.4

fixed tests

Comment 25

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/5ddd48e50e8c

Comment 26

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/5ddd48e50e8c

Comment 27

[Girish Sharma [:Optimizer]]

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 (he/him) [:past] (please ni?)]

(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]]

Okay. may be mark that bug blocking this one ?