1108189 - Color preference pane needs to be adjusted after bug 639134 changed boolean "browser.display.use_document_colors" to tristate "browser.display.document_color_use"

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Philip Chee]

(from Bug 639134 comment #0)

> When I select High contrast black theme in Windows 7 theme selector, the
> document colors revert to the theme default even if the checkbox "Allow
> pages to choose their own colors" is on. Moreover, all images are removed
> from the document view making some sites unusable.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. Select High Contrast Black theme in Personalize.. in Windows 7
> 
> Actual Results:  
> All document colors in each tab revert to theme default and images disappear.
> 
> Expected Results:  
> I expect the colors and images to stay document-specified like they do on my
> Ubuntu box.

Comment 1

[Philip Chee]

I've started a discussion on mozill.dev.platform
https://groups.google.com/d/msg/mozilla.dev.platform/th7gTtRSgo0/JK7uBsx-GJ0J
So hold on to your horses for a bit.

Comment 2

[Philip Chee]

(In reply to Philip Chee from comment #1)
> I've started a discussion on mozill.dev.platform
> https://groups.google.com/d/msg/mozilla.dev.platform/th7gTtRSgo0/JK7uBsx-GJ0J
> So hold on to your horses for a bit.
Looks like no further discussion is happening in this thread. I guess we need to adapt to the new regime.

Comment 3

[rsx11m]

I'm putting this on my plate, but it's rather unrealistic to assume that this will land in time for the next merge on January 12 (upcoming Monday). Thus, a special solution for aurora will be needed.

For trunk, I'd follow bug 1108198 comm-central changeset ca37edf292be for Thunderbird, using strings as checked in there unless any other proposals/requests are made. For aurora/2.33, a simple mapping of the checkbox to the tri-state integer pref should suffice, mimicking the logic applied for migration.

Comment 4

[[e-mail]]

(In reply to rsx11m from comment #3)
> For trunk, I'd follow bug 1108198 comm-central changeset ca37edf292be for
> Thunderbird, using strings as checked in there unless any other
> proposals/requests are made.

Actually we're already using a radiogroup so we just need a string for the high contrast theme case.

Comment 5

[rsx11m]

Ah, I was reading that to quickly and looked at the "Use system colors" option instead (which indeed is a checkbox, but tied to the browser.display.use_system_colors pref). This simplifies things.

Comment 6

[Philip Chee]

(In reply to rsx11m from comment #3)
> I'm putting this on my plate, but it's rather unrealistic to assume that
> this will land in time for the next merge on January 12 (upcoming Monday).
> Thus, a special solution for aurora will be needed.
I apologize for dropping the ball on this. BECAUSE Holidays!

We try not to do this but we can check-in needed strings before the next merge on January 12 which will give us some breathing space for Aurora.

(In reply to [e-mail] from comment #4)
> (In reply to rsx11m from comment #3)
> > For trunk, I'd follow bug 1108198 comm-central changeset ca37edf292be for
> > Thunderbird, using strings as checked in there unless any other
> > proposals/requests are made.
> 
> Actually we're already using a radiogroup so we just need a string for the
> high contrast theme case.
In Firefox Bug 1118032 we have settled on "Only with High Contrast themes"

Comment 7

[[e-mail]]

(In reply to Philip Chee from comment #6)
> (In reply to comment #4)
> > Actually we're already using a radiogroup so we just need a string for the
> > high contrast theme case.
> In Firefox Bug 1118032 we have settled on "Only with High Contrast themes"
Sure, but they didn't have existing UI. We'd have to use something like "Only ignore the page colours when using a High Contrast theme".

Comment 8

[rsx11m]

Attachment: Part I: String changes

Sounds good, I'd add the string as the /last/ option though given that it relates to the "Use my chosen colors, ignoring the colors and background image specified" of the previous option and "specified by the web page" of the first selection.

Obviously, this patch is untested. I'll get to the rest some time next week.

Neil, I'm asking you for both ui-review and review here to expedite the process. If you think that IanN should do the review then please move the r? (but we'd have to ping him in this case to get it done over the weekend).

Comment 9

[rsx11m]

Attachment: Part I: String changes (v2) r/ui-r=Neil [check-in comment 11]

Now, if I only had switched en-GB to en-US after copy-pasting...  :-[

Comment 10

[rsx11m]

Thanks, Neil!

Ratty, can you push this to comm-central? (maybe along with bug 1102576?)

Comment 11

[Philip Chee]

Comment on attachment 8546897 [details] [diff] [review]
Part I: String changes (v2) r/ui-r=Neil [check-in comment 11]

Pushed string changes to comm-central
http://hg.mozilla.org/comm-central/rev/e641b34420af

Comment 12

[rsx11m]

Attachment: Part II: Code and Help changes

I'm not posting a separate patch for Help updates. As I recall, those are ok for aurora since they won't "break" anything like string changes.

It was bugging me a bit that the Help content only talks about "web pages" but doesn't convey that the settings also apply to mail/news content and presumably RSS feeds. Thus, while I'm there, I've changed several occurrences to a more general "web pages and messages" and also expanded the already existing help for the use-document-colors preference.

This patch includes migration code for the pref as well as imported profiles from Thunderbird. Given that we don't have a UI-version number, I'm resetting the old preference once processed, to avoid running the migration code again.

Comment 13

[[e-mail]]

Comment on attachment 8549146 [details] [diff] [review]
Part II: Code and Help changes

>     try {
>       if (Services.prefs.getIntPref("privacy.donottrackheader.value") != 1) {
>         Services.prefs.clearUserPref("privacy.donottrackheader.enabled");
>         Services.prefs.clearUserPref("privacy.donottrackheader.value");
>       }
>     } catch (ex) {}
> 
>+    // Migration of document-color preference which changed from boolean to
>+    // tri-state; 0=always but not accessibility themes, 1=always, 2=never
>+    try {
>+      const kOldColorPref = "browser.display.use_document_colors";
>+      if (Services.prefs.prefHasUserValue(kOldColorPref)) {
>+        if (!Services.prefs.getBoolPref(kOldColorPref))
>+          Services.prefs.setIntPref("browser.display.document_color_use", 2);
>+        Services.prefs.clearUserPref(kOldColorPref);
>+      }
>+    } catch (ex) {}
Since you're in a try/catch already, no need to muck about with prefHasUserValue, just write something like this:
if (Services.prefs.getBoolPref("browser.display.use_document_colors")) {
  Services.prefs.setIntPref("browser.display.document_color_use", 2);
  Services.prefs.clearUserPref("browser.display.use_document_colors");
}

>-  MAKESAMETYPEPREFTRANSFORM("browser.display.use_document_colors",     Bool),
>+  MAKESAMETYPEPREFTRANSFORM("browser.display.document_color_use",      Int),
(Can't decide whether we should migrate old prefs from old profiles.)

Comment 14

[rsx11m]

Attachment: Part II: Code and Help changes (v2)

(In reply to [e-mail] from comment #13)
> Since you're in a try/catch already, no need to muck about with
> prefHasUserValue, just write something like this:
> if (Services.prefs.getBoolPref("browser.display.use_document_colors")) {
>   Services.prefs.setIntPref("browser.display.document_color_use", 2);
>   Services.prefs.clearUserPref("browser.display.use_document_colors");
> }

Yeah, that works fine and is more consistent with the donnottrack-pref handling just before it. If you force browser.display.use_document_colors="true" it will stick around whereas "false" is removed after setting browser.display.use_document_colors; shouldn't be a big deal given that it's a no-op with this setting anyway (and shouldn't happen as user prefs are usually removed when they match their default).

> >-  MAKESAMETYPEPREFTRANSFORM("browser.display.use_document_colors",     Bool),
> >+  MAKESAMETYPEPREFTRANSFORM("browser.display.document_color_use",      Int),
> (Can't decide whether we should migrate old prefs from old profiles.)

I was wondering the same but didn't find any precedence case. Since the recently changed TLS prefs aren't listed either for import, I'd think it's ok (and it's a small thing to change).

Comment 15

[rsx11m]

(In reply to rsx11m from comment #14)
> Since the recently changed TLS prefs aren't listed either for import,

The old boolean prefs for SSL3 and TLS, I mean. And, in contrast to those, a non-migration of browser.display.use_document_colors="false" would be immediately obvious to the user when visiting the first page which defines it's own color patterns.

Comment 16

[rsx11m]

Thanks, push of attachment 8549179 [details] [diff] [review] for comm-central please.

Comment 17

[rsx11m]

Comment on attachment 8549179 [details] [diff] [review]
Part II: Code and Help changes (v2)

Approval Request Comment
[Feature/regressing bug #]: bug 639134;
[User impact if declined]: non-functional UI, unable to change preference;
[Describe test coverage new/current, TBPL]: tested on c-c, just after merge
[Risks and why]: low;
[String/UUID change made/needed]: help updates only, strings already checked in.

Comment 18

[rsx11m]

Comment on attachment 8549179 [details] [diff] [review]
Part II: Code and Help changes (v2)

Sorry, flipped the wrong switch again...  :-[

Comment 19

[rsx11m]

Thanks, please push for comm-aurora as well (attachment 8549179 [details] [diff] [review] only).

Comment 20

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/5634cccd23fd

Comment 21

[Philip Chee]

http://hg.mozilla.org/releases/comm-aurora/rev/8628a2509820