Closed
Bug 1350325
Opened 8 years ago
Closed 8 years ago
video player's play button overlay is barely visible
Categories
(Toolkit :: Video/Audio Controls, defect)
Tracking
()
VERIFIED
FIXED
mozilla55
People
(Reporter: wip.the.gruik, Assigned: Gijs)
References
Details
Attachments
(2 files)
19.13 KB,
image/jpeg
|
Details | |
59 bytes,
text/x-review-board-request
|
ralin
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
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.
Reporter | ||
Comment 1•8 years ago
|
||
Blocks: 1271765
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Component: General → Video/Audio Controls
Ever confirmed: true
OS: Windows 7 → All
Product: Firefox → Toolkit
Hardware: x86_64 → All
Comment 2•8 years ago
|
||
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
Flags: needinfo?(wip.the.gruik)
(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•8 years ago
|
||
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....
Flags: needinfo?(wip.the.gruik)
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
(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?
Flags: needinfo?(ralin)
Assignee | ||
Comment 7•8 years ago
|
||
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).
Assignee | ||
Comment 8•8 years ago
|
||
(It's still a (separate) bug that the media query doesn't work, though.)
Comment 9•8 years ago
|
||
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.
Flags: needinfo?(ralin)
Assignee | ||
Comment 10•8 years ago
|
||
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.
Blocks: 1325591
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
Comment 11•8 years ago
|
||
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
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 12•8 years ago
|
||
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 13•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
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...
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
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
Attachment #8852555 -
Flags: approval-mozilla-beta?
Attachment #8852555 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment 20•8 years ago
|
||
mozreview-review |
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...
Attachment #8852555 -
Flags: review?(ralin) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Pulsebot's dead, but: https://hg.mozilla.org/integration/autoland/rev/1a49f602f5d1
Comment 23•8 years ago
|
||
backout bugherder |
https://hg.mozilla.org/mozilla-central/rev/1a49f602f5d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 24•8 years ago
|
||
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.
Attachment #8852555 -
Flags: approval-mozilla-beta?
Attachment #8852555 -
Flags: approval-mozilla-beta+
Attachment #8852555 -
Flags: approval-mozilla-aurora?
Attachment #8852555 -
Flags: approval-mozilla-aurora+
Comment 25•8 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe8f22399ef3 https://hg.mozilla.org/releases/mozilla-beta/rev/5dad3edd4df4
Comment 26•8 years ago
|
||
Flagging this for manual testing, str in Comment 0, additional instructions in Comment 19.
Flags: qe-verify+
Comment 27•8 years ago
|
||
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•