Skip to content

GitLab

T
tangled
Commit d0348b42 authored by Yuki Shiino's avatar Yuki Shiino Committed by Commit Bot
Browse files
Options

Revert "[Media Controls] Show volume slider only if user hover mute button for a while (200ms)" 

This reverts commit 305d9733.

Reason for revert: This patch seems like causing failure at:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29/6429

webkit_layout_tests on (none) GPU on Mac Run on OS: 'Mac-10.13'
Max shard duration: 0:23:45.917961 (shard #11)
Min shard duration: 0:18:50.330065 (shard #6)
Total tests: 88255
* Passed: 74827 (74757 expected, 70 unexpected)
* Skipped: 10735 (10735 expected, 0 unexpected)
* Failed: 2536 (2533 expected, >>>3 unexpected<<<)
* Flaky: 157 (157 expected, 0 unexpected)
 
Unexpected Failures:
* media/controls/volume-slider.html
* virtual/new-remote-playback-pipeline/media/controls/volume-slider.html
* virtual/video-surface-layer/media/controls/volume-slider.html

See also:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29/75687

Original change's description:
> [Media Controls] Show volume slider only if user hover mute button for a while (200ms)
> 
> Bug: 904554
> Change-Id: I3026b7c47df4f3950450b5fd721ae84bab00e382
> Reviewed-on: https://chromium-review.googlesource.com/c/1332718
> Reviewed-by: Tommy Steimel <steimel@chromium.org>
> Commit-Queue: Jazz Xu <jazzhsu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607790}

TBR=steimel@chromium.org,jazzhsu@chromium.org

Change-Id: I56471b2879938b9a5d6f142d6a812e0d175bf6a5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 904554
Reviewed-on: https://chromium-review.googlesource.com/c/1334993
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: default avatarYuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607932}
parent 70e9f7bc
Hide whitespace changes
Inline Side-by-side
Showing with 6 additions and 68 deletions
third_party/WebKit/LayoutTests/media/controls/volume-slider-accidental-hover.html deleted 100644 → 0
View file @ 70e9f7bc
<!DOCTYPE html>
<title>Accidental hovering mute button should not show volume slider.</title>
<script src='../../resources/testharness.js'></script>
<script src='../../resources/testharnessreport.js'></script>
<script src='../media-controls.js'></script>
<video controls width="400"></video>
<script>
async_test(t => {
const video = document.querySelector('video');
video.src = '../content/test.ogv';
enableTestMode(video);
video.onloadedmetadata = t.step_func(() => {
const muteBtn = muteButton(video);
const currentTime = currentTimeElement(video);
assert_false(isVolumeSliderOpen(video), "volume slider should start closed");
hoverOverControl(muteBtn, t.step_func(() => {
assert_false(isVolumeSliderOpen(video), "volume slider should not open yet");
hoverOverControl(currentTime, t.step_func_done(() => {
assert_false(isVolumeSliderOpen(video), "volume slider should not open if its accidental pass through");
}));
}));
});
});
</script>
third_party/WebKit/LayoutTests/media/controls/volume-slider.html
View file @ d0348b42
......@@ -57,7 +57,7 @@ function testMediaElement(t, element) {
// Hovering over the mute button before preloading should not open the volume
// slider.
hoverMuteButtonAndWaitForVolumeSliderTimeout(element, t.step_func(() => {
hoverOverControl(muteBtn, t.step_func(() => {
assert_false(isVolumeSliderOpen(element), element.id + ': volume slider should not open before metadata load');
// Move the cursor away from the mute button and start testing the loaded
......@@ -72,7 +72,7 @@ function testMediaElement(t, element) {
// Hovering over the mute button after preloading should open the volume
// slider.
hoverMuteButtonAndWaitForVolumeSliderTimeout(element, t.step_func(() => {
hoverOverControl(muteBtn, t.step_func(() => {
assert_true(isVolumeSliderOpen(element), element.id + ': volume slider should open when hovering the mute button');
// Moving the cursor onto the volume slider should not hide the volume slider.
......
third_party/WebKit/LayoutTests/media/media-controls.js
View file @ d0348b42
......@@ -427,17 +427,6 @@ function runAfterDoubleTapTimerFired(func) {
setTimeout(func, doubleTapTimeoutMs);
}
function runAfterVolumeSliderShowed(func) {
// 200ms timer plus 100ms slack.
const volumeSliderTimeoutMs = 200 + 100;
setTimeout(func, volumeSliderTimeoutMs)
}
function hoverMuteButtonAndWaitForVolumeSliderTimeout(video, func) {
const muteBtn = muteButton(video);
hoverOverControl(muteBtn, runAfterVolumeSliderShowed(func));
}
function hasEnabledFullscreenButton(element) {
var button = fullscreenButton(element);
return !button.disabled && button.style.display != "none";
......
third_party/blink/renderer/modules/media_controls/media_controls_impl.cc
View file @ d0348b42
......@@ -136,10 +136,6 @@ const char kImmersiveModeCSSClass[] = "immersive-mode";
// The delay between two taps to be recognized as a double tap gesture.
constexpr WTF::TimeDelta kDoubleTapDelay = TimeDelta::FromMilliseconds(300);
// The time user have to hover on mute button to show volume slider.
constexpr WTF::TimeDelta kTimeToShowVolumeSlider =
TimeDelta::FromMilliseconds(200);
// The number of seconds to jump when double tapping.
constexpr int kNumberOfSecondsToJump = 10;
......@@ -399,11 +395,7 @@ MediaControlsImpl::MediaControlsImpl(HTMLMediaElement& media_element)
tap_timer_(
media_element.GetDocument().GetTaskRunner(TaskType::kInternalMedia),
this,
&MediaControlsImpl::TapTimerFired),
volume_slider_wanted_timer_(
media_element.GetDocument().GetTaskRunner(TaskType::kInternalMedia),
this,
&MediaControlsImpl::VolumeSliderWantedTimerFired) {
&MediaControlsImpl::TapTimerFired) {
// On touch devices, start with the assumption that the user will interact via
// touch events.
Settings* settings = media_element.GetDocument().GetSettings();
......@@ -2170,24 +2162,14 @@ void MediaControlsImpl::StartHideMediaControlsIfNecessary() {
StartHideMediaControlsTimer();
}
void MediaControlsImpl::VolumeSliderWantedTimerFired(TimerBase*) {
volume_slider_->OpenSlider();
}
void MediaControlsImpl::OpenVolumeSliderIfNecessary() {
if (ShouldOpenVolumeSlider() && !volume_slider_wanted_timer_.IsActive()) {
volume_slider_wanted_timer_.StartOneShot(kTimeToShowVolumeSlider,
FROM_HERE);
}
if (ShouldOpenVolumeSlider())
volume_slider_->OpenSlider();
}
void MediaControlsImpl::CloseVolumeSliderIfNecessary() {
if (ShouldCloseVolumeSlider()) {
if (ShouldCloseVolumeSlider())
volume_slider_->CloseSlider();
if (volume_slider_wanted_timer_.IsActive())
volume_slider_wanted_timer_.Stop();
}
}
bool MediaControlsImpl::ShouldOpenVolumeSlider() const {
......
third_party/blink/renderer/modules/media_controls/media_controls_impl.h
View file @ d0348b42
......@@ -142,7 +142,6 @@ class MODULES_EXPORT MediaControlsImpl final : public HTMLDivElement,
void ToggleOverflowMenu();
bool OverflowMenuVisible();
void VolumeSliderWantedTimerFired(TimerBase*);
void OpenVolumeSliderIfNecessary();
void CloseVolumeSliderIfNecessary();
......@@ -416,10 +415,6 @@ class MODULES_EXPORT MediaControlsImpl final : public HTMLDivElement,
TaskRunnerTimer<MediaControlsImpl> tap_timer_;
bool is_paused_for_double_tap_ = false;
// Timer to delay showing the volume slider to avoid accidental triggering
// of the slider
TaskRunnerTimer<MediaControlsImpl> volume_slider_wanted_timer_;
bool is_test_mode_ = false;
};
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment