1047595 - Colors of and in XUL colorpicker don't work in-content when using high-contrast mode or when author colors are disabled in the preferences

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Alice0775 White]

Steps To Reproduce:
Steps To Reproduce:
1. Open Preferences
2. Select Content
3. Click "Colors..."
4. Turn off "Allow pages to choose their own colors, instead of my selections above"
   --- No indicate current color state --- BUG
5. Click a current color state to popup color picker
   --- No color

Actual Results:
Completely broken

Comment 1

[Alice0775 White]

Attachment: bug screenshot

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

in-content prefs is not shipping in 34. Dropping tracking.

Comment 4

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

From in-content preference triage today: Removing this as a blocker for shipping in-content prefs. We may want to simply remove these options from the preferences.

Comment 5

[Manish Goregaokar [:manishearth]]

WFM on nightly Linux. Alice, can you check if this bug still happens for you?

Comment 6

[Alice0775 White]

(In reply to Manish Goregaokar [:manishearth] from comment #5)
> WFM on nightly Linux. Alice, can you check if this bug still happens for you?

Still happens

teps To Reproduce:
1. Open Preferences
2. Select Content
3. Click "Colors..."
4. Select "Never"


Actual Results:
Completely broken

Comment 7

[Manish Goregaokar [:manishearth]]

Can you elaborate on what you mean by broken here?

This is what I see: http://i.stack.imgur.com/WUgTw.png

Comment 8

[Alice0775 White]

(In reply to Manish Goregaokar [:manishearth] from comment #7)
> Can you elaborate on what you mean by broken here?
> 
> This is what I see: http://i.stack.imgur.com/WUgTw.png

Did you click "OK" to perform the change?

Comment 9

[Alice0775 White]

Attachment: Screenshot

Comment 10

[Manish Goregaokar [:manishearth]]

Ah, yep, after clicking OK the entire prefs page loses color. Hm.

Comment 11

[Manish Goregaokar [:manishearth]]

So this is basically because we the pref pane is colored with the normal background-color (etc) CSS attributes.

We can either tweak Gecko so that it doesn't use the color prefs for XUL pages, or tweak this one panel so that it uses some more obscure way of getting colors.

Comment 12

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

I think Gijs was talking about having the in-content prefs opt out of this behavior?

Comment 13

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> I think Gijs was talking about having the in-content prefs opt out of this
> behavior?

I think you're thinking about having a media query for high contrast mode, which is a different discussion and wouldn't help here.

I don't know how hard fixing this is. Mostly, I don't know why the incontent prefs are hitting this behaviour in the first place - there are checks in the relevant code that check for chrome documents, which this should be - but apparently isn't because it's loaded in a tab? Clearly the windowed prefs don't have this issue, and they use the same dialog.

I don't have time to investigate this right now, but if you want, you should be able to set the appropriate conditional breakpoints and/or add logging here:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#609

The code at the bottom of GetDocumentColorPreferences determines whether we use the document's colors, and that should always result in 'true' here, but clearly doesn't.

Comment 14

[Alice0775 White]

The sub dialog UI has been changed.

Updated Steps To Reproduce:
1. Open Preferences
2. Select Content pane
3. Click "Colors..."
4. Select "Always"
5. OK Click "Colors..." again

Actual Results:
Color picker completely broken

Comment 15

[[:Aleksej]]

Reproduced with 2015-05-21-03-02-04-mozilla-central-firefox-41.0a1.ru.linux-x86_64.

Comment 17

[u532768]

Is is possible that always and never are reversed?  Since always behaves like never should behave (no color selection since the colors will never be used), and never behaves like always (color selection since the colors will be used).

The code comments give values, but are they perhaps wrong?

Comment 18

[u532768]

Here's a workaround to select the colors for always.

Step 1.  Select colors.  Then select the high contrast option and click OK.
Step 2.  Select colors.  Then select colors wanted, and click OK.
Step 3.  Select colors.  Then select always and click OK.

This will set firefox to always use the colors you selected in Step 2, the colors from the high contrast option.  Don't know why, but it works.

Comment 20

[tim]

the always/only with high contrast/never option appears to be irrelevant to the colour picker issue. If high contrast is on, the colours do not show, regardless of the option, whereas they always show if not in high contrast mode.
the behaviour is more unusual for standard html. see example, and try it out
a colour picker swatch test for accessibility is below<P>
<html><head></head><body><table border=1>
<tr><td bgcolor=red>&nbsp;&nbsp;&nbsp;</td><td bgcolor=yellow>&nbsp;&nbsp;&nbsp;</td><td bgcolor=green>&nbsp;&nbsp;&nbsp;</td></tr>
<tr><td bgcolor=blue>&nbsp;&nbsp;&nbsp;</td><td bgcolor=magenta>&nbsp;&nbsp;&nbsp;</td><td bgcolor=grey>&nbsp;&nbsp;&nbsp;</td></tr>
<tr><td bgcolor=cyan>&nbsp;&nbsp;&nbsp;</td><td bgcolor=orange>&nbsp;&nbsp;&nbsp;</td><td bgcolor=pink>&nbsp;&nbsp;&nbsp;</td></tr>
</table></body></html>
 
there are more issues with changing to high contrast mode or back to regular contrast in that firefox does not change the contrast of any other than the current tab unless a page reload is done for that tab.

Comment 21

[jd01752]

To bypass the background color problem, would it be acceptable to keep a bunch of, say 1px color image files (70 of them for the current grid) to be stretched across the button faces?

Comment 22

[:Gijs (he/him)]

(In reply to john.donahue from comment #21)
> To bypass the background color problem, would it be acceptable to keep a
> bunch of, say 1px color image files (70 of them for the current grid) to be
> stretched across the button faces?

This is an interesting idea. We might not even need 70 actual files (which is a little annoying and will probably have perf implications, because we'd need to actually read all those image files - the prefs page is written at a high enough level that it'd be hard to optimize that) if we could generate data URIs with SVGs (assuming we can get those to work in HCM / when the pref is turned on, which I'm actually not sure about).

Comment 23

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1047595 - followup: use SVG filled rects to make this workaround actually work, r?jaws

Bug 1047595 - make picking colors work in HCM / when author colors are disabled, r?jaws

Comment 24

[:Gijs (he/him)]

For bonus points, this removes a bit of duplication that became even more of an eyesore when I tried to hardcode the data URIs...

Comment 25

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

Comment on attachment 8673063 [details]
MozReview Request: Bug 1047595 - followup: use SVG filled rects to make this workaround actually work, r?jaws

https://reviewboard.mozilla.org/r/21837/#review19635

::: toolkit/content/widgets/colorpicker.xml:203
(Diff revision 1)
> +            let dataURI = 'data:image/svg+xml,<svg style="background-color: ' +
> +                          encodeURIComponent(el.getAttribute("color")) +
> +                          '" xmlns="http://www.w3.org/2000/svg" />';
> +            el.setAttribute("src", dataURI);

Can you add a comment here to say that this is necessary so HCM doesn't override the colors? Otherwise this will look like an overly complex hack to the next person who edits this file.

::: toolkit/content/widgets/colorpicker.xml:453
(Diff revision 1)
> +            'data:image/svg+xml,<svg style="background-color: ' +
> +            encodeURIComponent(val) +
> +            '" xmlns="http://www.w3.org/2000/svg" />');

I would prefer if you invert the quotes to have the attribute value string denoted by double-quotes and the internal strings use single-quotes.

Here and below.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/657b0ff4515d

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/657b0ff4515d

Comment 28

[jd01752]

(In reply to :Gijs Kruitbosch from comment #22)
> ... if we could generate
> data URIs with SVGs (assuming we can get those to work in HCM / when the
> pref is turned on, which I'm actually not sure about).

I tried the nightly build that includes this change -- the inspector window did show the new svg code when viewing color preferences.  Unfortunately the colors are still being overridden.
If it is in fact not working, is it possible to use the svg <rect> element and fill property instead of relying on the the svg's background-color property? so something like:

<svg width=100% height=100%>
  <rect width=100% height=100% fill=encodeURIComponent(this.color)>
</svg>

Comment 29

[:Gijs (he/him)]

(In reply to john.donahue from comment #28)
> (In reply to :Gijs Kruitbosch from comment #22)
> > ... if we could generate
> > data URIs with SVGs (assuming we can get those to work in HCM / when the
> > pref is turned on, which I'm actually not sure about).
> 
> I tried the nightly build that includes this change -- the inspector window
> did show the new svg code when viewing color preferences.  Unfortunately the
> colors are still being overridden.
> If it is in fact not working, is it possible to use the svg <rect> element
> and fill property instead of relying on the the svg's background-color
> property? so something like:
> 
> <svg width=100% height=100%>
>   <rect width=100% height=100% fill=encodeURIComponent(this.color)>
> </svg>

Well, this is embarrassing. I did actually test things, but I clearly did a poor job. I think part of what is (still!) confusing here is that if you follow the following steps:

1) start firefox while the pref is set to 'never' [override the colors]
2) open the preferences, open the colors dialog
3) switch to "always" [override the colors]
4) click OK
5) reopen the colors dialog

the right swatches still display (ie the things that indicate the current value of the colorpicker). Closing the preferences and reopening them also still shows them. It seems like that's because of SVG caching or something. If you restart Firefox those swatches do stop working, too, so it's just altogether busted.

I don't know if the workaround you suggested will help or not... but I can try...

Comment 30

[:Gijs (he/him)]

Comment on attachment 8673063 [details]
MozReview Request: Bug 1047595 - followup: use SVG filled rects to make this workaround actually work, r?jaws

Bug 1047595 - followup: use SVG filled rects to make this workaround actually work, r?jaws

Comment 31

[:Gijs (he/him)]

Comment on attachment 8673063 [details]
MozReview Request: Bug 1047595 - followup: use SVG filled rects to make this workaround actually work, r?jaws

John, thanks for testing this immediately, flagging up that this wasn't working, and suggesting an alternative. Super helpful.

I believe that this patch *does* work... and tested perhaps more extensively than I must have done before. Probably still good if you doublecheck though, Jared... :-\

Comment 32

[:Gijs (he/him)]

I stuck a note in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines#Gotchas about the effects here, and I filed bug 1215484 to get some of this adjusted in platform - I think the current mishmash of what does/doesn't work is wrong. In any case, I believe the correct behaviour would be for everything in an <image> or <img> tag to be rendered as intended by the author (whereas colors in background-images should get stripped... or the bg image should just not be rendered at all, which is probably simpler, actually).

Comment 33

[:Gijs (he/him)]

Comment on attachment 8673063 [details]
MozReview Request: Bug 1047595 - followup: use SVG filled rects to make this workaround actually work, r?jaws

This is now being addressed in bug 1215484. With the patch from that bug applied, this is indeed fixed. I'm going to clear review, add as a dep, and we can resolve this as fixed when bug 1215484 is fixed (already has a patch with review, so should be a matter of a few days at most). :-)

Comment 34

[:Gijs (he/him)]

Fixed by bug 1215484!

Comment 35

[Robert Longson [:longsonr]]

(In reply to :Gijs Kruitbosch from comment #32)
> I stuck a note in
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> SVG_Guidelines#Gotchas about the effects here, and I filed bug 1215484 to
> get some of this adjusted in platform - I think the current mishmash of what
> does/doesn't work is wrong. In any case, I believe the correct behaviour
> would be for everything in an <image> or <img> tag to be rendered as
> intended by the author (whereas colors in background-images should get
> stripped... or the bg image should just not be rendered at all, which is
> probably simpler, actually).

Could you update (or possibly remove) this now?

Comment 36

[:Gijs (he/him)]

(In reply to Robert Longson from comment #35)
> (In reply to :Gijs Kruitbosch from comment #32)
> > I stuck a note in
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> > SVG_Guidelines#Gotchas about the effects here, and I filed bug 1215484 to
> > get some of this adjusted in platform - I think the current mishmash of what
> > does/doesn't work is wrong. In any case, I believe the correct behaviour
> > would be for everything in an <image> or <img> tag to be rendered as
> > intended by the author (whereas colors in background-images should get
> > stripped... or the bg image should just not be rendered at all, which is
> > probably simpler, actually).
> 
> Could you update (or possibly remove) this now?

Done, thanks for reminding me. It might take a bit for caches to be updated and so on.