1347111 - Remove border and background fallback styling from checkbox.css

Status: RESOLVED FIXED

(Toolkit :: Themes, enhancement)

Description

[Dão Gottwald [:dao]]

+++ 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.

Comment 1

[Saurabh Shubham]

How can I get started with all these? How can I fix this bug and where to see the present situation of this?

Comment 2

[Dão Gottwald [:dao]]

(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

Comment 3

[Dão Gottwald [:dao]]

Actually, remove only border and background stuff -- we'll need to keep padding.

Comment 4

[Hrishikesh Barman[:geekodour08]]

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.

Comment 5

[Saurabh Shubham]

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

Comment 6

[Saurabh Shubham]

(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

Comment 7

[Dão Gottwald [:dao]]

Alright then...

Comment 8

[Hrishikesh Barman[:geekodour08]]

Attachment: 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.

Comment 9

[Dão Gottwald [:dao]]

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.

Comment 10

[Hrishikesh Barman[:geekodour08]]

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.

Comment 11

[Hrishikesh Barman[:geekodour08]]

Attachment: 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.

Comment 12

[Dão Gottwald [:dao]]

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;
> }

Comment 13

[Hrishikesh Barman[:geekodour08]]

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.

Comment 14

[Hrishikesh Barman[:geekodour08]]

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?

Comment 15

[Dão Gottwald [:dao]]

(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.

Comment 16

[Hrishikesh Barman[:geekodour08]]

Attachment: 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!

Comment 17

[Dão Gottwald [:dao]]

(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.

Comment 18

[Hrishikesh Barman[:geekodour08]]

Attachment: fixes1347111_p4.patch

Removed borders and background, this is done using comparing css rules with the linux/global/checkbox.css.

Comment 19

[Hrishikesh Barman[:geekodour08]]

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

Comment 20

[Dão Gottwald [:dao]]

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

Looks good.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ce569804cb
Remove border and background fallback styling from checkbox.css. r=dao

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 23

[Hrishikesh Barman[:geekodour08]]

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?

Comment 24

[Dão Gottwald [:dao]]

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.

Comment 25

[Hrishikesh Barman[:geekodour08]]

Attachment: 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.

Comment 26

[Dão Gottwald [:dao]]

(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

Comment 27

[Dão Gottwald [:dao]]

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 ./

Comment 28

[Hrishikesh Barman[:geekodour08]]

Attachment: 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.

Comment 29

[Dão Gottwald [:dao]]

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

looks good!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f205e33898ae
Remove border and background fallback styling from checkbox.css. r=dao

Comment 31

[Carsten Book [:Tomcat]]

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