1136671 - InContent prefs - Tree sort arrows should be darker

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Tim Nguyen :ntim]

Maybe dropdown.svg can be used (except it needs to be backwards). Also, that asset is high contrast friendly.

Comment 1

[Tim Nguyen :ntim]

To fix this bug, you need to remove sorter.png and [e-mail] 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

Comment 2

[Giovanny Gongora [:gioyik]]

Attachment: 1136671.patch

Patch v1

Comment 3

[Giovanny Gongora [:gioyik]]

Hi Tim, I created a patch for this, could you review it and tell me if is ok?

Comment 4

[Tim Nguyen :ntim]

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 [e-mail] 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 :)

Comment 5

[Giovanny Gongora [:gioyik]]

Attachment: 1136671.patch

Patch v2

Comment 6

[Giovanny Gongora [:gioyik]]

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 7

[Tim Nguyen :ntim]

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/[e-mail]             (../../shared/in-content/[e-mail])

There are some references for aero later in the file that you need to remove as well.

Comment 8

[Giovanny Gongora [:gioyik]]

Attachment: 1136671.patch

Patch v3

Comment 9

[Giovanny Gongora [:gioyik]]

Thanks for the feedback Tim, I didn't see those lines. Now the patch is updated.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 11

[Tim Nguyen :ntim]

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

Comment 12

[Giovanny Gongora [:gioyik]]

Attachment: 1136671.patch

Patch v4

Comment 13

[Giovanny Gongora [:gioyik]]

Seems this patch is correct. Could you review it?

Comment 14

[Carsten Book [:Tomcat]]

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!

Comment 15

[Giovanny Gongora [:gioyik]]

Attachment: 1136671.patch

Comment 16

[Giovanny Gongora [:gioyik]]

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

Comment 17

[Tim Nguyen :ntim]

Since this is just a rebasing problem, just set checkin-needed again.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/5e569b29f33b

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e569b29f33b

Comment 20

[:Gijs (he/him)]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f1b126e6981