Media Controls: delegate 'volumechange' and 'focusin' handling to an EventListener.

Media Controls: delegate 'volumechange' and 'focusin' handling to an EventListener.

MediaControls has an EventListener handling some HTMLMediaElement events
in order for the media element to no longer depends on MediaControls.

BUG=662761
Committed: https://crrev.com/15099fe2bbea4d7a205c0ef6220d23cfc6a802e2
Cr-Commit-Position: refs/heads/master@{#432053}

[mlamouri (slow - plz ping), 2016-11-06 23:28:33]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-06 23:28:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2016-11-06 23:29:41]

mlamouri@chromium.org changed reviewers:
+ foolip@chromium.org

[mlamouri (slow - plz ping), 2016-11-07 00:26:06]

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1093: updateVolume();
FYI, I couldn't find what that was for. I did some manual tests and everything
is fine. If there are no automated tests failing and you have no idea why that
was here, I would be in favour of removing.

[commit-bot: I haz the power, 2016-11-07 00:36:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 00:36:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[foolip, 2016-11-08 10:28:29]

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1093: updateVolume();
On 2016/11/07 00:26:05, mlamouri (slow) wrote:
> FYI, I couldn't find what that was for. I did some manual tests and everything
> is fine. If there are no automated tests failing and you have no idea why that
> was here, I would be in favour of removing.

https://chromium.googlesource.com/chromium/src/+/1f8312e78f2cb0b629dac384ae2b...
doesn't provide much of a clue, except that it came before the muted content
attribute.

If you remove this and use the content attribute, is there another call to
updateVolume() somewhere? Seem like without it things would go wrong.

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:492:
invalidate(m_muteButton);
Did you mean to remove this? Some bots are sad, related?

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:701:
m_volumeSlider->setVolume(mediaElement().muted() ? 0
Does mediaElement().effectiveMediaVolume() work?

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
(right):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:16:
m_mediaControls->m_mediaElement->addEventListener(
Can you test what happens with this code?

<video></video>
<script>
var video = document.querySelector('video');
video.addEventListener('volumechange', function(event) {
  event.stopImmediatePropagation();
});
video.controls = true;
</script>

[mlamouri (slow - plz ping), 2016-11-11 05:29:17]

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1093: updateVolume();
On 2016/11/08 at 10:28:29, foolip wrote:
> On 2016/11/07 00:26:05, mlamouri (slow) wrote:
> > FYI, I couldn't find what that was for. I did some manual tests and
everything
> > is fine. If there are no automated tests failing and you have no idea why
that
> > was here, I would be in favour of removing.
> 
>
https://chromium.googlesource.com/chromium/src/+/1f8312e78f2cb0b629dac384ae2b...
doesn't provide much of a clue, except that it came before the muted content
attribute.
> 
> If you remove this and use the content attribute, is there another call to
updateVolume() somewhere? Seem like without it things would go wrong.

I've added a test for this. It sounds fine. Let me know if I missed something.

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:492:
invalidate(m_muteButton);
On 2016/11/08 at 10:28:29, foolip wrote:
> Did you mean to remove this? Some bots are sad, related?

I don't think it's needed. Apart that it would allow the mute button to be
invalidated when the volume changed, it doesn't seem needed. I changed the mute
button to set it style (creating invalidation) based on muted and volume state
to match the expected rendering.

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:701:
m_volumeSlider->setVolume(mediaElement().muted() ? 0
On 2016/11/08 at 10:28:29, foolip wrote:
> Does mediaElement().effectiveMediaVolume() work?

I did that initially but given that in the long run, I want to make
MediaControls use regular web exposed things, I moved back to this. I'm happy to
change if you prefer.

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
(right):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:16:
m_mediaControls->m_mediaElement->addEventListener(
On 2016/11/08 at 10:28:29, foolip wrote:
> Can you test what happens with this code?
> 
> <video></video>
> <script>
> var video = document.querySelector('video');
> video.addEventListener('volumechange', function(event) {
>   event.stopImmediatePropagation();
> });
> video.controls = true;
> </script>

Added. Looks okay.

[mlamouri (slow - plz ping), 2016-11-11 05:33:40]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 05:33:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 08:36:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 08:37:00]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-11-11 09:50:20]

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1093: updateVolume();
On 2016/11/11 05:29:17, mlamouri (slow) wrote:
> On 2016/11/08 at 10:28:29, foolip wrote:
> > On 2016/11/07 00:26:05, mlamouri (slow) wrote:
> > > FYI, I couldn't find what that was for. I did some manual tests and
> everything
> > > is fine. If there are no automated tests failing and you have no idea why
> that
> > > was here, I would be in favour of removing.
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/1f8312e78f2cb0b629dac384ae2b...
> doesn't provide much of a clue, except that it came before the muted content
> attribute.
> > 
> > If you remove this and use the content attribute, is there another call to
> updateVolume() somewhere? Seem like without it things would go wrong.
> 
> I've added a test for this. It sounds fine. Let me know if I missed something.

Do you know why it works? It could be kind of accidental and therefore fragile.
The test should catch any regression, but understanding is nice, and I don't :)

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:492:
invalidate(m_muteButton);
On 2016/11/11 05:29:17, mlamouri (slow) wrote:
> On 2016/11/08 at 10:28:29, foolip wrote:
> > Did you mean to remove this? Some bots are sad, related?
> 
> I don't think it's needed. Apart that it would allow the mute button to be
> invalidated when the volume changed, it doesn't seem needed. I changed the
mute
> button to set it style (creating invalidation) based on muted and volume state
> to match the expected rendering.

Acknowledged.

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:701:
m_volumeSlider->setVolume(mediaElement().muted() ? 0
On 2016/11/11 05:29:17, mlamouri (slow) wrote:
> On 2016/11/08 at 10:28:29, foolip wrote:
> > Does mediaElement().effectiveMediaVolume() work?
> 
> I did that initially but given that in the long run, I want to make
> MediaControls use regular web exposed things, I moved back to this. I'm happy
to
> change if you prefer.

That's fine.

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
(right):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:16:
m_mediaControls->m_mediaElement->addEventListener(
On 2016/11/11 05:29:17, mlamouri (slow) wrote:
> On 2016/11/08 at 10:28:29, foolip wrote:
> > Can you test what happens with this code?
> > 
> > <video></video>
> > <script>
> > var video = document.querySelector('video');
> > video.addEventListener('volumechange', function(event) {
> >   event.stopImmediatePropagation();
> > });
> > video.controls = true;
> > </script>
> 
> Added. Looks okay.

Surprising. Do you know why it works, are internal event handlers always handled
first?

[mlamouri (slow - plz ping), 2016-11-11 19:13:28]

Just answered to one of the two comments (no more time).

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
(right):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:16:
m_mediaControls->m_mediaElement->addEventListener(
On 2016/11/11 at 09:50:20, foolip wrote:
> On 2016/11/11 05:29:17, mlamouri (slow) wrote:
> > On 2016/11/08 at 10:28:29, foolip wrote:
> > > Can you test what happens with this code?
> > > 
> > > <video></video>
> > > <script>
> > > var video = document.querySelector('video');
> > > video.addEventListener('volumechange', function(event) {
> > >   event.stopImmediatePropagation();
> > > });
> > > video.controls = true;
> > > </script>
> > 
> > Added. Looks okay.
> 
> Surprising. Do you know why it works, are internal event handlers always
handled first?

Hmm, I think it's because we eagerly create the controls when an element is
added to the DOM even if it doesn't have a controls attribute. I was able to
write a test case where things didn't work out very well.

I'm not super familiar with event propagation but would handling the event on
the capture phase highly reduce the chances of this happening?

In long run (outside of this CL), I see two ways we could tackle this:
1. we suppose that a website blocking such events probably use its own control
and simply ignore the problem (I wonder what would happen on Safari because they
have controls in JS IIRC)
2. we add a new type of EventListener in our internal code like to make sure
they are not blocked by content event listeners

I'm in favour of 2 because we had this problem in a few situations and if we
want to reduce internal plumbing to handle certain things, we need to make sure
they are reliable. I will talk to some people to see what we can do. Though,
what do you think?

[mlamouri (slow - plz ping), 2016-11-13 06:15:06]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[mlamouri (slow - plz ping), 2016-11-13 06:15:12]

PTAL

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1093: updateVolume();
On 2016/11/11 at 09:50:20, foolip wrote:
> On 2016/11/11 05:29:17, mlamouri (slow) wrote:
> > On 2016/11/08 at 10:28:29, foolip wrote:
> > > On 2016/11/07 00:26:05, mlamouri (slow) wrote:
> > > > FYI, I couldn't find what that was for. I did some manual tests and
> > everything
> > > > is fine. If there are no automated tests failing and you have no idea
why
> > that
> > > > was here, I would be in favour of removing.
> > > 
> > >
> >
https://chromium.googlesource.com/chromium/src/+/1f8312e78f2cb0b629dac384ae2b...
> > doesn't provide much of a clue, except that it came before the muted content
> > attribute.
> > > 
> > > If you remove this and use the content attribute, is there another call to
> > updateVolume() somewhere? Seem like without it things would go wrong.
> > 
> > I've added a test for this. It sounds fine. Let me know if I missed
something.
> 
> Do you know why it works? It could be kind of accidental and therefore
fragile. The test should catch any regression, but understanding is nice, and I
don't :)

That's because when readyState is changed, HTMLMediaElement calls reset() on the
MediaControls. Something to keep track of when using events for state change
listening but the test will help ;)

[commit-bot: I haz the power, 2016-11-13 06:15:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-13 10:07:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-13 10:07:55]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-11-14 16:07:38]

lgtm % nits

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/2477203002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1093: updateVolume();
On 2016/11/13 06:15:11, mlamouri (slow) wrote:
> On 2016/11/11 at 09:50:20, foolip wrote:
> > On 2016/11/11 05:29:17, mlamouri (slow) wrote:
> > > On 2016/11/08 at 10:28:29, foolip wrote:
> > > > On 2016/11/07 00:26:05, mlamouri (slow) wrote:
> > > > > FYI, I couldn't find what that was for. I did some manual tests and
> > > everything
> > > > > is fine. If there are no automated tests failing and you have no idea
> why
> > > that
> > > > > was here, I would be in favour of removing.
> > > > 
> > > >
> > >
>
https://chromium.googlesource.com/chromium/src/+/1f8312e78f2cb0b629dac384ae2b...
> > > doesn't provide much of a clue, except that it came before the muted
content
> > > attribute.
> > > > 
> > > > If you remove this and use the content attribute, is there another call
to
> > > updateVolume() somewhere? Seem like without it things would go wrong.
> > > 
> > > I've added a test for this. It sounds fine. Let me know if I missed
> something.
> > 
> > Do you know why it works? It could be kind of accidental and therefore
> fragile. The test should catch any regression, but understanding is nice, and
I
> don't :)
> 
> That's because when readyState is changed, HTMLMediaElement calls reset() on
the
> MediaControls. Something to keep track of when using events for state change
> listening but the test will help ;)

OK, since there's no volumechange event for this I guess it's fair that the
controls have to live by the same rules. Really the problem is that the muted
attribute should only be consulted once when the element is created, but it's
hard: https://crbug.com/350303

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/volumechange-muted-attribute.html
(right):

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/volumechange-muted-attribute.html:9:
audio.setAttribute('muted', true);
Add a TODO that this is actually wrong per HTML, https://crbug.com/350303. Maybe
we should change the spec to just set muted when the content attribute is added,
but per current spec this should do nothing at all.

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/volumechange-stopimmediatepropagation-expected.html
(right):

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/volumechange-stopimmediatepropagation-expected.html:19:
audios[i].src = findMediaFile('audio', '../content/test');
I think this'll be fine, but make sure to run the tests many times to make sure
it's not flaky. This depends on the window load event being delayed, which
should work and guarantee that the first frame is decoded, but better safe than
sorry.

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/volumechange-stopimmediatepropagation.html
(right):

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/volumechange-stopimmediatepropagation.html:3:
<title>This tests that controls are properly updated when muted content
attribute is changed.</title>
Copy-pasted title? Should say something about stopImmediatePropagation().

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:302:
setDisplayType((mediaElement().muted() || mediaElement().volume() == 0)
Why this change? Clicking it will only toggle muted, looking at this I'd expect
it to also change the volume to 1 if it was 0.

[mlamouri (slow - plz ping), 2016-11-14 23:46:29]

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/volumechange-muted-attribute.html
(right):

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/volumechange-muted-attribute.html:9:
audio.setAttribute('muted', true);
On 2016/11/14 at 16:07:37, foolip wrote:
> Add a TODO that this is actually wrong per HTML, https://crbug.com/350303.
Maybe we should change the spec to just set muted when the content attribute is
added, but per current spec this should do nothing at all.

Done.

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/volumechange-stopimmediatepropagation-expected.html
(right):

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/volumechange-stopimmediatepropagation-expected.html:19:
audios[i].src = findMediaFile('audio', '../content/test');
On 2016/11/14 at 16:07:37, foolip wrote:
> I think this'll be fine, but make sure to run the tests many times to make
sure it's not flaky. This depends on the window load event being delayed, which
should work and guarantee that the first frame is decoded, but better safe than
sorry.

Ack. Ran 100 times both tests locally and no flake.

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/controls/volumechange-stopimmediatepropagation.html
(right):

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/controls/volumechange-stopimmediatepropagation.html:3:
<title>This tests that controls are properly updated when muted content
attribute is changed.</title>
On 2016/11/14 at 16:07:37, foolip wrote:
> Copy-pasted title? Should say something about stopImmediatePropagation().

done.

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
(right):

https://codereview.chromium.org/2477203002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:302:
setDisplayType((mediaElement().muted() || mediaElement().volume() == 0)
On 2016/11/14 at 16:07:38, foolip wrote:
> Why this change? Clicking it will only toggle muted, looking at this I'd
expect it to also change the volume to 1 if it was 0.

Because we actually show a "muted" button if the volume is 0. I will add a
comment explaining. Hopefully, in the long run, this will be cleared out.

Added a comment for this.

[mlamouri (slow - plz ping), 2016-11-14 23:46:39]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-11-14 23:46:40]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2477203002/#ps80001
(title: "add comment")

[commit-bot: I haz the power, 2016-11-14 23:47:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 01:52:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-15 01:56:53]

Description was changed from

==========
Media Controls: delegate 'volumechange' and 'focusin' handling to an
EventListener.

MediaControls has an EventListener handling some HTMLMediaElement events
in order for the media element to no longer depends on MediaControls.

BUG=662761
==========

to

==========
Media Controls: delegate 'volumechange' and 'focusin' handling to an
EventListener.

MediaControls has an EventListener handling some HTMLMediaElement events
in order for the media element to no longer depends on MediaControls.

BUG=662761

Committed: https://crrev.com/15099fe2bbea4d7a205c0ef6220d23cfc6a802e2
Cr-Commit-Position: refs/heads/master@{#432053}
==========

[commit-bot: I haz the power, 2016-11-15 01:56:55]

Patchset 5 (id:??) landed as
https://crrev.com/15099fe2bbea4d7a205c0ef6220d23cfc6a802e2
Cr-Commit-Position: refs/heads/master@{#432053}