Closed
Bug 1280791
Opened 8 years ago
Closed 8 years ago
Don't apply min-width to the command-button-frames checkbox in firebug theme
Categories
(DevTools :: General, defect, P1)
DevTools
General
Tracking
(firefox48 unaffected, firefox49 affected, firefox50 verified)
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
status-firefox48:
--- → unaffected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
Component: Untriaged → Developer Tools
OS: Unspecified → All
Hardware: Unspecified → All
Updated•8 years ago
|
Blocks: devtools-html-2
Whiteboard: [devtools-html] [triage]
Updated•8 years ago
|
Flags: qe-verify+
Priority: -- → P2
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
Updated•8 years ago
|
Blocks: improve-fb-theme
Updated•8 years ago
|
Assignee: nobody → rchien
Updated•8 years ago
|
Status: NEW → ASSIGNED
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/
Attachment #8769082 -
Flags: review?(odvarko)
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 :)
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
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
Attachment #8769082 -
Flags: review?(odvarko)
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/
Attachment #8769082 -
Flags: review?(odvarko)
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.
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
Attachment #8769082 -
Flags: review?(odvarko)
Assignee | ||
Updated•8 years ago
|
Attachment #8769082 -
Flags: review?(odvarko)
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
Flags: needinfo?(ntim.bugs)
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.
Flags: needinfo?(ntim.bugs)
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
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
Attachment #8769082 -
Flags: review?(odvarko)
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/
Attachment #8769082 -
Flags: review?(odvarko)
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/
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
Attachment #8769082 -
Flags: feedback?(ntim.bugs)
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 ?
Updated•8 years ago
|
Attachment #8769082 -
Flags: feedback?(ntim.bugs) → feedback+
Updated•8 years ago
|
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
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/
Attachment #8769082 -
Flags: feedback+
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.
Attachment #8769082 -
Flags: feedback+
Updated•8 years ago
|
Attachment #8769082 -
Flags: review?(odvarko) → review+
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
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
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
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•