Bug 85990

Summary: Volume slider needs to be displayed below the mute button
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Victor Carbune <vcarbune>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, d-r, eric.carlson, feature-media-reviews, macpherson, menard, mrobinson, naginenis, pnormand, rakuco, tmpsantos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82124    
Attachments:
Description Flags
Preliminary patch
none
Added back custom rendering
none
Updated patch
none
Updated patch
none
Rebased patch
none
Patch for landing
none
PaPatch
none
Another rebase
none
Patch for landing none

Description Victor Carbune 2012-05-09 06:37:09 PDT
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
Comment 1 Victor Carbune 2012-05-09 06:46:48 PDT
Created attachment 140931 [details]
Preliminary patch
Comment 2 Build Bot 2012-05-09 07:02:25 PDT
Comment on attachment 140931 [details]
Preliminary patch

Attachment 140931 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12645690
Comment 3 Eric Carlson 2012-05-09 09:24:46 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2012-05-09 09:31:45 PDT
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.
Comment 5 Victor Carbune 2012-05-09 16:35:41 PDT
Created attachment 141049 [details]
Added back custom rendering
Comment 6 Victor Carbune 2012-05-09 16:51:05 PDT
(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.
Comment 7 Victor Carbune 2012-05-09 16:52:40 PDT
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.
Comment 8 Victor Carbune 2012-05-10 06:22:30 PDT
Created attachment 141160 [details]
Updated patch
Comment 9 Eric Carlson 2012-05-10 10:15:34 PDT
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.
Comment 10 Victor Carbune 2012-05-10 11:24:50 PDT
Created attachment 141205 [details]
Updated patch
Comment 11 Dimitri Glazkov (Google) 2012-05-10 11:29:57 PDT
Comment on attachment 141205 [details]
Updated patch

ok.
Comment 12 Victor Carbune 2012-05-10 11:53:37 PDT
Created attachment 141216 [details]
Rebased patch
Comment 13 Victor Carbune 2012-05-10 12:00:11 PDT
Comment on attachment 141216 [details]
Rebased patch

Bad rebase, sorry.
Comment 14 Victor Carbune 2012-05-10 12:48:36 PDT
Created attachment 141230 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-05-10 14:16:30 PDT
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
Comment 16 Victor Carbune 2012-05-10 14:29:55 PDT
Created attachment 141257 [details]
PaPatch

Seems I wrote 89950 instead of 85990 in test expectations. Corrected.
Comment 17 Victor Carbune 2012-05-10 14:41:34 PDT
Created attachment 141263 [details]
Another rebase

I'm not sure why the CQ fails to apply this patch
Comment 18 WebKit Review Bot 2012-05-13 11:01:41 PDT
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
Comment 19 Victor Carbune 2012-05-13 11:19:38 PDT
Created attachment 141609 [details]
Patch for landing
Comment 20 WebKit Review Bot 2012-05-13 11:20:07 PDT
Comment on attachment 141609 [details]
Patch for landing

Rejecting attachment 141609 [details] from commit-queue.

victor@rosedu.org 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.
Comment 21 Dimitri Glazkov (Google) 2012-05-13 11:33:31 PDT
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
Comment 22 WebKit Review Bot 2012-05-13 12:11:06 PDT
Comment on attachment 141609 [details]
Patch for landing

Clearing flags on attachment: 141609

Committed r116900: <http://trac.webkit.org/changeset/116900>
Comment 23 WebKit Review Bot 2012-05-13 12:11:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Raphael Kubo da Costa (:rakuco) 2012-05-13 14:45:04 PDT
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.
Comment 25 Philippe Normand 2012-05-13 18:50:00 PDT
(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
Comment 26 Victor Carbune 2012-05-14 04:59:31 PDT
(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.
Comment 27 Victor Carbune 2012-05-27 12:11:34 PDT
(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)