Closed Bug 1280791 Opened 4 years ago Closed 4 years ago

Don't apply min-width to the command-button-frames checkbox in firebug theme

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(firefox48 unaffected, firefox49 affected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox48 --- unaffected
firefox49 --- affected
firefox50 --- verified

People

(Reporter: magicp.jp, Assigned: rickychien)

References

Details

(Whiteboard: [devtools-html])

Attachments

(3 files)

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.
Blocks: 1266419
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [devtools-html] [triage]
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
Assignee: nobody → rchien
Status: NEW → ASSIGNED
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 :)
Attached image bug1280791.png
(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
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
Attachment #8769082 - Flags: review?(odvarko)
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/
Attachment #8769082 - Flags: review?(odvarko)
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.
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
Attachment #8769082 - Flags: review?(odvarko)
Attachment #8769082 - Flags: review?(odvarko)
(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
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
(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
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
Attachment #8769082 - Flags: review?(odvarko)
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/
Attachment #8769082 - Flags: review?(odvarko)
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/
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
Attachment #8769082 - Flags: feedback?(ntim.bugs)
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 ?
Attachment #8769082 - Flags: feedback?(ntim.bugs) → feedback+
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
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/
Attachment #8769082 - Flags: feedback+
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.
Attachment #8769082 - Flags: feedback+
Attachment #8769082 - Flags: review?(odvarko) → review+
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
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c492812c7932
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.