Forward/Back keyboard shortcuts need to flip for RTL
Created attachment 281413 [details] Patch
Created attachment 281414 [details] Patch
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 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 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 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.
Created attachment 281417 [details] Patch
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.
(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.
(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.
Created attachment 281462 [details] Patch
Created attachment 281463 [details] Patch
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 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?
Committed r202129: <http://trac.webkit.org/changeset/202129>