328812 - Windows users are unable to select non-default themes when high-contrast mode is enabled

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Per a conversation with mconnor on irc.m.o, this is a combination/clone of bugs 328119 and 328247 in order to escape the noise and give this issue a clean slate.

Pertinent information and patch from Alfred Kayser on bug 328119 comments 16 and 17:
> Ok, found the cause of this issue. Check out:
> Bug 239914 When high contrast theme used, automatically ignore author's colors
> (https://bugzilla.mozilla.org/show_bug.cgi?id=239914#c16)
> 
> Specifically comment #16, where it is described that when the 'OS
> Accessibility' flag is set, then
> the Mozilla applications (all of them!), always revert back to the 'classic'
> theme.
> The 'classic' theme is designed to use the system colours and styling, and the
> reasoning is that when the 'OS Accessibility' flag is set, that theme should be
> selected to apply the system colours and styling...
> 
> However, as also discussed in that bug, this may be valid for the
> pages/document, but not to the theme selection, as a user may want to
> explicitly select another theme (with extra large icons for example). 
> 
> Furthermore, the way that this feature is hidden: reverting back to 'classic'
> without any message, not even when someone tries to select another theme, based
> on an hidden internal OS accessibility flag, is confusing for a lot of people.
> (as seen by the start of this issue: 'Themes don't apply after install').
> 
> I propose to remove this hidden feature, as nowadays by default FF only as the
> 'classic' theme, so that people without themes, and the flag turned on, are
> still using 'classic'. But those people, like nuncus himself, want to
> EXPLICITELY select a specific theme (those that are designed for
> accessibility!), are now prevented to do so, as there is no way to override
> this behaviour!
> 
> To remove this feature, just remove the 'CheckUseAccessibleSkin' completely
> from: http://lxr.mozilla.org/mozilla/source/xpfe/bootstrap/nsAppRunner.cpp
> 
> Note, this feature may have been applicable when Mozilla by default had two
> themes installed, of which the 'modern' theme 'not very accessible' (very not
> ...), so reverting automatically to 'classic' was usefull. But nowadays only
> the 'classic' (or default theme is distributed). So, when another theme is
> installed the user explicitly wants to have that theme, but it prevented by
> this hidden feature. So, give control back to the users, and remove this
> 'feature'.

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Patch from Alfred Kayser ([e-mail])

See bug 328119 comment 17

Comment 2

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 328247 has been marked as a duplicate of this bug. ***

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 328119 has been marked as a duplicate of this bug. ***

Comment 4

[Alfred Kayser]

Thanks for the reviews!

Who can check this into the tree?

And shall be also do this for the 1.8 branch for FF2?
And for FF1.5.0.x??

Comment 5

[Aaron Leventhal]

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

Makes great sense. We needed it more in the days of the modern theme, although perhaps we should have still allowed users to adjust the theme. Thanks for the patch.

Comment 6

[Daniel Veditz [:dveditz]]

Too late for a non-security fix in 1.8.0.2, especially one that hasn't landed and been tested anywhere.

Comment 7

[Benjamin Smedberg]

This code doesn't affect Firefox at all; it's a seamonkey-only API. The equivalent toolkit codepath is:

http://lxr.mozilla.org/mozilla/source/chrome/src/nsChromeRegistry.cpp#571
called through
http://lxr.mozilla.org/mozilla/source/toolkit/xre/nsAppRunner.cpp#734

Comment 8

[Benjamin Smedberg]

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

Seamonkey still uses two themes, one of which doesn't obey high-contrast native theming: I'm not sure they want this patch.

Comment 9

[[e-mail]]

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

Not a problem as our default theme does obey high-contrast native theming.

Comment 10

[Aaron Leventhal]

Why isn't it checked into trunk?

Comment 11

[Reed Loden [:reed]]

Bug 380786 removed xpfe/bootstrap/nsAppRunner.cpp from trunk. Marking fixed.