Closed Bug 1136671 Opened 5 years ago Closed 5 years ago

InContent prefs - Tree sort arrows should be darker

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: ntim, Assigned: gioyik, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 4 obsolete files)

Maybe dropdown.svg can be used (except it needs to be backwards). Also, that asset is high contrast friendly.
Mentor: ntim007
Whiteboard: [good first bug][lang=css]
To fix this bug, you need to remove sorter.png and sorter@2x.png from [0]. And in [1], you need to replace sorter.png with dropdown.svg and remove 2dppx rule later in the file (line 702). On line 698, you'll need to replace [sortDirection="descending"] with [sortDirection="ascending"]

[0] : http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/
[1] : http://hg.mozilla.org/mozilla-central/file/460076a2e344/toolkit/themes/shared/in-content/common.inc.css#l693
Attached patch 1136671.patch (obsolete) — Splinter Review
Patch v1
Attachment #8569394 - Flags: review?(ntim007)
Hi Tim, I created a patch for this, could you review it and tell me if is ok?
Comment on attachment 8569394 [details] [diff] [review]
1136671.patch

Review of attachment 8569394 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch !

Whoops, I forgot to mention you had to remove the references to sorter.png and sorter@2x.png in the toolkit/themes/(windows|osx|linux)/global/jar.mn

Other than that, the changes look good. Btw, as I mentioned in the other bug you took, I'm not a peer, so my review is worth nothing (but I can still give feedback), you should ask :jaws for your next review :)
Attachment #8569394 - Flags: review?(ntim007) → feedback+
Attached patch 1136671.patch (obsolete) — Splinter Review
Patch v2
Attachment #8569394 - Attachment is obsolete: true
Attachment #8569407 - Flags: review?(jaws)
Thanks for the suggestions Tim, I didn't remember that were the person who says me that before, thank you. Now I attached a new patch with the modifications and review flag requested to the right person.
Comment on attachment 8569407 [details] [diff] [review]
1136671.patch

Review of attachment 8569407 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the new patch :)

Usually bugs are mentored by peers, so you can directly request review to the mentor, but there are some exceptions ;)

::: toolkit/themes/windows/global/jar.mn
@@ -179,5 @@
>          skin/classic/global/in-content/dropdown.svg              (../../shared/in-content/dropdown.svg)
>          skin/classic/global/in-content/help-glyph.svg            (../../shared/in-content/help-glyph.svg)
>          skin/classic/global/in-content/radio.svg                 (../../shared/in-content/radio.svg)
> -        skin/classic/global/in-content/sorter.png                (../../shared/in-content/sorter.png)
> -        skin/classic/global/in-content/sorter@2x.png             (../../shared/in-content/sorter@2x.png)

There are some references for aero later in the file that you need to remove as well.
Attached patch 1136671.patch (obsolete) — Splinter Review
Patch v3
Attachment #8569407 - Attachment is obsolete: true
Attachment #8569407 - Flags: review?(jaws)
Attachment #8569435 - Flags: review?(jaws)
Thanks for the feedback Tim, I didn't see those lines. Now the patch is updated.
Comment on attachment 8569435 [details] [diff] [review]
1136671.patch

Review of attachment 8569435 [details] [diff] [review]:
-----------------------------------------------------------------

Were you able to test that your changes worked before requesting review? Please test your changes and ask questions that you have while working on it. You can join #introduction on irc.mozilla.org to ask questions about getting your build running, or you can follow the steps at https://developer.mozilla.org/en-US/docs/Simple_Firefox_build to build Firefox.

With this patch there are no arrows in the Preferences > Applications > richlistbox. This is because the selector needs to reference chrome://global/skin/in-content/dropdown.svg#dropdown instead of just chrome://global/skin/in-content/dropdown.svg.

Also, the size of the arrow is too small at height:8px, width:12px. The SVG here is square, and we can use width:16px; height:16px; for it.
Attachment #8569435 - Flags: review?(jaws) → feedback+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Comment on attachment 8569435 [details] [diff] [review]
> 1136671.patch
> 
> Review of attachment 8569435 [details] [diff] [review]:
> -----------------------------------------------------------------
> Also, the size of the arrow is too small at height:8px, width:12px. The SVG
> here is square, and we can use width:16px; height:16px; for it.
16px feels a bit small to me. Maybe 18px like the menulist dropdown icons ?
Assignee: nobody → gioyik
Attached patch 1136671.patch (obsolete) — Splinter Review
Patch v4
Attachment #8570001 - Flags: review?(jaws)
Seems this patch is correct. Could you review it?
Keywords: checkin-needed
Hi,

this didn't apply cleanly:

adding 1136671 to series file
renamed 1136671 -> 1136671.patch
applying 1136671.patch
patching file toolkit/themes/osx/global/jar.mn
Hunk #1 FAILED at 188
1 out of 1 hunks FAILED -- saving rejects to file toolkit/themes/osx/global/jar.mn.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

could you take a look thanks!
Flags: needinfo?(gioyik)
Keywords: checkin-needed
Attached patch 1136671.patchSplinter Review
Attachment #8570001 - Attachment is obsolete: true
Hi,

I created the patch again with a latest mozilla-central source. Could you test it again? And if it fails again, could you tell me exactly what is the problem or where it is failing?

Thanks
Flags: needinfo?(gioyik) → needinfo?(cbook)
Since this is just a rebasing problem, just set checkin-needed again.
Flags: needinfo?(cbook)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/5e569b29f33b
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5e569b29f33b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][fixed-in-fx-team] → [good first bug][lang=css]
Target Milestone: --- → Firefox 39
Comment on attachment 8574500 [details] [diff] [review]
1136671.patch

(per meeting with Jared)

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: tree sort arrows are too light
[Describe test coverage new/current, TreeHerder]: nope
[Risks and why]: low, png to svg swap using an existing svg
[String/UUID change made/needed]: nope
Attachment #8574500 - Flags: approval-mozilla-aurora?
Attachment #8574500 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.