Closed
Bug 1325591
Opened 8 years ago
Closed 8 years ago
Videos are not displayed correctly with high contrast themes
Categories
(Toolkit :: Video/Audio Controls, defect)
Tracking
()
VERIFIED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | --- | wontfix |
firefox54 | --- | wontfix |
firefox55 | --- | verified |
People
(Reporter: hyacoub, Assigned: ralin)
References
Details
(Keywords: access, regression)
Attachments
(7 files)
[Affected versions]:
Nightly 53.0a1
[Affected platforms]:
All platforms: Windows 10 x 64, Mac OS X 10.11
[Steps to reproduce]:
1. Activate a high contrast theme.
- Windows: Go to Personalize> Themes> Theme Settings and activate a High Contrast Theme.
- Mac: Go to Accessibility and change the Display contrast.
2. Launch Nightly 53.0a1 with a new profile and then open this link https://people-mozilla.org/~ralin/vtt/.
3. Observe the video.
[Expected result]:
The video is correctly displayed.
[Actual result]:
The media control buttons are not displayed.
[Note]:
On Mac OS X 10.11 - Only the buffered zone is not displayed with high contrast theme.
Comment 1•8 years ago
|
||
The content area used to display properly in HCM before playing. Not sure if that's a regression from the new controls or not. It does display correctly once you play the video in HCM (even on current trunk) but obviously that's hard to do if you can't see that there's a video there...
Does this work better/worse on videos with a working poster attribute?
The controls never worked well in HCM, unfortunately, but if we can fix that in one go here, that'd be awesome.
Reporter | ||
Comment 2•8 years ago
|
||
regression-window |
Using the mozregression tool I got to this regression window:
Last good revision: df34cd3d3f88b71fc1840cecdbd1e02a0f3b59be
First bad revision: ad69b78ed3e02f9c3255fbab700d049f0debb325
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=df34cd3d3f88b71fc1840cecdbd1e02a0f3b59be&tochange=ad69b78ed3e02f9c3255fbab700d049f0debb325
INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1243720
Comment 3•8 years ago
|
||
(In reply to Hani Yacoub from comment #2)
> Using the mozregression tool I got to this regression window:
>
> Last good revision: df34cd3d3f88b71fc1840cecdbd1e02a0f3b59be
> First bad revision: ad69b78ed3e02f9c3255fbab700d049f0debb325
> INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=df34cd3d3f88b71fc1840cecdbd1e02a0f3b59be&tochange=ad69
> b78ed3e02f9c3255fbab700d049f0debb325
>
> INFO: Looks like the following bug has the changes which introduced the
> regression:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1243720
That looks like an e10s vs. non-e10s parity bug. What's the regression window without it, or does the content of the video show up correctly when e10s is disabled on current nightly?
Blocks: 1243720
Flags: needinfo?(hani.yacoub)
Reporter | ||
Comment 4•8 years ago
|
||
(In reply to :Gijs from comment #3)
> (In reply to Hani Yacoub from comment #2)
> > Using the mozregression tool I got to this regression window:
> >
> > Last good revision: df34cd3d3f88b71fc1840cecdbd1e02a0f3b59be
> > First bad revision: ad69b78ed3e02f9c3255fbab700d049f0debb325
> > INFO: Pushlog:
> > https://hg.mozilla.org/integration/mozilla-inbound/
> > pushloghtml?fromchange=df34cd3d3f88b71fc1840cecdbd1e02a0f3b59be&tochange=ad69
> > b78ed3e02f9c3255fbab700d049f0debb325
> >
> > INFO: Looks like the following bug has the changes which introduced the
> > regression:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1243720
>
> That looks like an e10s vs. non-e10s parity bug. What's the regression
> window without it, or does the content of the video show up correctly when
> e10s is disabled on current nightly?
The content of the video doesn't show up correctly when e10s is disabled also on current nightly.
I couldn't provide with the regression range without e10s, because I couldn't find any good reversion.
Flags: needinfo?(hani.yacoub)
Updated•8 years ago
|
status-firefox50:
--- → wontfix
status-firefox51:
--- → wontfix
status-firefox52:
--- → affected
Version: 53 Branch → 47 Branch
Comment 6•8 years ago
|
||
(In reply to Hani Yacoub from comment #4)
> I couldn't provide with the regression range without e10s, because I
> couldn't find any good reversion.
FWIW, I am surprised by this, because current beta (51) on my win10 machine, with hcm on and e10s off, shows me an image when checking http://www.quirksmode.org/html5/tests/video.html . I've been meaning to look at this again myself but not had time. I did just recheck beta, and I continue to see the video 'preview' there when hcm is on.
Updated•8 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 7•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Mike, can you please take a look at this?
I'm not sure I can be much use here. :/ According to comment 4, this problem exists with e10s disabled as well. The rest of the content area appears to understand that we're in high contrast mode, so my patch for bug 1243720 appears to be doing the right thing. which suggests to me that this is somewhere in either our widget layer, or perhaps within media or graphics? Somewhere in there, I'll bet.
Flags: needinfo?(mconley)
Comment 9•8 years ago
|
||
The content area of the video (before playing it) displaying correctly regressed with the new video controls.
As I stated in comment #1, the controls were never good in HCM (cf. bug 641587, bug 1022553 - and yes, one of those should be duped).
I'm mostly concerned about the content area though - before bug 1271765 you could see the video against the page background. Now that no longer works, so there's almost no indication the video is even there - there's just a big gap on the page.
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to :Gijs from comment #9)
> The content area of the video (before playing it) displaying correctly
> regressed with the new video controls.
I found the root cause is that there's a white background with transparency block overlay on content area before playing, and overlay turns into black block when HCM is on.
I guess we should hide the overlay **in HCM** to at least move away this regression.
---
Hi, :dao
I heard from Scott that you've mentioned some tips about HCM css selector. Do you know how to override CSS rule *in HCM* by specified pseudo class/selector?
I wonder if I could do something like:
.controlsOverlay {
background-color: rgba(255,255,255,.4);
}
.controlsOverlay:-moz-hcm {
background-color: unset;
}
Thank you :)
Flags: needinfo?(ralin) → needinfo?(dao+bmo)
Comment 11•8 years ago
|
||
(In reply to Ray Lin[:ralin] from comment #10)
> (In reply to :Gijs from comment #9)
> > The content area of the video (before playing it) displaying correctly
> > regressed with the new video controls.
> I found the root cause is that there's a white background with transparency
> block overlay on content area before playing, and overlay turns into black
> block when HCM is on.
>
> I guess we should hide the overlay **in HCM** to at least move away this
> regression.
>
> ---
> Hi, :dao
>
> I heard from Scott that you've mentioned some tips about HCM css selector.
> Do you know how to override CSS rule *in HCM* by specified pseudo
> class/selector?
>
> I wonder if I could do something like:
> .controlsOverlay {
> background-color: rgba(255,255,255,.4);
> }
>
> .controlsOverlay:-moz-hcm {
> background-color: unset;
> }
Nope, there's no such selector. There is bug 957988.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 12•8 years ago
|
||
It seems that we have no better choice but need to remove the white transparent overlay now. I've discussed with Peko about this issue offline, and she agreed with this approach.
Jared, do you it's a good idea? Thanks
Attachment #8826096 -
Flags: feedback?(jaws)
Comment 13•8 years ago
|
||
I have a number of suggestions before we give up doing this completely:
- this used to work, and we always seemed to have a fade over the <video> when it wasn't being played yet. Can we check how it used to work?
- we could try using an opaque background color for the spacer but set the spacer's opacity to 0.4, instead of using an rgba colour. This should still cross-fade in HCM, I think, though it might be with black instead of white in dark HCM themes. I think that'd be OK.
- the same, but with an SVG mask instead of setting opacity:.
- if you move the rule(s) in question into html.css or similar UA stylesheets, it might not be affected by the high contrast mode disabling all use of colours. I think in this case, given that it's a core HTML element (ie <video>) that we're talking about, that may actually make some sense, though it's possible we should use a system colour instead of white (and opacity to create the semi-transparency).
- On Windows, we could in principle use the default theme media query to only do something on the main (non-high-contrast) theme. Of course, that wouldn't cover the cases where the user has specified "override author colours". This also wouldn't help OS X's HCM, if that's also affected (I didn't think we really supported that, and I know we don't properly support Linux HCM themes (in part because they are (used to be?) poorly specified - the only thing distinguishing them from any other theme was their name :-\ )).
Comment 14•8 years ago
|
||
I'm not sure what this overlay thing is that everybody is talking about, but if it's new and non-essential to the design, I would recommend removing it to fix the regression and have something upliftable for aurora.
Fancier solutions can be researched after deploying that short-term fix. Note however that eventually the right solution is probably going to be what Boris described in bug 1022553 comment 2.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•8 years ago
|
||
Thank :Gijs and :dao for all helpful suggestions. :-)
Per suggestion comment 13, 14, I made a patch based on default media query, simply change overlay's background color to `transparent` only for Windows hcm. I'm afraid I could not think of how a desirable/accessible controls should look like now, so just fix the most concerned regression(no indication of video content area).
I suspect that we need to make current controls be the same as it used to be in hcm (yellow border around buttons...). If it make more sense for users to have it, I would like to update patch about it. If not or there's no urgent necessary, I prefer to save the controls part to other bug for a much proper solution, such as bug 1022553.
-----
Mike,
Could you help me to review this patch? thanks!
Comment hidden (mozreview-request) |
Comment 18•8 years ago
|
||
Ray, I'm a bit confused as to the scope for this bug. When I look at Fx built with your change and a high-contrast theme active, I see: http://www.evernote.com/shard/s251/sh/ae002d7b-0422-465d-a035-e35006b9fb0f/d1e1f87a182871316ed1e3dd5dc31d5e, which doesn't look very good still.
Can you show me what you intend to change with your patch?
Updated•8 years ago
|
Flags: needinfo?(ralin)
Comment 19•8 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #18)
> Ray, I'm a bit confused as to the scope for this bug. When I look at Fx
> built with your change and a high-contrast theme active, I see:
> http://www.evernote.com/shard/s251/sh/ae002d7b-0422-465d-a035-e35006b9fb0f/
> d1e1f87a182871316ed1e3dd5dc31d5e, which doesn't look very good still.
>
> Can you show me what you intend to change with your patch?
I noted in comment #1 and comment #9 that the controls were never visible in HCM. That's not a regression. Not being able to see the video is a regression. From your screenshot, it looks like the patch addresses that regression. We can fix the controls in bug 1022553.
Assignee | ||
Comment 20•8 years ago
|
||
Hi Mike,
(I forgot to attached a screenshot to indicate the regression. As shown on screenshot, the video element was covered by a black overlay, and was invisible before playing. )
I intend to fix this problem, so the expected result would be that the content region of video is visible as long as page loaded.
Thanks
Flags: needinfo?(ralin)
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8827072 [details]
Bug 1325591 - Hide overlay and clickToPlay of video in Windows high contrast theme.
https://reviewboard.mozilla.org/r/104902/#review105908
Gijs, Ray, thanks for the explanation! I'm looking forward to seeing bug 1022553 fixed, as well ;-)
::: toolkit/themes/shared/media/videocontrols.css:482
(Diff revision 1)
> [error="errorGeneric"] > [anonid="errorGeneric"] {
> display: inline;
> }
> +
> +/* For high contrast theme in Windows */
> +%ifdef XP_WIN
Yeah, we'll only get rid of this pesky ifdef with help from platform....
::: toolkit/themes/shared/media/videocontrols.css:490
(Diff revision 1)
> + background-color: transparent;
> +}
> +
> +.controlsSpacer:hover {
> + background-color: rgb(254,255,255);
> + opacity: 0.4;
nit: `.4`
Attachment #8827072 -
Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•8 years ago
|
||
Thank you Mike!
rebased and nit fixed, patch is ready to land.
Keywords: checkin-needed
Comment 24•8 years ago
|
||
There are pending issues in MozReview that are blocking Autoland from checking this in.
Keywords: checkin-needed
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> There are pending issues in MozReview that are blocking Autoland from
> checking this in.
ah, issue closed now, thanks :)
Keywords: checkin-needed
Comment 26•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6d632eefccb4
Hide overlay and clickToPlay of video in Windows high contrast theme. r=mikedeboer
Keywords: checkin-needed
Comment 27•8 years ago
|
||
Backed out for failing a11y/accessible/tests/mochitest/actions/test_media.html:
https://hg.mozilla.org/integration/autoland/rev/c779cd2ff47cf22837e6c8038cf598d51e9b501c
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6d632eefccb4fb46f2e157403a0790b916ff7e24
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=69615991&repo=autoland
[task 2017-01-17T17:44:36.995157Z] 17:44:36 INFO - TEST-START | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html
[task 2017-01-17T17:44:37.466732Z] 17:44:37 INFO - TEST-INFO | started process screentopng
[task 2017-01-17T17:44:37.886852Z] 17:44:37 INFO - TEST-INFO | screentopng: exit 0
[task 2017-01-17T17:44:37.886945Z] 17:44:37 INFO - Buffered messages logged at 17:44:37
[task 2017-01-17T17:44:37.886998Z] 17:44:37 INFO - must wait for load
[task 2017-01-17T17:44:37.888262Z] 17:44:37 INFO - TEST-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | Focus test are disabled until bug 494175 is fixed.
[task 2017-01-17T17:44:37.888415Z] 17:44:37 INFO - Invoke the 'invoke an action press at index undefined on ['button node', address: [object HTMLButtonElement], role: pushbutton, name: 'Mute', address: [xpconnect wrapped nsIAccessible]]' test { scenario #0: expected 'mousedown' event; expected 'mouseup' event; expected 'click' event; expected 'name changed' event; }
[task 2017-01-17T17:44:37.889081Z] 17:44:37 INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | No actions on the accessible for ['button node', address: [object HTMLButtonElement], role: pushbutton, name: 'Mute', address: [xpconnect wrapped nsIAccessible]]
[task 2017-01-17T17:44:37.889245Z] 17:44:37 INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | Wrong action name of the accessible for ['button node', address: [object HTMLButtonElement], role: pushbutton, name: 'Mute', address: [xpconnect wrapped nsIAccessible]]
[task 2017-01-17T17:44:37.889914Z] 17:44:37 INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | Wrong name of ['button node', address: [object HTMLButtonElement], role: pushbutton, name: 'Unmute', address: [xpconnect wrapped nsIAccessible]] on focus
[task 2017-01-17T17:44:37.890873Z] 17:44:37 INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | Test with ID = 'mousedown event handling' succeed. Event 'mousedown' was handled.
[task 2017-01-17T17:44:37.891712Z] 17:44:37 INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | Test with ID = 'mouseup event handling' succeed. Event 'mouseup' was handled.
[task 2017-01-17T17:44:37.892602Z] 17:44:37 INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | Test with ID = 'click event handling' succeed. Event 'click' was handled.
[task 2017-01-17T17:44:37.893442Z] 17:44:37 INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | Test with ID = 'name change handling' succeed. Event 'name changed' was handled.
[task 2017-01-17T17:44:37.894210Z] 17:44:37 INFO - Invoke the 'invoke an action press at index undefined on ['div node', address: [object HTMLDivElement], role: section, address: [xpconnect wrapped nsIAccessible]]' test { scenario #0: expected 'mousedown' event; expected 'mouseup' event; expected 'click' event; expected 'name changed' event; }
[task 2017-01-17T17:44:37.894406Z] 17:44:37 INFO - Buffered messages finished
[task 2017-01-17T17:44:37.895339Z] 17:44:37 INFO - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html | No actions on the accessible for ['div node', address: [object HTMLDivElement], role: section, address: [xpconnect wrapped nsIAccessible]]
[task 2017-01-17T17:44:37.895525Z] 17:44:37 INFO - actionInvoker_invoke@chrome://mochitests/content/a11y/accessible/tests/mochitest/actions.js:125:5
[task 2017-01-17T17:44:37.896157Z] 17:44:37 INFO - eventQueue_processNextInvoker@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:436:9
[task 2017-01-17T17:44:37.896581Z] 17:44:37 INFO - eventQueue_processNextInvokerInTimeout/<@chrome://mochitests/content/a11y/accessible/tests/mochitest/events.js:456:43
[task 2017-01-17T17:44:37.897255Z] 17:44:37 INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(ralin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•8 years ago
|
||
Sorry for messing this bug up, I should not have had a fix for `aria-hidden` typo here. I filed another bug for typo bug 1331835.
The a11y is green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25ef68e4f9f6cb0e691ce33c02534c1645d547a6
Flags: needinfo?(ralin)
Keywords: checkin-needed
Comment 30•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/27346172ee35
Hide overlay and clickToPlay of video in Windows high contrast theme. r=mikedeboer
Keywords: checkin-needed
Comment 31•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 32•8 years ago
|
||
Please request Aurora approval on this when you're comfortable doing so.
Flags: needinfo?(ralin)
Reporter | ||
Comment 33•8 years ago
|
||
Tested this issue on Ubuntu 16.04 x64, Windows 10 x 64, Mac OS X 10.11, and I can confirm that the video is correctly displayed on high contrast with the e10s disabled and with the e10s enable, but the Audio/video controls are still invisible in High Contrast mode.
Status: RESOLVED → VERIFIED
Comment 34•8 years ago
|
||
The patch that landed had rgb(254,255,255) but it should have been rgb(255,255,255). ralin, can you fix this?
Updated•8 years ago
|
Attachment #8826096 -
Flags: feedback?(jaws)
Assignee | ||
Comment 35•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #32)
> Please request Aurora approval on this when you're comfortable doing so.
Sorry for the late reply. The new design and its css rules are introduced in bug 1271765 which landed on 53, so it seems no necessary(or not safe) to uplift to Aurora. Thanks.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #34)
> The patch that landed had rgb(254,255,255) but it should have been
> rgb(255,255,255). ralin, can you fix this?
Ahh! I'm was so careless...that not notice this nit. Will file a new bug for this :)
Flags: needinfo?(ralin)
Comment 36•8 years ago
|
||
Ah sorry, got a bit mixed up with the original regression range pointing at bug 1243720 as the culprit.
Updated•8 years ago
|
Reporter | ||
Comment 37•8 years ago
|
||
I observed after this fix landed that when hovering the video it becomes much brighter. Please see the attached screencast.
I tried to find out the regression range and it's got me to this bug.
Last good revision: 280836582bde2dbc2f883ffb85f56cfc65c593b0
First bad revision: 27346172ee35332e28011fc4143de2bce10eaef8
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=280836582bde2dbc2f883ffb85f56cfc65c593b0&tochange=27346172ee35332e28011fc4143de2bce10eaef8
Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1325591
Was this intended with this fix of the current bug?
Flags: needinfo?(ralin)
Assignee | ||
Comment 38•8 years ago
|
||
(In reply to Hani Yacoub from comment #37)
> Created attachment 8828813 [details]
> videoFocus.mp4
>
> I observed after this fix landed that when hovering the video it becomes
> much brighter. Please see the attached screencast.
> I tried to find out the regression range and it's got me to this bug.
>
> Last good revision: 280836582bde2dbc2f883ffb85f56cfc65c593b0
> First bad revision: 27346172ee35332e28011fc4143de2bce10eaef8
> Pushlog:
> https://hg.mozilla.org/integration/autoland/
> pushloghtml?fromchange=280836582bde2dbc2f883ffb85f56cfc65c593b0&tochange=2734
> 6172ee35332e28011fc4143de2bce10eaef8
>
> Looks like the following bug has the changes which introduced the regression:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1325591
>
> Was this intended with this fix of the current bug?
I was tend to restore to the behavior we used to have in old video control, but likely breaks hover on windows. I filed a bug 1332807 for this issue, also for the incorrect color scheme mentioned by Jared at comment 34.
Flags: needinfo?(ralin)
Comment 39•8 years ago
|
||
Updating status flags based on Comment 33.
Comment 40•8 years ago
|
||
Hi Liz,
Because this fix and its followup causes bug 1350325, which affects all non-high-contrast themes (which is the vast majority of users) on Windows when e10s is turned on, Ray and I would like to back out this bug and bug 1332807 from beta, aurora and Nightly (53, 54, 55), and then reopen this bug (probably with a dep on making media queries work in e10s, but maybe we can find a way of working around this...). What's the best way of doing this?
Flags: needinfo?(lhenry)
If you could open a new bug for the backouts, that will be easiest for relman and sheriffs to track what's happening. Then we can reopen this bug as you suggest. Thanks!
Flags: needinfo?(lhenry)
Comment 42•8 years ago
|
||
Backed out from mozilla-central.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 43•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #41)
> If you could open a new bug for the backouts, that will be easiest for
> relman and sheriffs to track what's happening. Then we can reopen this bug
> as you suggest. Thanks!
I decided to use bug 1350325 for the backout, please see the approval requests there. Thanks to Guilherme for reopening this one.
Comment 44•8 years ago
|
||
Ray, can you put up a new patch combining the changes in bug 1332807 and here for re-landing, now that bug 1351676 is fixed? (and check it does work correctly in e10s hcm and non-hcm! :-) )
Flags: needinfo?(ralin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 46•8 years ago
|
||
Thank you :Gijs,
I've checked the patch in e10s(non-e10s) + hcm(non-hcm), and all work as expected. Could you help me to review this?
Flags: needinfo?(ralin)
Comment 47•8 years ago
|
||
mozreview-review |
Comment on attachment 8827072 [details]
Bug 1325591 - Hide overlay and clickToPlay of video in Windows high contrast theme.
https://reviewboard.mozilla.org/r/104902/#review136804
In fact, now that we have context-fill, can we create a `<use>` variant in the play icon that uses context-fill, and in HCM, use that icon and context-fill with `currentColor`, and use opacity to mix the semi-transparent background and the video frame? That seems like it should work in HCM, and would be better than having no visual indication the video can be played.
::: toolkit/themes/shared/media/videocontrols.css:484
(Diff revision 5)
> +
> +%ifdef XP_WIN
> +@media (-moz-windows-default-theme: 0) {
> + .controlsSpacer,
> + .clickToPlay {
> + background-color: transparent;
Nit: please remove the trailing space.
Attachment #8827072 -
Flags: review?(gijskruitbosch+bugs)
Comment 48•8 years ago
|
||
mozreview-review |
Comment on attachment 8827072 [details]
Bug 1325591 - Hide overlay and clickToPlay of video in Windows high contrast theme.
https://reviewboard.mozilla.org/r/104902/#review136808
Though, to be fair, if this is a net improvement, maybe we should take this and move the better fix to a followup bug? Up to you.
Attachment #8827072 -
Flags: review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 50•8 years ago
|
||
(In reply to :Gijs (gone until May 2, you may prefer other reviewers) from comment #47)
> In fact, now that we have context-fill, can we create a `<use>` variant in
> the play icon that uses context-fill, and in HCM, use that icon and
> context-fill with `currentColor`, and use opacity to mix the
> semi-transparent background and the video frame? That seems like it should
> work in HCM, and would be better than having no visual indication the video
> can be played.
Thanks for your suggestion :)
The image doesn't show up after those tweak. It seems HCM has more limit in content process that all the background-* style could not be applied. Perhaps <img> instead of background-image or using trick like CSS border triangle might helps this case.
> ::: toolkit/themes/shared/media/videocontrols.css:484
> (Diff revision 5)
> > +
> > +%ifdef XP_WIN
> > +@media (-moz-windows-default-theme: 0) {
> > + .controlsSpacer,
> > + .clickToPlay {
> > + background-color: transparent;
>
> Nit: please remove the trailing space.
Patch updated. I'd like to land this first and leave the rest of improvements to maybe bug 1022553.
Thanks.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 51•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/260f2ff492af
Hide overlay and clickToPlay of video in Windows high contrast theme. r=Gijs,mikedeboer
Keywords: checkin-needed
Comment 52•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Comment 53•8 years ago
|
||
Should we just let this ride the 55 train?
Assignee | ||
Comment 54•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #53)
> Should we just let this ride the 55 train?
This only works with the fix in Bug 1351676, so I think we need to make both ride 54.
Flags: needinfo?(ralin)
Updated•8 years ago
|
Updated•7 years ago
|
Flags: qe-verify+
Comment 55•7 years ago
|
||
I tried to verify this bug, but I ran into some issues. The video that is attached to the comment 0 is not working, so I used this one: https://www.iandevlin.com/html5test/webvtt/html5-video-webvtt-sample.html.
I tested using Nightly from 2017-12-22 and Firefox 55.0b12 on Windows 10 x64 and macOS 10.11.
- On Windows the issues is still reproducing even on the latest beta. On Firefox 55.0b12 the control buttons are displayed, but they are the same color as the theme color. (Example: if the theme color is black then the control buttons are displayed in black color. The same thing goes for white theme.)
- On macOS I couldn't reproduce the issue even on Nightly from 2017-12-22.
Can you please tell me if the video is the right one to verify this bug? Or is there something else that I missed?
Flags: needinfo?(ralin)
Comment 56•7 years ago
|
||
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #55)
> I tried to verify this bug, but I ran into some issues. The video that is
> attached to the comment 0 is not working, so I used this one:
> https://www.iandevlin.com/html5test/webvtt/html5-video-webvtt-sample.html.
>
> I tested using Nightly from 2017-12-22 and Firefox 55.0b12 on Windows 10 x64
> and macOS 10.11.
> - On Windows the issues is still reproducing even on the latest beta. On
> Firefox 55.0b12 the control buttons are displayed, but they are the same
> color as the theme color. (Example: if the theme color is black then the
> control buttons are displayed in black color. The same thing goes for white
> theme.)
> - On macOS I couldn't reproduce the issue even on Nightly from 2017-12-22.
>
> Can you please tell me if the video is the right one to verify this bug? Or
> is there something else that I missed?
Please read comment #1 and comment #9 and comment #33, and see also bug 1022553.
This bug isn't about the controls on the video, it's about the contents of the video and its play button overlay that appears on the initial pageload.
Comment 58•7 years ago
|
||
(In reply to :Gijs from comment #56)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #55)
> > I tried to verify this bug, but I ran into some issues. The video that is
> > attached to the comment 0 is not working, so I used this one:
> > https://www.iandevlin.com/html5test/webvtt/html5-video-webvtt-sample.html.
> >
> > I tested using Nightly from 2017-12-22 and Firefox 55.0b12 on Windows 10 x64
> > and macOS 10.11.
> > - On Windows the issues is still reproducing even on the latest beta. On
> > Firefox 55.0b12 the control buttons are displayed, but they are the same
> > color as the theme color. (Example: if the theme color is black then the
> > control buttons are displayed in black color. The same thing goes for white
> > theme.)
> > - On macOS I couldn't reproduce the issue even on Nightly from 2017-12-22.
> >
> > Can you please tell me if the video is the right one to verify this bug? Or
> > is there something else that I missed?
>
> Please read comment #1 and comment #9 and comment #33, and see also bug
> 1022553.
>
> This bug isn't about the controls on the video, it's about the contents of
> the video and its play button overlay that appears on the initial pageload.
Ups, I missed that. The thing is that the content of the video is properly displayed, but the play button overlay is missing. You can see this in the gif I attached to this comment. I tested using Firefox 55.0b12, Nightly from 2017-04-29 and using the latest Nightly on Windows 10.
Do you have any input about the situation?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 59•7 years ago
|
||
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #58)
> Created attachment 8891215 [details]
> missing play button
>
> (In reply to :Gijs from comment #56)
> > (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #55)
> > > I tried to verify this bug, but I ran into some issues. The video that is
> > > attached to the comment 0 is not working, so I used this one:
> > > https://www.iandevlin.com/html5test/webvtt/html5-video-webvtt-sample.html.
> > >
> > > I tested using Nightly from 2017-12-22 and Firefox 55.0b12 on Windows 10 x64
> > > and macOS 10.11.
> > > - On Windows the issues is still reproducing even on the latest beta. On
> > > Firefox 55.0b12 the control buttons are displayed, but they are the same
> > > color as the theme color. (Example: if the theme color is black then the
> > > control buttons are displayed in black color. The same thing goes for white
> > > theme.)
> > > - On macOS I couldn't reproduce the issue even on Nightly from 2017-12-22.
> > >
> > > Can you please tell me if the video is the right one to verify this bug? Or
> > > is there something else that I missed?
> >
> > Please read comment #1 and comment #9 and comment #33, and see also bug
> > 1022553.
> >
> > This bug isn't about the controls on the video, it's about the contents of
> > the video and its play button overlay that appears on the initial pageload.
>
> Ups, I missed that. The thing is that the content of the video is properly
> displayed, but the play button overlay is missing. You can see this in the
> gif I attached to this comment. I tested using Firefox 55.0b12, Nightly from
> 2017-04-29 and using the latest Nightly on Windows 10.
>
> Do you have any input about the situation?
Is the play button and overlay missing just in high contrast themes, or also in the default windows theme? If the former, based on:
(In reply to Pulsebot from comment #51)
> Pushed by cbook@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/260f2ff492af
> Hide overlay and clickToPlay of video in Windows high contrast theme.
> r=Gijs,mikedeboer
I think that's correct.
Otherwise, that sounds problematic.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bogdan.maris)
Comment 60•7 years ago
|
||
It's missing only from High Contrast themes, on the default windows theme the play button is displayed with no issues. (Fx55 and 56)
Flags: needinfo?(bogdan.maris) → needinfo?(gijskruitbosch+bugs)
Comment 61•7 years ago
|
||
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #60)
> Created attachment 8891588 [details]
> .gif showing the issue
>
> It's missing only from High Contrast themes, on the default windows theme
> the play button is displayed with no issues. (Fx55 and 56)
OK, I think that's sufficient to mark this as verified fixed, given that compared to comment 9 (not seeing the video at all) this is an improvement.
Status: RESOLVED → VERIFIED
Flags: needinfo?(gijskruitbosch+bugs)
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•