Remove border and background fallback styling from checkbox.css

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Themes
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: dao, Assigned: geekodour08, Mentored)

Tracking

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

Trunk
mozilla55
All
Windows
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

2 months ago
+++ This bug was initially created as a clone of Bug #1343196 +++

In toolkit/themes/windows/global/checkbox.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

2 months ago
Depends on: 1347113
(Reporter)

Updated

2 months ago
Mentor: dao+bmo@mozilla.com
Keywords: good-first-bug
Whiteboard: [good first bug][lang=css]

Comment 1

2 months ago
How can I get started with all these? How can I fix this bug and where to see the present situation of this?
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 2

2 months ago
(In reply to Saurabh Shubham from comment #1)
> How can I get started with all these?

Have you read https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction or some other guide? Have you built Firefox yet?

> How can I fix this bug and where to see the present situation of this?

You need to open toolkit/themes/windows/global/checkbox.css in the editor of your choice and remove border, background and padding properties from the CSS rules.

To test your changes, build Firefox again and check if checkboxes still look right in these places:

1. bookmark properties dialog (right click on a bookmark and pick "Properties" from the menu)
2. about:preferences
Flags: needinfo?(dao+bmo)
Summary: Remove border and background fallback styling from checkbox.css → Remove border/background/padding fallback styling from checkbox.css
(Reporter)

Comment 3

2 months ago
Actually, remove only border and background stuff -- we'll need to keep padding.
Summary: Remove border/background/padding fallback styling from checkbox.css → Remove border and background fallback styling from checkbox.css
(Assignee)

Comment 4

a month ago
Saurabh are you working on this issue? if not, Dao can it be assigned to me? I made the patch ready but according to patch guide that I read just now I should be assigned the bug to be able to submit one.
Flags: needinfo?(saurabh.friday)
Flags: needinfo?(dao+bmo)
Comment hidden (typo)

Comment 6

a month ago
(In reply to Hrishikesh Barman[:geekodour08] from comment #4)
> Saurabh are you working on this issue? if not, Dao can it be assigned to me?
> I made the patch ready but according to patch guide that I read just now I
> should be assigned the bug to be able to submit one.

Buddy, I am not able to understand how to start with Bugzilla. I am not doing anything on this issue. You may submit your patch. 
Thank you
(Reporter)

Comment 7

a month ago
Alright then...
Assignee: nobody → hrishikeshbman
Flags: needinfo?(saurabh.friday)
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 8

a month ago
Created attachment 8848746 [details] [diff] [review]
fixes1347111.patch

I've attached the patch file, please review it.
Also I did  
| hg export -g -r -1 > fixes1347111.patch |
because hg bzexport was giving some error.
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 9

a month ago
Comment on attachment 8848746 [details] [diff] [review]
fixes1347111.patch

This is a good start but not sufficient -- there are more rules setting border or background properties, please remove them all.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 10

a month ago
thanks for the feedback Dao, I just realized i am making changes to the windows build and checking the changes on my linux machine. I am setting up my windows environment and will test and submit a patch then, otherwise I'll attached a new patch right now in concern to your feedback.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 11

a month ago
Created attachment 8848984 [details] [diff] [review]
fixes1347111_p2.patch

The only difference this has to the last one is that I removed the one line (border: 2px solid;) from .checkbox-check, I'll send again after I actually test it on windows. let me know of what changes should I make.
(Reporter)

Comment 12

a month ago
Comment on attachment 8848984 [details] [diff] [review]
fixes1347111_p2.patch

>--- a/toolkit/themes/windows/global/checkbox.css
>+++ b/toolkit/themes/windows/global/checkbox.css
>@@ -60,16 +60,10 @@ checkbox[disabled="true"]:-moz-system-me
> .checkbox-check {
>   -moz-appearance: checkbox;
>   -moz-box-align: center;
>-  border: 2px solid;
>-  -moz-border-top-colors: ThreeDShadow ThreeDDarkShadow;
>-  -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
>-  -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
>-  -moz-border-left-colors: ThreeDShadow ThreeDDarkShadow;
>   min-width: 13px;
>   min-height: 13px;
>-  background: -moz-Field no-repeat 50% 50%;
> }

There are still more rules involving border / background. For instance, this one:

> checkbox:hover:active > .checkbox-check {
>   background-color: -moz-Dialog;
> }
Flags: needinfo?(dao+bmo)
(Reporter)

Updated

a month ago
Attachment #8848746 - Attachment is obsolete: true
(Assignee)

Comment 13

a month ago
Thanks for reviewing Dao, I'll send it back in few hours was having some trouble setting up my windows build. I am not able to test my changes otherwise.
like about,
> checkbox:hover:active > .checkbox-check {
>   background-color: -moz-Dialog;
> }
I thought it was necessary, because I was not able to test it.
(Assignee)

Comment 14

a month ago
I am not able to build it on windows, (not able to setup my windows environment for some windows error), I'll be updating in a while with a new patch. (Will compare and fix /windows/global/checkbox.css with /linux/global/checkbox.css) Other than that, I was just curious about why "-moz-appereance" was not implemented on the /linux/global/checkbox.css?
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 15

a month ago
(In reply to Hrishikesh Barman[:geekodour08] from comment #14)
> I am not able to build it on windows, (not able to setup my windows
> environment for some windows error), I'll be updating in a while with a new
> patch.

We can use the Try server to build for us. Just upload the patch, I'll trigger the Try build for you, and once finished you can download it.

> (Will compare and fix /windows/global/checkbox.css with
> /linux/global/checkbox.css) Other than that, I was just curious about why
> "-moz-appereance" was not implemented on the /linux/global/checkbox.css?

toolkit/themes/linux/global/checkbox.css does use -moz-appearance as far as I can tell.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 16

a month ago
Created attachment 8850740 [details] [diff] [review]
fixes1347111_p3.patch

Removing these lines does not have any affect on the checkboxes, but I was not able to test it for it's disabled state, but removing these lines makes does not break anything.

Additionally, this behavior is also observed in checkbox.css file of linux, should I modify that too?

Let me know of the changes I should make, and please review it and let me know. 
Thanks!
Attachment #8848984 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 17

a month ago
(In reply to Hrishikesh Barman[:geekodour08] from comment #16)
> Additionally, this behavior is also observed in checkbox.css file of linux,
> should I modify that too?

You're probably looking at an outdated file. Bug 1345432 already took care of Linux. I believe your Windows file is outdated too (it doesn't have bug 1347113's patch). You should update your working directory.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 18

a month ago
Created attachment 8851198 [details] [diff] [review]
fixes1347111_p4.patch

Removed borders and background, this is done using comparing css rules with the linux/global/checkbox.css.
Attachment #8850740 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 19

a month ago
and yes, you were correct I was using outdated files, the updated /linux/global/checkbox.css is already fixed. thanks for pointing that out. :)
(Reporter)

Comment 20

a month ago
Comment on attachment 8851198 [details] [diff] [review]
fixes1347111_p4.patch

Looks good.
Flags: needinfo?(dao+bmo)
Attachment #8851198 - Flags: review+

Comment 21

a month ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ce569804cb
Remove border and background fallback styling from checkbox.css. r=dao
(Reporter)

Updated

a month ago
Blocks: 1350539
Sorry, I had to back this out for failing browser_all_files_referenced.js on Windows:

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb91648b0991d96140e7304a0bba45dd0e271595

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e1ce569804cb812cdee76a5e1a1c1bbaa2295049&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=86413234&repo=mozilla-inbound

05:33:00     INFO - TEST-START | browser/base/content/test/static/browser_all_files_referenced.js
05:33:07     INFO - TEST-INFO | started process screenshot
05:33:07     INFO - TEST-INFO | screenshot: exit 0
05:33:07     INFO - Buffered messages logged at 05:33:00
05:33:07     INFO - Entering test bound checkAllTheFiles
05:33:07     INFO - Buffered messages finished
05:33:07     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 2, expected 0
05:33:07     INFO - Stack trace:
05:33:07     INFO -     chrome://mochikit/content/browser-test.js:test_is:911
05:33:07     INFO -     chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:462
05:33:07     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
05:33:07     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
05:33:07     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
05:33:07     INFO - Not taking screenshot here: see the one that was previously logged
05:33:07     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced chrome file: chrome://global/skin/checkbox/cbox-check-dis.gif - 
05:33:07     INFO - Stack trace:
05:33:07     INFO -     chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:464
05:33:07     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
05:33:07     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
05:33:07     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
05:33:07     INFO - Not taking screenshot here: see the one that was previously logged
05:33:07     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced chrome file: chrome://global/skin/checkbox/cbox-check.gif - 
05:33:07     INFO - Stack trace:
05:33:07     INFO -     chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:464
05:33:07     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
05:33:07     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7

Your css changes removed references to two files and https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js#99 needs to be updated for that. Please attach the new patch. If you have questions, ask them.
Flags: needinfo?(hrishikeshbman)
(Assignee)

Comment 23

a month ago
Does the new patch only needs to remove those two files from (https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js#99) or do I need to put the last changes in checkbox.css and the changes in browser_all_files_referenced.js into a single patch?
Flags: needinfo?(hrishikeshbman) → needinfo?(aryx.bugmail)
(Reporter)

Comment 24

a month ago
To fix the test failure I think you'll need to:

1. Move toolkit/themes/windows/global/checkbox/ to toolkit/themes/faststripe/global/checkbox/. You can use this command:
> hg move toolkit/themes/windows/global/checkbox/. toolkit/themes/faststripe/global/

2. Move the cbox-check.gif and cbox-check-dis.gif entries from toolkit/themes/shared/non-mac.jar.inc.mn to toolkit/themes/faststripe/global/jar.mn.

3. Remove the cbox-check.gif and cbox-check-dis.gif entries from browser_all_files_referenced.js: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js#98-101

You can make these changes together with the checkbox.css changes in one single patch.
(Assignee)

Comment 25

a month ago
Created attachment 8851286 [details] [diff] [review]
fixes1347111_p5.patch

I've made the changes as mentioned.
In line 98, where the 1348529(https://bugzilla.mozilla.org/show_bug.cgi?id=1348529) bug was listed, I removed the lines from there, should I remove the comment too?

>  {file: "chrome://global/skin/arrow/panelarrow-vertical.svg",
>  platforms: ["linux"]},
>  // Bug 1348529
>  // Bug 1348359
>  {file: "chrome://global/skin/dirListing/folder.png", platforms: ["linux"]},

Also in the file, /faststrip/global/jar.mn
I added the checkboxes files relative to that file, I hope I am doing it right.

Please review and test it and let me know.

Another question, how can I run these tests?
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Running_Tests
does not have anything related to running js tests.
Attachment #8851198 - Attachment is obsolete: true
Flags: needinfo?(aryx.bugmail) → needinfo?(dao+bmo)
(Reporter)

Comment 26

a month ago
(In reply to Hrishikesh Barman[:geekodour08] from comment #25)
> Created attachment 8851286 [details] [diff] [review]
> fixes1347111_p5.patch
> 
> I've made the changes as mentioned.
> In line 98, where the
> 1348529(https://bugzilla.mozilla.org/show_bug.cgi?id=1348529) bug was
> listed, I removed the lines from there, should I remove the comment too?

Yes.

> Another question, how can I run these tests?

./mach mochitest browser/base/content/test/static/browser_all_files_referenced.js
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 27

a month ago
Comment on attachment 8851286 [details] [diff] [review]
fixes1347111_p5.patch

>--- a/toolkit/themes/faststripe/global/jar.mn
>+++ b/toolkit/themes/faststripe/global/jar.mn
>@@ -16,3 +16,7 @@ toolkit.jar:
>    skin/classic/global/tabbox.css
>    skin/classic/global/textbox.css
>    skin/classic/global/scrollbars.css                       (xulscrollbars.css)
>+
>+# These are the image files that have been changed from windows
>+   skin/classic/global/checkbox/cbox-check.gif              (./checkbox/cbox-check.gif)
>+   skin/classic/global/checkbox/cbox-check-dis.gif          (./checkbox/cbox-check-dis.gif)

nit: we don't need the blank line and the comment

You can also just write (checkbox/cbox-check.gif) without the leading ./
(Assignee)

Comment 28

a month ago
Created attachment 8851374 [details] [diff] [review]
fixes1347111_p6.patch

Thankyou for the help, I made changes mentioned in above comments also I ran the tests, the unreferenced error was not there. hopefully it resolved the issue but there were other fails in the test. 
Please review and test it again and let me know.
Attachment #8851286 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
(Reporter)

Comment 29

a month ago
Comment on attachment 8851374 [details] [diff] [review]
fixes1347111_p6.patch

looks good!
Flags: needinfo?(dao+bmo)
Attachment #8851374 - Flags: review+

Comment 30

a month ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f205e33898ae
Remove border and background fallback styling from checkbox.css. r=dao

Comment 31

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f205e33898ae
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

a month ago
Blocks: 1348529
You need to log in before you can comment on or make changes to this bug.