Bug 85990 – Volume slider needs to be displayed below the mute button

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Victor Carbune]

When the controls are very close to the top of the page,
the volume slider should be displayed below the mute button.

http://code.google.com/p/chromium/issues/detail?id=126385

[Victor Carbune]

Created attachment 140931 [details]
Preliminary patch

[Build Bot]

Comment on attachment 140931 [details]
Preliminary patch

Attachment 140931 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12645690

[Eric Carlson]

Comment on attachment 140931 [details]
Preliminary patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140931&action=review

> Source/WebCore/html/shadow/MediaControlElements.cpp:375
> +void MediaControlVolumeSliderContainerElement::displayBelowMuteButton(bool status)
> +{
> +    DEFINE_STATIC_LOCAL(AtomicString, classAttribute, ("class"));
> +    DEFINE_STATIC_LOCAL(AtomicString, classAttributeValue, ("webkit-media-controls-volume-slider-below"));
> +    ExceptionCode ec;
> +
> +    setAttribute(classAttribute, classAttributeValue, ec);
> +    setNeedsStyleRecalc();
> +}

Don't you need to do something with "status"?

> Source/WebCore/html/shadow/MediaControlElements.cpp:380
> +RenderObject* MediaControlVolumeSliderContainerElement::createRenderer(RenderArena *arena, RenderStyle *style)
> +{
> +    return new (arena) RenderMediaVolumeSliderContainer(this);
> +}

"style" isn't used so this will fail to compile on Mac/Windows.

[Dimitri Glazkov (Google)]

Comment on attachment 140931 [details]
Preliminary patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140931&action=review

I don't like this. We shouldn't be mutating DOM inside layout.

