Bug 158856 – Web video playback controls should have RTL volume slider

• Component: Media
• Status: RESOLVED FIXED

[Antoine Quint]

The video controls should flip the volume control layout when the system is set to use a right-to-left language.

[Antoine Quint]

<rdar://problem/25971769>

[Antoine Quint]

Created attachment 281503 [details]
Patch

[Antoine Quint]

Created attachment 281507 [details]
Patch

Seems like this should respect the userInterfaceDirectionPolicy property of the configuration.

We should also use booleans with “LTR” rather than “RTL” in the name (and set their values appropriately).

[Antoine Quint]

(In reply to comment #4)
> Seems like this should respect the userInterfaceDirectionPolicy property of
> the configuration.

This change only applies to full-screen video controls and the directionality should be dictated by the WebView and not the content, so the policy should not apply.

> We should also use booleans with “LTR” rather than “RTL” in the name (and
> set their values appropriately).

thorton provided feedback on IRC that we should propagate the direction rather than an RTL flag, so I will make that change.

[Antoine Quint]

Created attachment 281532 [details]
Patch

[WebKit Commit Bot]

Attachment 281532 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.h:142:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:21:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

I'll have a test up in the morning.

[Antoine Quint]

Created attachment 281568 [details]
Patch

[WebKit Commit Bot]

Attachment 281568 [details] did not pass style-queue:


ERROR: Source/WebCore/page/Page.h:142:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/testing/Internals.h:489:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 281568 [details]
Patch

The WebCore and LayoutTest changes look good to me; this patch should get a WK2 Owner to sign off on the WK2 changes.

[Tim Horton]

Comment on attachment 281568 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:2242
> +        this.controls.volumeBox.classList.toggle(this.ClassNames.usesRTLUserInterfaceLayoutDirection, usesRTLUserInterfaceLayoutDirection);

You seem to have not followed mitz's review comment here. This should be about LTR, not RTL.

> Source/WebCore/page/Page.h:142
> +    enum class UserInterfaceLayoutDirection { LTR, RTL };

Really, right here in Page? You're not in good company, so it seems like a bad place :)

> Source/WebCore/testing/Internals.cpp:3352
> +    page->setUserInterfaceLayoutDirection(userInterfaceLayoutDirection == UserInterfaceLayoutDirection::LTR ? Page::UserInterfaceLayoutDirection::LTR : Page::UserInterfaceLayoutDirection::RTL);

:| (fix the below, and this will go away)

> Source/WebCore/testing/Internals.h:489
> +    enum class UserInterfaceLayoutDirection { LTR, RTL };

Why so many of these? Why not just one, in its own file.

> Source/WebKit2/UIProcess/UserInterfaceLayoutDirection.h:-31
> -enum class UserInterfaceLayoutDirection { LTR, RTL };

It was already in its own file!

[Antoine Quint]

Created attachment 281580 [details]
Patch

[WebKit Commit Bot]

Attachment 281580 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/UserInterfaceLayoutDirection.h:31:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/testing/Internals.h:489:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 281580 [details]
Patch

Attachment 281580 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1519761

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 281588 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281580 [details]
Patch

Attachment 281580 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1519764

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 281590 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 281580 [details]
Patch

Attachment 281580 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1519798

New failing tests:
http/tests/media/hls/video-controls-live-stream.html

[Build Bot]

Created attachment 281591 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Antoine Quint]

Created attachment 281592 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 281592 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/UserInterfaceLayoutDirection.h:31:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/testing/Internals.h:489:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 281592 [details]
Patch for landing

Clearing flags on attachment: 281592

Committed r202183: <http://trac.webkit.org/changeset/202183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.