magicp Reporter
	Description • 8 years ago

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

Steps to reproduce:

1. Start Nightly (or Aurora)
2. Open DevTools > Toolbox Options
3. Switch theme to Firebug
4. See checkbox "Select an iframe as the currently targeted document"


Actual results:

#command-button-frames checkbox has unnecessary min-width.

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


Expected results:

Don't apply min-width to the command-button-frames checkbox.

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 1 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/63066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63066/

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 2 • 8 years ago

There is a naming conflict in command-button-frames between checkbox and toolbox icon. The quickest way to fix this is that remove .theme-firebug #command-button-frames rule and it's fine that won't affect to toolbox icon.

I know naming conflict is good but it would take more time to fix these conflict part as well as test failures. I'd like to hear your option for this patch :)

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 3 • 8 years ago

(In reply to Ricky Chien [:rickychien] from comment #2)
> There is a naming conflict in command-button-frames between checkbox and
> toolbox icon. The quickest way to fix this is that remove .theme-firebug
> #command-button-frames rule and it's fine that won't affect to toolbox icon.
> 
> I know naming conflict is good but it would take more time to fix these
> conflict part as well as test failures. I'd like to hear your option for
> this patch :)

Thanks for working on this!

The Frame list button is broken when I apply this patch (see the attached screenshot, the drop down arrow now overlaps with the button icon).

Could you try changing the CSS selector instead of removing it so, it applies only on the button in the toolbar? Something like: button#command-button-frames ?

This should be done for all selectors that are applied on the frame toolbar button to make sure they don't leak to the options panel.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 4 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

https://reviewboard.mozilla.org/r/63066/#review60016

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 5 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63066/diff/1-2/

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 6 • 8 years ago

Good point! I also noticed there is another unexpected style from toolbars.css which applied accidentally to #command-button-frames. Therefore, I modified devtools/client/themes/toolbars.css to fix this issue.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 7 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

https://reviewboard.mozilla.org/r/63066/#review60244

Great!

One little thing, can you use `#toolbox-buttons` instead of `toolbar.devtools-tabbar` in the CSS Selector. Just like you did in firebug-theme.css file. This makes it a bit more clear that we are targeting toolbox-buttons here.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 8 • 8 years ago

(discussed on IRC) `#command-button-pick` doesn't belong to #toolbox-buttons, so using `toolbar.devtools-tabbar` selector is actually is better. So, I think the patch is ready.

---

Ntim: what do you think about the patch any comments?

Honza

	Tim Nguyen :ntim
	Comment 9 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> (discussed on IRC) `#command-button-pick` doesn't belong to
> #toolbox-buttons, so using `toolbar.devtools-tabbar` selector is actually is
> better. So, I think the patch is ready.
> 
> ---
> 
> Ntim: what do you think about the patch any comments?
> 
> Honza

Right now, it seems like a bug that every panel loads all the toolbox related CSS. I think a better solution would be splitting out the command button related css (and all other toolbox related css) into a new file (called toolbox.css ?) and loading it only in toolbox.xul. It would prevent similar issues from happening in the future without introducing complicated CSS selectors.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 10 • 8 years ago

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #9)
> Right now, it seems like a bug that every panel loads all the toolbox
> related CSS. I think a better solution would be splitting out the command
> button related css (and all other toolbox related css) into a new file
> (called toolbox.css ?) and loading it only in toolbox.xul. It would prevent
> similar issues from happening in the future without introducing complicated
> CSS selectors.

Sounds good to me. Ricky, can you look into it? 
* toolbox.css file name sounds nice
* It think it should also be inserted into: devtools\client\jar.mn
* and included in toolbox.xul

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 11 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

https://reviewboard.mozilla.org/r/63066/#review60304

I am removing myself from the review waiting for the toolbox.css file.

Honza

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 12 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63066/diff/2-3/

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 13 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63066/diff/3-4/

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 14 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

The patch works for me (fixes the reported issue), waiting for feedback.

Honza

	Tim Nguyen :ntim
	Comment 15 • 8 years ago

https://reviewboard.mozilla.org/r/63066/#review60344

::: devtools/client/themes/firebug-theme.css:247
(Diff revision 4)
>  
>  .theme-firebug .devtools-toolbarbutton {
>    min-width: 24px;
>  }
>  
> -.theme-firebug #command-button-frames {
> +.theme-firebug #toolbox-buttons #command-button-frames {

Can you move this rule at the end of toolbox.css and remove #toolbox-buttons ?

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 16 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63066/diff/4-5/

	Ricky Chien [:rickychien] (inactive) Assignee
	Comment 17 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

Don't know how I cancel feedback+ unexpectedly after updating my mozreview.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 18 • 8 years ago

Comment on attachment 8769082 [details]
Bug 1280791 - Don't apply min-width to the command-button-frames checkbox in firebug theme

https://reviewboard.mozilla.org/r/63066/#review60594

Lookg great, thanks Ricky!

Honza

	Pulsebot
	Comment 19 • 8 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/c492812c7932
Don't apply min-width to the command-button-frames checkbox in firebug theme r=Honza

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago
bugherder

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

	Ada [:adalucinet]
	Comment 21 • 8 years ago

Reproduced with 50.0a1 from 2016-07-13 under Windows 10 64-bit
Verified fixed with latest Nightly 50.0a1, under Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.9.5