> Source/WebCore/css/mediaControlsChromium.css:178
> +::-webkit-media-controls * div[class="webkit-media-controls-volume-slider-below"] {

Why do you need the * selector there?

> Source/WebCore/html/shadow/MediaControlElements.cpp:351
> +        slider->displayBelowMuteButton(true);

Mutating DOM inside layout seems like a hideous violation of layering to me.

[Victor Carbune]

Created attachment 141049 [details]
Added back custom rendering

[Victor Carbune]

(In reply to comment #4)
> (From update of attachment 140931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140931&action=review
> 
> I don't like this. We shouldn't be mutating DOM inside layout.
> 
> > Source/WebCore/css/mediaControlsChromium.css:178
> > +::-webkit-media-controls * div[class="webkit-media-controls-volume-slider-below"] {
> 
> Why do you need the * selector there?
The mute button and volume slider are in the same <div> container (and not direct children of the panel element.

I suggest we keep this for two reasons:
i) we can let css position the slider within the <div> container in most of the cases
ii) within the layout() method the positioning is done only on the y-axis, when it's needed to be rendered below.

> > Source/WebCore/html/shadow/MediaControlElements.cpp:351
> > +        slider->displayBelowMuteButton(true);
> 
> Mutating DOM inside layout seems like a hideous violation of layering to me.
It was a possible alternative that I initially thought it deserves some attention. My bad.

[Victor Carbune]

Comment on attachment 141049 [details]
Added back custom rendering

I need to run and see how this looks on a mac machine before we can check this in.

[Victor Carbune]

Created attachment 141160 [details]
Updated patch

[Eric Carlson]

Comment on attachment 141160 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141160&action=review

This looks fine to me modulo the minor comments, but it would probably be good to get a nod from dglazkov as well since he had strong feelings about the previous version.

> Source/WebCore/ChangeLog:15
> +        Changed positioning of the slider to absolute, otherwise it is possible to position it from the layout() method.

Nit: do you mean "otherwise it is *not* possible"?

> Source/WebCore/html/shadow/MediaControlElements.cpp:263
> +    // Setting the display:none property on controls is not working
> +    // because localToAbsolute is behaving differently(?!)
> +
> +    // startTimer();

I think it would be better to just remove the comment and code. You and I won't forget about it just because it isn't here :-(

> LayoutTests/media/media-volume-slider-rendered-below.html:17
> +        function test() {

Nit: a function's opening brace should be on its own line.

> LayoutTests/media/media-volume-slider-rendered-below.html:29
> +                    consoleWrite("ERROR: unable to get controls coordinates.");
> +
> +                    layoutTestController.notifyDone();

Nit: You could simplify slightly this by using failTest().

> LayoutTests/media/media-volume-slider-rendered-below.html:48
> +        function initialize() {

Nit: the brace should be on a new line.

[Victor Carbune]

Created attachment 141205 [details]
Updated patch

[Dimitri Glazkov (Google)]

Comment on attachment 141205 [details]
Updated patch

ok.

[Victor Carbune]

Created attachment 141216 [details]
Rebased patch

[Victor Carbune]

Comment on attachment 141216 [details]
Rebased patch

Bad rebase, sorry.

[Victor Carbune]

Created attachment 141230 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141230 [details]
Patch for landing

Rejecting attachment 141230 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
controls-strict-expected.txt
patching file LayoutTests/platform/mac/media/video-controls-rendering-expected.txt
patching file LayoutTests/platform/mac/media/video-display-toggle-expected.txt
patching file LayoutTests/platform/mac/media/video-playing-and-pause-expected.txt
patching file LayoutTests/platform/mac/test_expectations.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Carls..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12648966

[Victor Carbune]

Created attachment 141257 [details]
PaPatch

Seems I wrote 89950 instead of 85990 in test expectations. Corrected.

[Victor Carbune]

Created attachment 141263 [details]
Another rebase

I'm not sure why the CQ fails to apply this patch

[WebKit Review Bot]

Comment on attachment 141263 [details]
Another rebase

Rejecting attachment 141263 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
file LayoutTests/platform/mac/media/video-controls-rendering-expected.txt
patching file LayoutTests/platform/mac/media/video-display-toggle-expected.txt
patching file LayoutTests/platform/mac/media/video-playing-and-pause-expected.txt
patching file LayoutTests/platform/mac/test_expectations.txt
Hunk #1 succeeded at 204 with fuzz 1.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Carls..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12678663

[Victor Carbune]

Created attachment 141609 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141609 [details]
Patch for landing

Rejecting attachment 141609 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Dimitri Glazkov (Google)]

Comment on attachment 141609 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=141609&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

This will be filled in by webkit-patch land-safely (but only if you have an actual active r+ on the bug). Don't let it be cleared by webkit-patch upload

[WebKit Review Bot]

Comment on attachment 141609 [details]
Patch for landing

Clearing flags on attachment: 141609

Committed r116900: <http://trac.webkit.org/changeset/116900>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

This patch caused the EFL bots to fail media/video-controls-rendering-toggle-display-none.html and media/video-controls-toggling.html, I've skipped them for now. It looks like the commit has caused failures for GTK+ as well.

[Philippe Normand]

(In reply to comment #24)
> This patch caused the EFL bots to fail media/video-controls-rendering-toggle-display-none.html and media/video-controls-toggling.html, I've skipped them for now. It looks like the commit has caused failures for GTK+ as well.

See bug 86328

[Victor Carbune]

(In reply to comment #24)
> This patch caused the EFL bots to fail media/video-controls-rendering-toggle-display-none.html and media/video-controls-toggling.html, I've skipped them for now. It looks like the commit has caused failures for GTK+ as well.

This is, unfortunately, expected; that's why I have marked the tests as failing in Mac and Chromium at this time.

Seems that after toggling "display:none", localToAbsolute returns different coordinates. I'm trying to find out what's the cause of this.

Thanks for marking them accordingly Raphael, Philippe.

[Victor Carbune]

(In reply to comment #26)
> (In reply to comment #24)
> > This patch caused the EFL bots to fail media/video-controls-rendering-toggle-display-none.html and media/video-controls-toggling.html, I've skipped them for now. It looks like the commit has caused failures for GTK+ as well.
> 
> This is, unfortunately, expected; that's why I have marked the tests as failing in Mac and Chromium at this time.
> 
> Seems that after toggling "display:none", localToAbsolute returns different coordinates. I'm trying to find out what's the cause of this.
I tried figuring out what's happening there, but this seems to be non-trivial. The easiest solution I could come with is to hide the volume slider each time the controls are hidden (by JS or fadeout).

> Thanks for marking them accordingly Raphael, Philippe.
I have un-marked them in the same patch containing the changes mentioned above (https://bugs.webkit.org/show_bug.cgi?id=87591)