Bug 158856

Summary: Web video playback controls should have RTL volume slider
Product: WebKit Reporter: Antoine Quint <graouts>
Component: MediaAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, graouts, mitz, mmaxfield, rniwa, sam, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Patch for landing none

Antoine Quint
Reported 2016-06-16 16:24:02 PDT
The video controls should flip the volume control layout when the system is set to use a right-to-left language.
Attachments
Patch (14.44 KB, patch)
2016-06-16 16:37 PDT, Antoine Quint
no flags
Patch (14.40 KB, patch)
2016-06-16 16:43 PDT, Antoine Quint
no flags
Patch (37.99 KB, patch)
2016-06-16 22:11 PDT, Antoine Quint
no flags
Patch (44.53 KB, patch)
2016-06-17 11:14 PDT, Antoine Quint
no flags
Patch (51.44 KB, patch)
2016-06-17 13:25 PDT, Antoine Quint
no flags
Archive of layout-test-results from ews103 for mac-yosemite (987.42 KB, application/zip)
2016-06-17 14:20 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (849.11 KB, application/zip)
2016-06-17 14:23 PDT, Build Bot
no flags
Archive of layout-test-results from ews117 for mac-yosemite (1.48 MB, application/zip)
2016-06-17 14:43 PDT, Build Bot
no flags
Patch for landing (53.37 KB, patch)
2016-06-17 14:55 PDT, Antoine Quint
no flags
Antoine Quint
Comment 1 2016-06-16 16:24:35 PDT
Antoine Quint
Comment 2 2016-06-16 16:37:25 PDT
Antoine Quint
Comment 3 2016-06-16 16:43:43 PDT
mitz
Comment 4 2016-06-16 17:17:53 PDT
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
Comment 5 2016-06-16 17:33:40 PDT
(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
Comment 6 2016-06-16 22:11:06 PDT
WebKit Commit Bot
Comment 7 2016-06-16 22:12:50 PDT
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
Comment 8 2016-06-16 22:23:52 PDT
I'll have a test up in the morning.
Antoine Quint
Comment 9 2016-06-17 11:14:56 PDT
WebKit Commit Bot
Comment 10 2016-06-17 11:16:03 PDT
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 11 2016-06-17 11:24:10 PDT
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 12 2016-06-17 11:35:53 PDT
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
Comment 13 2016-06-17 13:25:46 PDT
WebKit Commit Bot
Comment 14 2016-06-17 13:28:33 PDT
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 15 2016-06-17 14:20:28 PDT
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
Comment 16 2016-06-17 14:20:31 PDT
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 17 2016-06-17 14:23:43 PDT
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
Comment 18 2016-06-17 14:23:46 PDT
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 19 2016-06-17 14:43:32 PDT
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
Comment 20 2016-06-17 14:43:35 PDT
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
Comment 21 2016-06-17 14:55:21 PDT
Created attachment 281592 [details] Patch for landing
WebKit Commit Bot
Comment 22 2016-06-17 14:56:39 PDT
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 23 2016-06-17 15:24:55 PDT
Comment on attachment 281592 [details] Patch for landing Clearing flags on attachment: 281592 Committed r202183: <http://trac.webkit.org/changeset/202183>
WebKit Commit Bot
Comment 24 2016-06-17 15:25:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.