Bug 168518 - [Modern Media Controls] Scrubber stops moving while scrubbing on macOS
Summary: [Modern Media Controls] Scrubber stops moving while scrubbing on macOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-17 08:13 PST by Antoine Quint
Modified: 2017-02-22 17:56 PST (History)
2 users (show)

See Also:


Attachments
Patch (4.94 KB, patch)
2017-02-22 14:38 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (5.12 KB, patch)
2017-02-22 15:52 PST, Antoine Quint
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-01 for mac-elcapitan (835.64 KB, application/zip)
2017-02-22 16:38 PST, WebKit Commit Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2017-02-17 08:13:27 PST
If you start scrubbing using the scrubber on macOS, the slider's thumb will eventually stop tracking you. This only happens when the controls are hooked into a shadow root, so maybe this is some ShadowRoot bug and not a bug in the media controls code itself.
Comment 1 Radar WebKit Bug Importer 2017-02-17 08:13:52 PST
<rdar://problem/30577637>
Comment 2 Antoine Quint 2017-02-22 14:38:59 PST
Created attachment 302444 [details]
Patch
Comment 3 Dean Jackson 2017-02-22 14:44:59 PST
Comment on attachment 302444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302444&action=review

> Source/WebCore/ChangeLog:18
> +        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.

You need smart diffing!

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:132
> +            for (let i = children.length - 1; i >= 0; --i) {
> +                if (children[i] !== this._children[i]) {

Shame you can't use forEach. No way to break.
Comment 4 Antoine Quint 2017-02-22 14:46:01 PST
(In reply to comment #3)
> Comment on attachment 302444 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302444&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        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.
> 
> You need smart diffing!

I considered it but figured it wouldn't be worth the trouble.

> > Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:132
> > +            for (let i = children.length - 1; i >= 0; --i) {
> > +                if (children[i] !== this._children[i]) {
> 
> Shame you can't use forEach. No way to break.

Yass.
Comment 5 Antoine Quint 2017-02-22 15:52:02 PST
Created attachment 302456 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2017-02-22 16:38:31 PST
Comment on attachment 302456 [details]
Patch for landing

Rejecting attachment 302456 [details] from commit-queue.

New failing tests:
editing/spelling/spellcheck-async-remove-frame.html
Full output: http://webkit-queues.webkit.org/results/3175408
Comment 7 WebKit Commit Bot 2017-02-22 16:38:34 PST
Created attachment 302462 [details]
Archive of layout-test-results from webkit-cq-01 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Antoine Quint 2017-02-22 17:56:53 PST
Committed r212869: <http://trac.webkit.org/changeset/212869>