1350325 - video player's play button overlay is barely visible

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Franck (Wip)]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
version 53.0b5 - build id 20170320143328

The play-button overlay displayed when an HTML5 <video> is not yet started is a tiny white triangle which is barley visible with bright poster-image.
e.g. https://www.iandevlin.com/html5test/webvtt/html5-video-webvtt-sample.html (though not the best example)

The former video controls (Firefox 52) have a dark triangle within a bright circle allowing the button to be visible whatever the composition of the poster-image.

Based on the spec https://bugzilla.mozilla.org/attachment.cgi?id=8842340 of bug 1271765, it is missing a dark circle around the play icon.

May be a simple dark outline to the triangle will suffice to make it visible in all cases.

Comment 1

[Franck (Wip)]

Attachment: video-player play-button overlay

Comment 2

[Ray Lin[:ralin]]

Hi Franck,

Are you using customize Window theme(or 3rd-party theme)? Since we do have dark circle around for default theme[0]. The difference is intended to ensure display correctness in non-default theme like high-contrast theme. 


[0]default theme: Luna, Royale, Zune, or Aero (including Vista Basic, Vista Advanced, and Aero Glass

Comment 3

[YF (Yang)]

(In reply to Ray Lin[:ralin] from comment #2)
> Hi Franck,
> 
> Are you using customize Window theme(or 3rd-party theme)? Since we do have
> dark circle around for default theme[0]. The difference is intended to
> ensure display correctness in non-default theme like high-contrast theme. 
> 
> 
> [0]default theme: Luna, Royale, Zune, or Aero (including Vista Basic, Vista
> Advanced, and Aero Glass

Are you use Firefox 53 or higher? I see the issue in Fx53 and Fx54 on Win10.

Comment 4

[Ray Lin[:ralin]]

Ah, you're right. I don't see the background with e10s enabled, but once I turned off e10s, the dark circle just shows up....

Comment 5

[Ray Lin[:ralin]]

Sorry to bother you again, :Gijs


I can not figure out how e10s affect media query, but it occurs that these rules[0] aren't applied for default Windows theme when e10s enabled. Do you have any idea? Thanks.


[0] https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/toolkit/themes/shared/media/videocontrols.css#483-491

Comment 6

[:Gijs (he/him)]

(In reply to Ray Lin[:ralin] from comment #5)
> Sorry to bother you again, :Gijs
> 
> 
> I can not figure out how e10s affect media query, but it occurs that these
> rules[0] aren't applied for default Windows theme when e10s enabled. Do you
> have any idea? Thanks.
> 
> 
> [0]
> https://dxr.mozilla.org/mozilla-central/rev/
> e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/toolkit/themes/shared/media/
> videocontrols.css#483-491

Uh, that's surprising. Clearly, the -moz-windows-default-theme media query isn't matching. You can verify this with the toolbox - when using the default theme, the parent process (browser toolbox) returns an object with matches: true for

window.matchMedia("(-moz-windows-default-theme)")

and the web toolbox for any ordinary webpage returns one with matches: false.

I don't know why that would be the case. You could try debugging it. Following the trail through DXR, -moz-windows-default-theme gets converted to the atom _moz_windows_default_theme, which appears in nsMediaFeatures.cpp (and some stylo code, which I don't think would affect 53?) and points to using getsystemmetric with nsGkAtoms::windows_default_theme. With that atom, you find the code in nsCSSRuleProcessor.cpp , widget/windows/nsLookAndFeel.cpp and nsUXThemeData.cpp . Doing some logging and/or breakpoints there would likely clear things up as to why it doesn't apply. Perhaps we don't have the right permissions with the sandboxed child process to know these things, or perhaps something's broken in CSS code, so perhaps we need to pass this type of info in from the parent. You probably want to talk to a layout or widget person about that, depending on what the problem ends up being. I don't really have cycles to do the debugging and so on, but hopefully these pointers help?

Comment 7

[:Gijs (he/him)]

That said... as long as we can guarantee that the background and foreground of that circle with an arrow are opposite colours, maybe we can use text and background colors in such a way that it is properly visible independent of whether people use high contrast mode and/or custom colours that override document colours? That would work better for the latter case (where the default theme detection doesn't help right now).

Comment 8

[:Gijs (he/him)]

(It's still a (separate) bug that the media query doesn't work, though.)

Comment 9

[Ray Lin[:ralin]]

Appreciate all your informative comments, it's really helpful :-D 

I'll try to follow your suggestion to find out something make this happen.

(In reply to :Gijs from comment #7)
> That said... as long as we can guarantee that the background and foreground
> of that circle with an arrow are opposite colours, maybe we can use text and
> background colors in such a way that it is properly visible independent of
> whether people use high contrast mode and/or custom colours that override
> document colours? That would work better for the latter case (where the
> default theme detection doesn't help right now).

I second that, as current SVG background seems not work fine with HCM now, text with opposite background color should ideally compatible with all theme. Before getting the cause of broken media query, I'll forward this idea to visual designer first and see what we can do now to make current clickToPlay look better.

Comment 10

[:Gijs (he/him)]

bug 1325591 is what regressed this. Given that this affects the default theme, maybe we need to back it out on 53 to avoid things looking bad on the default theme on 53, over things looking bad in HCM.

Comment 11

[Ray Lin[:ralin]]

Instead of backing it out, I'd prefer uplift a new patch to revert only clickToPlay part since we still need the overlay fix in HCM in bug 1325591. Do you think it acceptable? thanks

Comment 12

[:Gijs (he/him)]

(In reply to Ray Lin[:ralin] from comment #11)
> Instead of backing it out, I'd prefer uplift a new patch to revert only
> clickToPlay part since we still need the overlay fix in HCM in bug 1325591.
> Do you think it acceptable? thanks

I'd need to see the patch, and either way it'd need more testing than a straight backout. We'll need both that and a "proper" fix, and so a straight backout is easier to deal with.

Comment 13

[Franck (Wip)]

(In reply to Ray Lin[:ralin] from comment #2)
> Hi Franck,
> 
> Are you using customize Window theme(or 3rd-party theme)? Since we do have
> dark circle around for default theme[0]. The difference is intended to
> ensure display correctness in non-default theme like high-contrast theme. 

Hi Ray,

For the record I am on Windows 7 using the default theme.
But, by reading the content of this bug since you asked the question, I suppose it is no longer relevant.

Comment 14

[Ray Lin[:ralin]]

(In reply to :Gijs from comment #12)
> so a straight backout is easier to deal with.

I think aside from bug 1325591, we might need to back out bug 1332807 as well because it's a followup. 

btw, sorry that I was offsite today and have no Windows environment around, I'll attach a patch and continue debugging tomorrow.

Comment 15

[Ray Lin[:ralin]]

Sorry for late

> and so a straight backout is easier to deal with.
I'm not familiar with backout process, could you help me back out bug 1332807 and 1325591, in order not to affect default theme. Thanks.

I spent some time to debug or workaround this problem, but still not close enough to come out with a proper fix for this. And I'm afraid that I could not devote on this recently as my queue is quite full for form autofill. If possible, I'd like to file a bug that address media query with e10s problem, and see if someone who knows better about it can help with the problem.

> Perhaps we don't have the right permissions with the sandboxed child process to know these things, or perhaps something's broken in CSS code, 
It's weird that I've seen some precedents using media query in toolkit, don't see any different would lead to this case...

Comment 16

[Ray Lin[:ralin]]

s/1325591/bug 1325591/

Comment 17

[:Gijs (he/him)]

I asked Liz in bug 1325591.

Comment 18

[:Gijs (he/him)]

Attachment: Bug 1350325 - back out changeset 195972ebe7a1 and 27346172ee35 (bug 1325591 and bug 1332807) for causing worse problems for the default theme,

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

Comment 19

[:Gijs (he/him)]

Comment on attachment 8852555 [details]
Bug 1350325 - back out changeset 195972ebe7a1 and 27346172ee35 (bug 1325591 and bug 1332807) for causing worse problems for the default theme,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1325591 and bug 1332807
[User impact if declined]: can't see the play icon on HTML5 video
[Is this code covered by automated tests?]: no, styling-only change
[Has the fix been verified in Nightly?]: not yet, but is a straight backout
[Needs manual test from QE? If yes, steps to reproduce]: check for play icon on HTML5 video in non-high-contrast themes on Windows when e10s is turned on
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: straight backout
[String changes made/needed]: no

Comment 20

[Ray Lin[:ralin]]

Comment on attachment 8852555 [details]
Bug 1350325 - back out changeset 195972ebe7a1 and 27346172ee35 (bug 1325591 and bug 1332807) for causing worse problems for the default theme,

https://reviewboard.mozilla.org/r/124748/#review127258

Thank you so much for your help on this bug...

Comment 21

[:Gijs (he/him)]

Pulsebot's dead, but: https://hg.mozilla.org/integration/autoland/rev/1a49f602f5d1

Comment 22

[Ray Lin[:ralin]]

Thanks Gijs :)

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1a49f602f5d1

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8852555 [details]
Bug 1350325 - back out changeset 195972ebe7a1 and 27346172ee35 (bug 1325591 and bug 1332807) for causing worse problems for the default theme,

Regression from 53, let's back out for aurora and beta.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fe8f22399ef3
https://hg.mozilla.org/releases/mozilla-beta/rev/5dad3edd4df4

Comment 26

[Andrei Vaida [:avaida]]

Flagging this for manual testing, str in Comment 0, additional instructions in Comment 19.

Comment 27

[Bogdan Maris, Desktop Test Engineering]

I reproduced the initial issue on Firefox 53 beta 5, verified that the issue is fixed using Firefox 53 beta 9, latest Developer Edition 54.0a2 and latest Nightly 55.0a1 across platforms (Windows 10 64bit, macOS 10.12.2 and Ubuntu 16.04 32bit).