Remove border and background fallback styling from toolbar.css

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Themes
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: dao, Assigned: Barun Parruck, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

Trunk
mozilla54
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [good first bug][lang=css])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 months ago
In toolkit/themes/windows/global/toolbar.css, toolkit/themes/osx/global/toolbar.css and toolkit/themes/linux/global/toolbar.css, we can remove border and background declarations from rules that also set -moz-appearance, since -moz-appearance causes these borders and backgrounds not to be used anyway.
(Reporter)

Updated

5 months ago
status-firefox54: affected → ---
Comment hidden (mozreview-request)
(Reporter)

Comment 2

5 months ago
mozreview-review
Comment on attachment 8842563 [details]
Bug 1343196 - Removes some redundant background and border declarations .

https://reviewboard.mozilla.org/r/116350/#review118030

::: toolkit/themes/windows/global/toolbar.css:15
(Diff revision 1)
>  
>  toolbox {
>    -moz-appearance: toolbox;
> -  background-color: -moz-Dialog;
> -  border-top: 2px solid;
>    -moz-border-top-colors: ThreeDShadow ThreeDHighlight;

-moz-border-* declarations can be removed too.

::: toolkit/themes/windows/global/toolbar.css:28
(Diff revision 1)
>  
>  toolbar {
>    min-width: 1px;
>    min-height: 19px;
>    border-top: 1px solid ThreeDHighlight;
>    border-bottom: 1px solid ThreeDShadow;

This isn't used either.

::: toolkit/themes/windows/global/toolbar.css:34
(Diff revision 1)
>  }
>  
>  toolbar:first-child, menubar {
>    min-width: 1px;
>    border-bottom: 1px solid ThreeDShadow;
>    border-top: 0px !important;

ditto
Attachment #8842563 - Flags: review-
(Reporter)

Comment 3

5 months ago
Otherwise this patch looks good.
Assignee: nobody → barun.parruck
Comment hidden (mozreview-request)
(Assignee)

Comment 5

5 months ago
Added in the recommended changes :)
(Reporter)

Comment 6

5 months ago
You seem to have committed the previous patch and this new one is against that commit. Instead we'll need all changes in one patch (diff against mozilla-central tip rather than on top of a local commit).
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8842750 - Attachment is obsolete: true
(Assignee)

Comment 8

5 months ago
I think it's okay now :)
(Assignee)

Updated

5 months ago
Attachment #8842750 - Attachment is obsolete: false
Attachment #8842750 - Attachment is patch: true
Attachment #8842750 - Attachment mime type: text/x-review-board-request → text/plain
(Assignee)

Comment 9

5 months ago
Comment on attachment 8842750 [details]
Removes further redundant border declarations

>https://reviewboard.mozilla.org/r/116512/diff/#index_header
Attachment #8842750 - Attachment is obsolete: true
Attachment #8842750 - Attachment is patch: false
(Assignee)

Comment 10

5 months ago
Why has my review request been discarded?
Comment hidden (mozreview-request)
(Reporter)

Comment 12

5 months ago
(In reply to Barun Parruck from comment #10)
> Why has my review request been discarded?

Likely because you didn't put "r?dao" in the commit message?
(Reporter)

Comment 13

5 months ago
mozreview-review
Comment on attachment 8842563 [details]
Bug 1343196 - Removes some redundant background and border declarations .

https://reviewboard.mozilla.org/r/116350/#review118228

::: toolkit/themes/windows/global/toolbar.css:39
(Diff revision 2)
>  menubar:-moz-lwtheme,
>  toolbox:-moz-lwtheme,
>  toolbar:-moz-lwtheme {
>    -moz-appearance: none;
>    background: none;
>    border-color: transparent;

I didn't see this earlier, but the above two lines can now be removed too.
Attachment #8842563 - Flags: review-
(Assignee)

Comment 14

5 months ago
Got it, I'm still learning the process :)
Comment hidden (mozreview-request)
(Reporter)

Comment 16

5 months ago
mozreview-review
Comment on attachment 8842563 [details]
Bug 1343196 - Removes some redundant background and border declarations .

https://reviewboard.mozilla.org/r/116350/#review118230

Looks good!
Attachment #8842563 - Flags: review?(dao+bmo) → review+

Comment 17

5 months ago
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 2 changes to 2 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev ee1fdb0b6fb3 needs "Bug N" or "No bug" in the commit message.
remote: Barun Parruck <barun.parruck@gmail.com>
remote: Bug #1343196 : Removes some redundant background and border declarations r=dao
remote: 
remote: MozReview-Commit-ID: Fiq04L00lBQ
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
(Reporter)

Comment 18

5 months ago
Can you please remove "#" from the commit message, replace ":" with "-" and add a dot or comma at the end before the review tag? Thanks!
Comment hidden (mozreview-request)
(Assignee)

Comment 20

5 months ago
Done :D
(Assignee)

Comment 21

5 months ago
Thank you so much for helping me work through this!

Comment 22

5 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2492756574ea
Removes some redundant background and border declarations . r=dao

Comment 23

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2492756574ea
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Reporter)

Updated

5 months ago
Blocks: 1344189
You need to log in before you can comment on or make changes to this bug.