Bug 158823

Summary:

Forward/Back keyboard shortcuts need to flip for RTL

Product:

WebKit

Reporter:

Sam Weinig <sam>

Component:

New Bugs

Assignee:

Sam Weinig <sam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, conrad_shultz

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	darin: review+

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.