Bug 158823 - Forward/Back keyboard shortcuts need to flip for RTL
Summary: Forward/Back keyboard shortcuts need to flip for RTL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-15 17:28 PDT by Sam Weinig
Modified: 2016-06-16 10:54 PDT (History)
2 users (show)

See Also:


Attachments
Patch (30.39 KB, patch)
2016-06-15 17:32 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (30.46 KB, patch)
2016-06-15 17:33 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (30.49 KB, patch)
2016-06-15 17:47 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (30.09 KB, patch)
2016-06-16 09:36 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (30.11 KB, patch)
2016-06-16 09:37 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2016-06-15 17:28:34 PDT
Forward/Back keyboard shortcuts need to flip for RTL
Comment 1 Sam Weinig 2016-06-15 17:32:21 PDT
Created attachment 281413 [details]
Patch
Comment 2 Sam Weinig 2016-06-15 17:33:34 PDT
Created attachment 281414 [details]
Patch
Comment 3 WebKit Commit Bot 2016-06-15 17:36:22 PDT
Attachment 281414 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:78:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:81:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:83:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:85:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 15 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Tim Horton 2016-06-15 17:42:47 PDT
Comment on attachment 281414 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4309
> +@property (nonatomic) UISemanticContentAttribute semanticContentAttribute NS_AVAILABLE_IOS(9_0);

???

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4429
> +    _impl->setUserInterfaceLayoutDirection(userInterfaceLayoutDirection);

Super!

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1440
> +    _data->_impl->setUserInterfaceLayoutDirection(userInterfaceLayoutDirection);

Super!

> Source/WebKit2/UIProcess/ios/WKContentView.mm:198
> +    auto direction = [UIView userInterfaceLayoutDirectionForSemanticContentAttribute:contentAttribute] == UIUserInterfaceLayoutDirectionLeftToRight) ? UserInterfaceLayoutDirection::LTR : UserInterfaceLayoutDirection::RTL

Go bye bye.
Comment 5 Conrad Shultz 2016-06-15 17:45:49 PDT
Comment on attachment 281414 [details]
Patch

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

Does any of this need version checks?

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4040
> +    return UserInterfaceLayoutDirection::LTR;

Is this needed?
Comment 6 Darin Adler 2016-06-15 17:46:55 PDT
Comment on attachment 281414 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4305
> +    auto direction = [UIView userInterfaceLayoutDirectionForSemanticContentAttribute:contentAttribute] == UIUserInterfaceLayoutDirectionLeftToRight) ? UserInterfaceLayoutDirection::LTR : UserInterfaceLayoutDirection::RTL

In the Mac file you chose to use a function to convert from one enum to the other. But not here. I like the approach where you use the function.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4309
>> +@property (nonatomic) UISemanticContentAttribute semanticContentAttribute NS_AVAILABLE_IOS(9_0);
> 
> ???

I’m a little confused. Did you paste this here for reference and forget to remove it?

>> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4040
>> +    return UserInterfaceLayoutDirection::LTR;
> 
> Is this needed?

ASSERT_NOT_REACHED?

>> Source/WebKit2/UIProcess/ios/WKContentView.mm:198
>> +    auto direction = [UIView userInterfaceLayoutDirectionForSemanticContentAttribute:contentAttribute] == UIUserInterfaceLayoutDirectionLeftToRight) ? UserInterfaceLayoutDirection::LTR : UserInterfaceLayoutDirection::RTL
> 
> Go bye bye.

Would be nice to share the conversion function with WKWebView. In fact, would be nice to find a way to not have this spread across both files ideally.

Looks like you forgot to use direction in this function, by the way.
Comment 7 Sam Weinig 2016-06-15 17:47:04 PDT
Created attachment 281417 [details]
Patch
Comment 8 WebKit Commit Bot 2016-06-15 17:49:54 PDT
Attachment 281417 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:78:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:81:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:83:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:85:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 15 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Sam Weinig 2016-06-15 17:53:48 PDT
(In reply to comment #5)
> Comment on attachment 281414 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281414&action=review
> 
> Does any of this need version checks?

Don't think so. Mac stuff has been around since 10.8. iOS stuff since iOS 9.

> 
> > Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:4040
> > +    return UserInterfaceLayoutDirection::LTR;
> 
> Is this needed?

Sadly, yes.
Comment 10 Conrad Shultz 2016-06-15 18:20:48 PDT
(In reply to comment #9)
> (In reply to comment #5)
> > Comment on attachment 281414 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=281414&action=review
> > 
> > Does any of this need version checks?
> 
> Don't think so. Mac stuff has been around since 10.8. iOS stuff since iOS 9.

Sorry, I was wondering whether the behavior change itself is intended to apply to all versions.
Comment 11 Sam Weinig 2016-06-16 09:36:23 PDT
Created attachment 281462 [details]
Patch
Comment 12 Sam Weinig 2016-06-16 09:37:24 PDT
Created attachment 281463 [details]
Patch
Comment 13 WebKit Commit Bot 2016-06-16 09:39:15 PDT
Attachment 281463 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:78:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:81:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:83:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:85:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/CommandBackForward.mm:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 15 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Darin Adler 2016-06-16 09:43:43 PDT
Comment on attachment 281463 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4304
> +    auto direction = [UIView userInterfaceLayoutDirectionForSemanticContentAttribute:contentAttribute];
> +    switch (direction) {

Maybe better without the local variable?
Comment 15 Sam Weinig 2016-06-16 10:54:34 PDT
Committed r202129: <http://trac.webkit.org/changeset/202129>