158823 – Forward/Back keyboard shortcuts need to flip for RTL

RESOLVED FIXED 158823

Forward/Back keyboard shortcuts need to flip for RTL

https://bugs.webkit.org/show_bug.cgi?id=158823

Summary Forward/Back keyboard shortcuts need to flip for RTL

Sam Weinig

Reported 2016-06-15 17:28:34 PDT

Forward/Back keyboard shortcuts need to flip for RTL

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Sam Weinig

Comment 1 2016-06-15 17:32:21 PDT

Created attachment 281413 [details] Patch

Sam Weinig

Comment 2 2016-06-15 17:33:34 PDT

Created attachment 281414 [details] Patch

WebKit Commit Bot

Comment 3 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.

Tim Horton

Comment 4 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.

Conrad Shultz

Comment 5 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?

Darin Adler

Comment 6 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.

Sam Weinig

Comment 7 2016-06-15 17:47:04 PDT

Created attachment 281417 [details] Patch

WebKit Commit Bot

Comment 8 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.

Sam Weinig

Comment 9 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.

Conrad Shultz

Comment 10 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.

Sam Weinig

Comment 11 2016-06-16 09:36:23 PDT

Created attachment 281462 [details] Patch

Sam Weinig

Comment 12 2016-06-16 09:37:24 PDT

Created attachment 281463 [details] Patch

WebKit Commit Bot

Comment 13 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.

Darin Adler

Comment 14 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?

Sam Weinig

Comment 15 2016-06-16 10:54:34 PDT

Committed r202129: <http://trac.webkit.org/changeset/202129>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Sam Weinig

Reported

2016-06-15 17:28 PDT

Modified

2016-06-16 10:54 PDT History

CC List

2 users Show

URL

Keywords

Depends on

Blocks