1277199 - The command-button-frames in toolbox-buttons does not have the open attribute

Status: VERIFIED FIXED

(DevTools :: General, enhancement, P1)

Description

[magicp]

Attachment: checked-icon-state-is-not-enabled.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160531030258

Steps to reproduce:

1. Start latest Nightly
2. Go to any sites (e.g. https://bugzilla.mozilla.org)
3. Open DevTools
4. Click command-button-frames button (Select an iframe as the currently targeted document)
5. Confirm its icon color


Actual results:

command-button-frames icon does not switch to the opening state color (highlight-blue), because it does not have the open attribute.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=3a87296fe4145138c2ce15512bb31f76fe869cb4&tochange=42fab251fe111d5f891c9bde0ee1fb6f7f946a50


Expected results:

command-button-frames should have the open attribute.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The regression window points to bug 1266419.  Honza, can you take a look?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1277199.patch

Patch attached.

@magicp: thanks for the report!

Honza

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8761162 [details] [diff] [review]
bug1277199.patch

Seems like :ochameau and / or :ntim have more context about this from bug 1266419.

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8761162 [details] [diff] [review]
bug1277199.patch

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

The JS changes look fine. May be worth a test ?

::: devtools/client/themes/toolbars.css
@@ +798,5 @@
> +/* Use checked-icon state when the associated drop down menu is opened. */
> +#command-button-frames[open]::before {
> +  opacity: 1;
> +  filter: url(images/filters.svg#checked-icon-state);
> +}

I think this CSS isn't needed since it's already handled here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#298, can you remove it ?

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8761162 [details] [diff] [review]
bug1277199.patch

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

Thanks for fixing that.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1277199.patch

(In reply to Tim Nguyen :ntim from comment #4)
> The JS changes look fine. May be worth a test ?
Yes, since this is so little feature I extended an existing test.

> ::: devtools/client/themes/toolbars.css
> @@ +798,5 @@
> > +/* Use checked-icon state when the associated drop down menu is opened. */
> > +#command-button-frames[open]::before {
> > +  opacity: 1;
> > +  filter: url(images/filters.svg#checked-icon-state);
> > +}
> 
> I think this CSS isn't needed since it's already handled here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/
> toolbars.css#298, can you remove it ?
True, done.

Thanks!

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd08bd9da646

Honza

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try's green, checkin needed.

Honza

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ba42259dc467
Fix CSS for command-button-frames; r=ochameau, ntim

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba42259dc467

Comment 11

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I managed to test this bug on 50.0a1 (2016-06-13)
using 
- Windows 10 x64
- Mac OS X 10.11
- Ubuntu 14.04 x86
The fix has landed and it works as expected, so I am marking this issue as Verified Fixed.

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8761536 [details] [diff] [review]
bug1277199.patch

Approval Request Comment
[Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=1266419
[User impact if declined]: see first comment
[Describe test coverage new/current, TreeHerder]: in nightly
[Risks and why]: low, small js fix with test
[String/UUID change made/needed]: no

Comment 13

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8761536 [details] [diff] [review]
bug1277199.patch

Fix for regression from 49, please uplift to aurora.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0260659187b2

Comment 15

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I tested again this bug using 49.0a2 (2016-07-03)on 
- Windows 10 x64
- Mac OS X 10.11
- Ubuntu 14.04 x86
This build is Verified Fixed.