Closed Bug 1108189 Opened 5 years ago Closed 5 years ago

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

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(seamonkey2.33 unaffected, seamonkey2.34+ fixed, seamonkey2.35+ fixed)

RESOLVED FIXED
seamonkey2.35
Tracking Status
seamonkey2.33 --- unaffected
seamonkey2.34 + fixed
seamonkey2.35 + fixed

People

(Reporter: philip.chee, Assigned: rsx11m.pub)

References

()

Details

Attachments

(2 files, 2 obsolete files)

(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.
Summary: Colour preference prefpane needs to be adjusted Bug Bug 639134 changed boolean "browser.display.use_document_colors" to tristate "browser.display.document_color_use" → Color preference pane needs to be adjusted after bug 639134 changed boolean "browser.display.use_document_colors" to tristate "browser.display.document_color_use"
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.
(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.
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.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
(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.
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.
(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 neil@parkwaycc.co.uk 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"
(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".
Attached patch Part I: String changes (obsolete) — Splinter Review
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).
Attachment #8546892 - Flags: ui-review?(neil)
Attachment #8546892 - Flags: review?(neil)
Now, if I only had switched en-GB to en-US after copy-pasting...  :-[
Attachment #8546892 - Attachment is obsolete: true
Attachment #8546892 - Flags: ui-review?(neil)
Attachment #8546892 - Flags: review?(neil)
Attachment #8546897 - Flags: ui-review?(neil)
Attachment #8546897 - Flags: review?(neil)
Attachment #8546897 - Flags: ui-review?(neil)
Attachment #8546897 - Flags: ui-review+
Attachment #8546897 - Flags: review?(neil)
Attachment #8546897 - Flags: review+
Thanks, Neil!

Ratty, can you push this to comm-central? (maybe along with bug 1102576?)
Keywords: checkin-needed
Whiteboard: [leave open]
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
Keywords: checkin-needed
Attachment #8546897 - Attachment description: Part I: String changes (v2) → Part I: String changes (v2) r/ui-r=Neil [check-in comment 11]
Attached patch Part II: Code and Help changes (obsolete) — Splinter Review
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.
Attachment #8549146 - Flags: ui-review?(neil)
Attachment #8549146 - Flags: review?(iann_bugzilla)
Whiteboard: [leave open]
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.)
Attachment #8549146 - Flags: ui-review?(neil) → ui-review+
(In reply to neil@parkwaycc.co.uk 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).
Attachment #8549146 - Attachment is obsolete: true
Attachment #8549146 - Flags: review?(iann_bugzilla)
Attachment #8549179 - Flags: ui-review+
Attachment #8549179 - Flags: review?(iann_bugzilla)
(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.
Attachment #8549179 - Flags: review?(iann_bugzilla) → review+
Thanks, push of attachment 8549179 [details] [diff] [review] for comm-central please.
Keywords: checkin-needed
Whiteboard: [c-n: push #8549179 for comm-central]
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.
Attachment #8549179 - Flags: approval-mozilla-aurora?
Comment on attachment 8549179 [details] [diff] [review]
Part II: Code and Help changes (v2)

Sorry, flipped the wrong switch again...  :-[
Attachment #8549179 - Flags: approval-mozilla-aurora? → approval-comm-aurora?
Attachment #8549179 - Flags: approval-comm-aurora? → approval-comm-aurora+
Thanks, please push for comm-aurora as well (attachment 8549179 [details] [diff] [review] only).
Whiteboard: [c-n: push #8549179 for comm-central] → [c-n: push #8549179 for comm-central/comm-aurora]
http://hg.mozilla.org/comm-central/rev/5634cccd23fd
Target Milestone: --- → seamonkey2.35
Whiteboard: [c-n: push #8549179 for comm-central/comm-aurora] → [c-n: push #8549179 for comm-aurora]
http://hg.mozilla.org/releases/comm-aurora/rev/8628a2509820
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: push #8549179 for comm-aurora]
You need to log in before you can comment on or make changes to this bug.