1113173 - InContent prefs - Search tree styling issues

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Tim Nguyen :ntim]

Since bug 1113100 was landed at the same time as bug 1111504, it caused some small conflicts :
- search.css forces the text to be black, even for the selected row (which has a blue background after bug 1111504), to fix this, we can simply remove the rule in search.css that forces the text color to -moz-fieldText.
- Now that the selected row background is blue, the checkbox is almost invisible if the row is selected.

Also, I've noticed a glitch while selecting, and hovering the items, the row sometimes blinks, but this looks rather like a platform issue.

Comment 1

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #0)
> Since bug 1113100 was landed [...]
Argh, meant bug 1106559

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch

Currently building to test if it works

Comment 3

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

Comment on attachment 8538600 [details] [diff] [review]
Patch

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

WFM, I can't get the rows to flicker as you stated in comment #0.

Comment 4

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

Comment on attachment 8538600 [details] [diff] [review]
Patch

Actually, this is broken when Page Colors are disabled. We show a white checkbox on a near-white background.

Comment 5

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

Can we use the SVG checkmark here?

Comment 6

[Florian Quèze [:florian]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Can we use the SVG checkmark here?

No, it doesn't work in XUL trees :(.

Comment 7

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> > Can we use the SVG checkmark here?
> 
> No, it doesn't work in XUL trees :(.

It seems I was wrong on this, apparently Dão successfully did it in bug 1101996.

Comment 8

[Tim Nguyen :ntim]

Attachment: Patch v2

Switched to the SVG file. Still haven't tested it locally though.

Comment 9

[Tim Nguyen :ntim]

Attachment: Patch v2.1

Forgot to remove the pngs in my previous patch.

Comment 10

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

Comment on attachment 8538712 [details] [diff] [review]
Patch v2.1

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

::: toolkit/themes/shared/in-content/check.svg
@@ +8,5 @@
>      use {
>        fill: #2292d0;
>      }
> +    use[id$="-inverted"] {
> +      fill: #fff;

Won't this still break if the Page Colors are disabled?

Comment 11

[Tim Nguyen :ntim]

Current waiting for bug 1112566 to land before I can test things properly.

Comment 12

[Tim Nguyen :ntim]

Attachment: Patch v3

The SVG didn't work on the file-tree, but it required the width and height attributes.

Comment 13

[Tim Nguyen :ntim]

Attachment: Patch v3.1

Argh, wrong patch.

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8539432 [details] [diff] [review]
Patch v3.1

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

::: toolkit/themes/shared/in-content/check.svg
@@ +15,5 @@
>      }
> +    use[id$="-inverted"] {
> +      fill: #fff;
> +      stroke: #0095dd;
> +      stroke-width: 1;

This is the only way I got it visible with Page Colors disabled, without changing the default styling.

Comment 15

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: this bug makes the selected row of the search engines tree difficult to use.

36 is not affected as bug 1111504 didn't land there.

Comment 16

[Tim Nguyen :ntim]

Florian, can you back out this rule : [0] from beta and aurora ? Bug 1087618 Part 1 didn't land on these channels, so the tree still uses the native styling, and since [0] is forcing black text, it'll likely break.

[0] : https://hg.mozilla.org/releases/mozilla-beta/rev/d70d976bf024#l8.47

Comment 17

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

Comment on attachment 8539432 [details] [diff] [review]
Patch v3.1

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

::: toolkit/themes/shared/in-content/check.svg
@@ +15,5 @@
>      }
> +    use[id$="-inverted"] {
> +      fill: #fff;
> +      stroke: #0095dd;
> +      stroke-width: 1;

Let's go with `stroke-width: .5;`. With the stroke-width at 1, the inverted fill area is too skinny when Page Colors are enabled. A stroke of .5 still shows the border enough when Page Colors are disabled.

Comment 18

[Tim Nguyen :ntim]

Attachment: Patch v3.2 (r=jaws)

Used stroke-width: 0.5;

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1a47604b575e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a47604b575e