Bug 85990 - Volume slider needs to be displayed below the mute button
Summary: Volume slider needs to be displayed below the mute button
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Victor Carbune
URL:
Keywords:
Depends on:
Blocks: 82124
  Show dependency treegraph
 
Reported: 2012-05-09 06:37 PDT by Victor Carbune
Modified: 2012-05-27 12:11 PDT (History)
13 users (show)

See Also:


Attachments
Preliminary patch (4.92 KB, patch)
2012-05-09 06:46 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Added back custom rendering (8.86 KB, patch)
2012-05-09 16:35 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Updated patch (20.11 KB, patch)
2012-05-10 06:22 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Updated patch (20.05 KB, patch)
2012-05-10 11:24 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Rebased patch (18.37 KB, patch)
2012-05-10 11:53 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Patch for landing (21.22 KB, patch)
2012-05-10 12:48 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
PaPatch (21.22 KB, patch)
2012-05-10 14:29 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Another rebase (20.64 KB, patch)
2012-05-10 14:41 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Patch for landing (20.02 KB, patch)
2012-05-13 11:19 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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)