Remove border and background fallback styling from checkbox.css

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dao, Assigned: geekodour08, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla55
All
Windows
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 years 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 years ago
Depends on: 1347113
(Reporter)

Updated

2 years ago
Mentor: dao+bmo
Keywords: good-first-bug
Whiteboard: [good first bug][lang=css]

Comment 1

2 years 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 years 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 years 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

2 years 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

2 years 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

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

Comment 8

2 years ago
Posted patch fixes1347111.patch (obsolete) — Splinter Review
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

2 years 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

2 years 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

2 years ago
Posted patch fixes1347111_p2.patch (obsolete) — Splinter Review
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

2 years 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

2 years ago
Attachment #8848746 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years 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

2 years 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

2 years 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

2 years ago
Posted patch fixes1347111_p3.patch (obsolete) — Splinter Review
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

2 years 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

2 years ago
Posted patch fixes1347111_p4.patch (obsolete) — Splinter Review
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

2 years 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

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

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

Comment 21

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Posted patch fixes1347111_p5.patch (obsolete) — Splinter Review
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

2 years 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

2 years 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

2 years ago
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

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

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

Comment 30

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f205e33898ae
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

2 years ago
Blocks: 1348529
(Reporter)

Updated

2 years ago
See Also: → 1411640
You need to log in before you can comment on or make changes to this bug.