1022553 - Audio/video controls unusable in High Contrast mode

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot

The audio/video controls are completely unusable when using High Contrast mode. See attached screenshot.

Comment 1

[Dão Gottwald [:dao]]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Dão Gottwald [:dao]]

(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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

David, thoughts on that idea?  See comment 2.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sounds fine with me.

Comment 6

[Justin Dolske [:Dolske]]

(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).

Comment 8

[Hani Yacoub, Desktop QA]

Attachment: 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.)

Comment 10

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

(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?

Comment 12

[Jet Villegas (inactive)]

Let's ask dholbert who's also looking into bug 1319653

Comment 14

[Daniel Holbert [:dholbert]]

(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.)

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sorry, I meant comment 2. And with background-image properties in UA sheet they will not be ignored in high contrast mode.

Comment 17

[Daniel Holbert [:dholbert]]

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.

Comment 18

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

Attachment: Bug 1022553 - Attempt to introduce a stylesheet type for XBL documents.

This patch adds a stylesheet type of XblDoc. I put it next to SheetType::Doc in the cascade, which I think is correct but I'm not sure about. After building with this patch, I'm seeing a segfault at nsRuleNode.cpp line 1979 ('while (*curr != aNode)').

Perhaps I missed incrementing another counter or I missed updating another area of code that will need to know about this new SheetType?

Review commit: https://reviewboard.mozilla.org/r/153162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153162/

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.

Comment 21

[Morgan Reschenberg [:morgan]]

This should be fixed as of backplate landing, :dholbert do you know what the original STR for that screenshot in #c1 were?

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: screenshot as of March 2020 in Linux with "override the colors: always"

I think this is still an issue, actually, though it doesn't look as bad as the original screenshots here. (But still kinda unusable.)

The STR are just to view & interact with the controls for any <video controls> element. A trivial reduced testcase (with no actual video data) is data:text/html,<video controls>. And some examples with actual video content are http://camendesign.com/code/video_for_everybody/test.html and https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video .

And the "Actual results" at this point (for me, in Linux nightly with high-contrast mode activated via about:preferences "Override the colors...: Always"):

• the icons are all invisible unless hovered (play/pause, mute/unmute, fullscreen)
• the slider tracks are invisible
• the picture-in-picture overlay (which appears on the right side of video when I hover it) is white-on-lightgray

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: screenshot with yellow-on-black

(In reply to Daniel Holbert [:dholbert] from comment #22)

• the icons are all invisible unless hovered (play/pause, mute/unmute, fullscreen)

Note: it looks like these icons are actually just always white (regardless of my chosen foreground/background colors), so they may or may not be visible depending on the background color.

I think these icons are SVG, so this might really end up depending on bug 1616916 (and could require some opt-in markup in the SVG, if opt-in is needed).

• the slider tracks are invisible
• the picture-in-picture overlay (which appears on the right side of video when I hover it) is white-on-lightgray

These ^^ issues still happen regardless of my choice of background color, FWIW.

Comment 24

[Daniel Holbert [:dholbert]]

The styling for these controls is e.g.

.playButton {
  background-image: url(chrome://global/skin/media/pauseButton.svg);
}

https://searchfox.org/mozilla-central/source/toolkit/themes/shared/media/videocontrols.css#168

You can take a look at the button itself (in isolation) by just directly visiting chrome://global/skin/media/pauseButton.svg , or you can see it in searchfox at
https://searchfox.org/mozilla-central/source/toolkit/themes/shared/media/pauseButton.svg

Its fill-color is set via fill="context-fill" in the SVG source, which uses a mozilla-internal way to pass in a custom possibly-browser-theme-controlled color from an external document to an embedded SVG document. (The outer document opts in to passing in the value by setting e.g. -moz-context-properties: fill; as you can see elsewhere in videocontrols.css, and the inner document opts in to using that value via the special color keyword context-fill. This only works for the svg-specific properties fill and stroke, as I recall, because we don't want it to become a generalized thing.)

Anyway, I say all that just to clarify how these icons are drawn right now. Ideally we should have them do whatever is necessary to honor the high-contrast requirements, via bug 1616916 or whatever other high-contrast-for-SVG work ends up happening.

Comment 25

[Morgan Reschenberg [:morgan]]

Thanks for taking a look and adding all that info! :)

Comment 26

[Tim Nguyen :ntim]

Attachment: Bug 1022553 - Improve usability of video controls in high contrast mode.

Comment 27

[Tim Nguyen :ntim]

This patch gets us most of the way, but with this patch applied, the control bar icons color is always forced to ButtonText due to bug 1625036. With bug 1625036 fixed, everything should work fine.

Comment 28

[Petruta Horea [:phorea], Desktop QA]

Attachment: video controls.png

HTML5 video from https://www.w3.org/2010/05/video/mediaevents.html doesn't display the progress bars (video and sound) on Windows 10 and HCM. This also reproduces on older Firefox versions, so not a recent regression. Please let me know if I should file a new bug for this.

Mac OSX 11.6 and Ubuntu 21.04 are not affected.

Comment 29

[:Gijs (he/him)]

(In reply to Petruta Horea [:phorea] from comment #28)

Created attachment 9266227 [details]
video controls.png

HTML5 video from https://www.w3.org/2010/05/video/mediaevents.html doesn't display the progress bars (video and sound) on Windows 10 and HCM. This also reproduces on older Firefox versions, so not a recent regression. Please let me know if I should file a new bug for this.

Mac OSX 11.6 and Ubuntu 21.04 are not affected.

Yes, please file a new bug.

Comment 30

[Petruta Horea [:phorea], Desktop QA]

Thank you!(In reply to :Gijs (he/him) from comment #29)

Yes, please file a new bug.

Thank you! I've created bug 1758081 for the mentioned issue.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 32

[kpatenio]

Hi Morgan - I'm no longer seeing any contrast problems with our native video controls on Windows 10 High Contrast Mode and Windows 11 High Contrast Themes. Do you think this issue can be closed?

Comment 33

[Morgan Reschenberg [:morgan]]

Yep! Sounds good :)

Comment 34

[kpatenio]

Thanks! Closing this bug as resolved - fixed.