[webkit-reviews] review denied: [Bug 170048] AX: Media controls should be able to be re-activated after faded away : [Attachment 305309] Patch

[bugzilla-daemon at webkit.org]

Antoine Quint <graouts at apple.com> has denied Aaron Chu <aaron_chu at apple.com>'s
request for review:
Bug 170048: AX: Media controls should be able to be re-activated after faded
away
https://bugs.webkit.org/show_bug.cgi?id=170048

Attachment 305309: Patch

https://bugs.webkit.org/attachment.cgi?id=305309&action=review




--- Comment #5 from Antoine Quint <graouts at apple.com> ---
Comment on attachment 305309
  --> https://bugs.webkit.org/attachment.cgi?id=305309
Patch

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

Not a fan of this approach. The layout delegate is concerned with layout, and
this is setting a property on it that is focus-related, so I don't think that
it works. Also, I don't think it's a good approach to set a property on a
delegate, typically you'd have a delegation pattern where an IconButton informs
its delegate that it has received or lost focus and maintaining a focused state
is then the responsibility of the delegate.

But couldn't we take advantage of event bubbling here and register "focusin"
and "focusout" event listeners on the MediaControls instance? This would
require no change to IconButton and also would allow other types of focusable
items, such as the volume slider and scrubbers to be tracked.

> Source/WebCore/Modules/modern-media-controls/controls/icon-button.js:88
> +	   else if (event.type === "focus" && event.currentTarget ===
this.element)

WebKit style is closing curly bracket and else statement on the same line like
it was before.