Audio/video controls unusable in High Contrast mode

NEW
Unassigned

Status

()

Toolkit
Video/Audio Controls
P1
major
4 years ago
2 months ago

People

(Reporter: Unfocused, Unassigned)

Tracking

(Blocks: 2 bugs, {access})

Trunk
All
Windows
access
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Created attachment 8436796 [details]
Screenshot

The audio/video controls are completely unusable when using High Contrast mode. See attached screenshot.
Flags: firefox-backlog+
Blocks: 1016556
No longer blocks: 1022551
Keywords: access
Priority: -- → P1
The main problem seems to be that we're using background-image extensively:

https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/media/videocontrols.css

Not sure if there's a good CSS-only alternative (maybe some hack involving ::before?) or whether we should use img tags instead (not great in terms of separating styling from content.)

Boris, short of bug 957988, is there some way to make Gecko not remove background images in this particular case?
Flags: needinfo?(bzbarsky)
Severity: normal → major
OS: Windows 8.1 → Windows
Hardware: x86_64 → All
Whiteboard: p=3
Version: unspecified → Trunk
So the issue here is that background colors/images are ignored when forcing document colors, which is the default behavior in high-contrast themes.

This means that you get the same problem simply if you force the user-prefence colors in preferences, right?

The right general fix for this is to put the relevant styles into a UA or user sheet, not an author sheet.  Those are not affected by this setting.  Unfortunately, video controls use an xbl stylesheet, not a UA sheet, even though conceptually they are part of UA styling...

I suppose we could introduce a new cascade level specifically for XBL, which will be just like doc rules except for ignoring the CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED bit.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #2)
> So the issue here is that background colors/images are ignored when forcing
> document colors, which is the default behavior in high-contrast themes.
> 
> This means that you get the same problem simply if you force the
> user-prefence colors in preferences, right?

yes

> I suppose we could introduce a new cascade level specifically for XBL, which
> will be just like doc rules except for ignoring the
> CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED bit.

This might be handy for bug 1022605 too.
David, thoughts on that idea?  See comment 2.
Flags: needinfo?(dbaron)
Sounds fine with me.
Flags: needinfo?(dbaron)
(In reply to Dão Gottwald [:dao] from comment #1)

> Not sure if there's a good CSS-only alternative (maybe some hack involving
> ::before?) or whether we should use img tags instead (not great in terms of
> separating styling from content.)

FWIW, they're background-images because having them be <image>s caused |load| events to be emitted... Which were visible to content, and that's bad. Especially early on when <video> itself emitted a |load| event (although iirc that's since been removed).

Updated

11 months ago
Duplicate of this bug: 641587

Updated

11 months ago
Blocks: 492516

Comment 8

10 months ago
Created attachment 8828358 [details]
High contrast(Nightly52xNightly53).png

The Audio/video controls in High Contrast mode in the latest nightly are displayed differently comparing with Firefox Nightly 52.0a1.
The controls in Firefox Nightly 52.0a1 are also invisible but there is box displayed where the icon should be. (Please see the screenshot attached.)

Updated

10 months ago
Duplicate of this bug: 1338469

Comment 10

10 months ago
(In reply to Hani Yacoub from comment #8)
> Created attachment 8828358 [details]
> High contrast(Nightly52xNightly53).png
> 
> The Audio/video controls in High Contrast mode in the latest nightly are
> displayed differently comparing with Firefox Nightly 52.0a1.
> The controls in Firefox Nightly 52.0a1 are also invisible but there is box
> displayed where the icon should be. (Please see the screenshot attached.)

I don't think this makes a meaningful difference. They were unusable before and they're pretty much just as unusable now.

Comment 11

10 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> The right general fix for this is to put the relevant styles into a UA or
> user sheet, not an author sheet.  Those are not affected by this setting. 
> Unfortunately, video controls use an xbl stylesheet, not a UA sheet, even
> though conceptually they are part of UA styling...
> 
> I suppose we could introduce a new cascade level specifically for XBL, which
> will be just like doc rules except for ignoring the
> CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED bit.

Jet, how hard is this and/or could someone from the platform team work on this?
Flags: needinfo?(bugs)

Comment 12

10 months ago
Let's ask dholbert who's also looking into bug 1319653
Flags: needinfo?(bugs) → needinfo?(dholbert)
Duplicate of this bug: 1339512
(For reference, for me & anyone else: To easily trigger High Contrast Mode on non-windows platforms, set about:config pref browser.display.document_color_use = 2.  This is exposed in our Preferences UI, too, though it's a bit buried so the about:config pref is more direct.)
See Also: → bug 1338502
Before comment 10 can be implemented, it might make sense to dynamic inject a UA sheet with DOMWindowUtils like this:

http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/toolkit/content/datepicker.xhtml#40-47
Sorry, I meant comment 2. And with background-image properties in UA sheet they will not be ignored in high contrast mode.
Sorry for not getting to this sooner - I've been focused on Quantum Flow stuff recently, and I didn't know the answer to comment 11 (RE how hard this is) offhand.

Just poked around the code with jaws, with bz's suggestions in mind from comment 2 here, and I think it's pretty straightforward.  jaws has some relevant code up in tabs and is spinning up a patch right now -- transferring needinfo to him.
Flags: needinfo?(dholbert) → needinfo?(jaws)
See Also: → bug 1373655
Comment hidden (mozreview-request)
Comment on attachment 8882066 [details]
Bug 1022553 - Attempt to introduce a stylesheet type for XBL documents.

https://reviewboard.mozilla.org/r/153162/#review158380

Please make sure to file a bug on doing this for stylo too...

::: layout/style/SheetType.h:25
(Diff revision 1)
>  enum class SheetType : uint8_t {
>    Agent, // CSS
>    User, // CSS
>    PresHint,
>    Doc, // CSS
> +  XblDoc, // CSS

You need to document why this new type is not in gCSSSheetTypes.

And you presumably want to document why the end of nsStyleSet::GatherRuleProcessors doesn't need to create an nsCSSRuleProcessor for this type.  And avoit it hitting the fatal assert there.

In nsStyleSet::FileRules you need to update the "Cascading order" comment and more importantly the code: you need to SetLevel the right thing before walking XBL rules.  You also need to save the right start/end rulenode pointers for the new level so the !important rules can also be done with the right SetLevel.

And you probably need to add the new thing to gCascadeLevels in the !important section, right?

::: layout/style/nsRuleNode.cpp:10643
(Diff revision 1)
>                // author-specified rules if we're looking at a non-color
>                // property or if we're looking at the background color and it's
>                // set to transparent.  Anything else should get set to a dummy
>                // value instead.
>                if (aAuthorColorsAllowed ||
> +                  ruleData.mLevel == SheetType::XblDoc ||

This part doesn't look right.  Why is this not checked where we check for agent/user styles?

::: layout/style/nsStyleStruct.h:107
(Diff revision 1)
>  #define NS_RULE_NODE_IS_ANIMATION_RULE      0x01000000
>  // Free bit                                 0x02000000
>  #define NS_RULE_NODE_USED_DIRECTLY          0x04000000
>  #define NS_RULE_NODE_IS_IMPORTANT           0x08000000
> -#define NS_RULE_NODE_LEVEL_MASK             0xf0000000
> -#define NS_RULE_NODE_LEVEL_SHIFT            28
> +#define NS_RULE_NODE_LEVEL_MASK             0xe0000000
> +#define NS_RULE_NODE_LEVEL_SHIFT            24

Why these changes?  They don't look right to me at all...  In particular, changing the shift from 28 to 24 is just wrong and will lead to incorrect levels being gotten from rulenodes, as far as I can tell.
Attachment #8882066 - Flags: review?(bzbarsky) → review-
Flags: needinfo?(jaws)

Updated

2 months ago
See Also: → bug 1387344

Updated

2 months ago
See Also: → bug 1266172
You need to log in before you can comment on or make changes to this bug.