From 5d4c0f2c72b23cd81c6ec55dd28b8d9a92493363 Mon Sep 17 00:00:00 2001 From: "graouts@webkit.org" Date: Thu, 23 Feb 2017 01:55:05 +0000 Subject: [PATCH] [Modern Media Controls] Scrubber stops moving while scrubbing on macOS https://bugs.webkit.org/show_bug.cgi?id=168518 Reviewed by Dean Jackson. Source/WebCore: As we start to scrub, controlValueWillStartChanging() is called on ScrubberSupport and pauses the media if it's not already paused. This causes the play/pause button to change icon and for layout() to be called on MacOSInlineMediaControls. This in turns sets the .children property on the .controlsBar and eventually yields a DOM manipulation which re-inserts the scrubber's DOM hierarchy, causing stutters during user interaction. Our solution is to make the .children setter smarter about identifying that the children list hasn't changed and that no DOM invalidation is necessary. * Modules/modern-media-controls/controls/layout-node.js: (LayoutNode.prototype.set children): LayoutTests: Add assertions to check that setting children to a copy of itself doesn't mark nodes as needing layout. * media/modern-media-controls/layout-node/children-expected.txt: * media/modern-media-controls/layout-node/children.html: git-svn-id: https://svn.webkit.org/repository/webkit/trunk@212869 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- LayoutTests/ChangeLog | 14 +++++++++++++ .../layout-node/children-expected.txt | 5 +++++ .../layout-node/children.html | 6 ++++++ Source/WebCore/ChangeLog | 23 ++++++++++++++++++++++ .../modern-media-controls/controls/layout-node.js | 12 +++++++++++ 5 files changed, 60 insertions(+) diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 2e3669d..4c17813 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,5 +1,19 @@ 2017-02-22 Antoine Quint + [Modern Media Controls] Scrubber stops moving while scrubbing on macOS + https://bugs.webkit.org/show_bug.cgi?id=168518 + + + Reviewed by Dean Jackson. + + Add assertions to check that setting children to a copy of itself doesn't + mark nodes as needing layout. + + * media/modern-media-controls/layout-node/children-expected.txt: + * media/modern-media-controls/layout-node/children.html: + +2017-02-22 Antoine Quint + [Modern Media Controls] Controls bar may disappear while captions menu is visible https://bugs.webkit.org/show_bug.cgi?id=168751 diff --git a/LayoutTests/media/modern-media-controls/layout-node/children-expected.txt b/LayoutTests/media/modern-media-controls/layout-node/children-expected.txt index 91a7281..05765c4 100644 --- a/LayoutTests/media/modern-media-controls/layout-node/children-expected.txt +++ b/LayoutTests/media/modern-media-controls/layout-node/children-expected.txt @@ -20,6 +20,11 @@ PASS node.element.firstElementChild === a.element is true PASS node.element.firstElementChild.nextElementSibling === b.element is true PASS node.element.lastElementChild === c.element is true +Set children to be a copy of itself +PASS node.children[0].needsLayout is false +PASS node.children[1].needsLayout is false +PASS node.children[2].needsLayout is false + Set children to [b, a] PASS node.children.length === 2 is true PASS node.children[0] === b is true diff --git a/LayoutTests/media/modern-media-controls/layout-node/children.html b/LayoutTests/media/modern-media-controls/layout-node/children.html index 38b8db4..24ef088 100644 --- a/LayoutTests/media/modern-media-controls/layout-node/children.html +++ b/LayoutTests/media/modern-media-controls/layout-node/children.html @@ -42,6 +42,12 @@ scheduler.frameDidFire = function() shouldBeTrue("node.element.firstElementChild.nextElementSibling === b.element"); shouldBeTrue("node.element.lastElementChild === c.element"); debug(""); + debug("Set children to be a copy of itself"); + node.children = Array.from(node.children); + shouldBeFalse("node.children[0].needsLayout"); + shouldBeFalse("node.children[1].needsLayout"); + shouldBeFalse("node.children[2].needsLayout"); + debug(""); debug("Set children to [b, a]"); node.children = [b, a]; shouldBeTrue("node.children.length === 2"); diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index e65babe..d3bfa44 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,5 +1,28 @@ 2017-02-22 Antoine Quint + [Modern Media Controls] Scrubber stops moving while scrubbing on macOS + https://bugs.webkit.org/show_bug.cgi?id=168518 + + + Reviewed by Dean Jackson. + + As we start to scrub, controlValueWillStartChanging() is called on + ScrubberSupport and pauses the media if it's not already paused. This + causes the play/pause button to change icon and for layout() to be + called on MacOSInlineMediaControls. This in turns sets the .children + property on the .controlsBar and eventually yields a DOM manipulation + which re-inserts the scrubber's DOM hierarchy, causing stutters during + user interaction. + + Our solution is to make the .children setter smarter about identifying + that the children list hasn't changed and that no DOM invalidation is + necessary. + + * Modules/modern-media-controls/controls/layout-node.js: + (LayoutNode.prototype.set children): + +2017-02-22 Antoine Quint + [Modern Media Controls] Controls bar may disappear while captions menu is visible https://bugs.webkit.org/show_bug.cgi?id=168751 diff --git a/Source/WebCore/Modules/modern-media-controls/controls/layout-node.js b/Source/WebCore/Modules/modern-media-controls/controls/layout-node.js index 2a87f27..7db9219 100644 --- a/Source/WebCore/Modules/modern-media-controls/controls/layout-node.js +++ b/Source/WebCore/Modules/modern-media-controls/controls/layout-node.js @@ -126,6 +126,18 @@ class LayoutNode set children(children) { + if (children.length === this._children.length) { + let arraysDiffer = false; + for (let i = children.length - 1; i >= 0; --i) { + if (children[i] !== this._children[i]) { + arraysDiffer = true; + break; + } + } + if (!arraysDiffer) + return; + } + while (this._children.length) this.removeChild(this._children[0]); -- 1.8.3.1