Closed Bug 1277199 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: General, enhancement, P1)

enhancement

Tracking

(firefox49- verified, firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.1 - Jun 20
Tracking Status
firefox49 - verified
firefox50 --- verified

People

(Reporter: magicp.jp, Assigned: Honza)

References

Details

(Keywords: regression, Whiteboard: [devtools-html])

Attachments

(2 files, 1 obsolete file)

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.
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools
The regression window points to bug 1266419.  Honza, can you take a look?
Blocks: 1266419
Flags: needinfo?(odvarko)
Priority: -- → P1
Priority: P1 → --
Whiteboard: [devtools-html] [triage]
Severity: normal → enhancement
Attached patch bug1277199.patch (obsolete) — Splinter Review
Patch attached.

@magicp: thanks for the report!

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Attachment #8761162 - Flags: review?(jryans)
Iteration: --- → 50.1
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
Comment on attachment 8761162 [details] [diff] [review]
bug1277199.patch

Seems like :ochameau and / or :ntim have more context about this from bug 1266419.
Attachment #8761162 - Flags: review?(poirot.alex)
Attachment #8761162 - Flags: review?(ntim.bugs)
Attachment #8761162 - Flags: review?(jryans)
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 ?
Attachment #8761162 - Flags: review?(ntim.bugs) → review+
Comment on attachment 8761162 [details] [diff] [review]
bug1277199.patch

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

Thanks for fixing that.
Attachment #8761162 - Flags: review?(poirot.alex) → review+
Attached patch bug1277199.patchSplinter Review
(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
Attachment #8761162 - Attachment is obsolete: true
Attachment #8761536 - Flags: review+
Try's green, checkin needed.

Honza
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/ba42259dc467
Fix CSS for command-button-frames; r=ochameau, ntim
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba42259dc467
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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.
Status: RESOLVED → VERIFIED
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
Attachment #8761536 - Flags: approval-mozilla-aurora?
Comment on attachment 8761536 [details] [diff] [review]
bug1277199.patch

Fix for regression from 49, please uplift to aurora.
Attachment #8761536 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.