Bug 162564 - [GTK] Mac defaults are used for key shortcuts on Linux
Summary: [GTK] Mac defaults are used for key shortcuts on Linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-26 09:21 PDT by Tomas Popela
Modified: 2016-09-27 02:29 PDT (History)
9 users (show)

See Also:


Attachments
Proposed patch (1.64 KB, patch)
2016-09-26 09:28 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff
Fix style issue (1.65 KB, patch)
2016-09-26 09:38 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff
Proposed patch (1.62 KB, patch)
2016-09-26 12:13 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1018.30 KB, application/zip)
2016-09-26 13:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.05 MB, application/zip)
2016-09-26 13:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.50 MB, application/zip)
2016-09-26 13:22 PDT, Build Bot
no flags Details
Fix Mac tests (2.34 KB, patch)
2016-09-27 00:14 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff
Fix the wrong 'reviewed by' phrase (2.35 KB, patch)
2016-09-27 01:54 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 2016-09-26 09:21:01 PDT
Trying to use Control + Left/Right doesn't move the caret in console, but Alt + Left/Right does (where there is actually no web view's history item, otherwise it will move to it). The cause is that we are using the macDefault keymap as fallthrough for CodeMirror.
Comment 1 Radar WebKit Bug Importer 2016-09-26 09:21:22 PDT
<rdar://problem/28475235>
Comment 2 Tomas Popela 2016-09-26 09:28:43 PDT
Created attachment 289832 [details]
Proposed patch
Comment 3 Carlos Garcia Campos 2016-09-26 09:33:51 PDT
Comment on attachment 289832 [details]
Proposed patch

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

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:591
> -        fallthrough: "macDefault"
> +        fallthrough: mac ? "macDefault":"pcDefault"

leave space between :
Comment 4 Tomas Popela 2016-09-26 09:38:28 PDT
Created attachment 289833 [details]
Fix style issue
Comment 5 Matt Baker 2016-09-26 11:29:01 PDT
Comment on attachment 289833 [details]
Fix style issue

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

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:575
> +    var mac = CodeMirror.keyMap["default"] == CodeMirror.keyMap.macDefault;

Style: strict equality (===) is used almost exclusively throughout the codebase. Also we've been preferring `let` over `var` for new code.
Comment 6 Tomas Popela 2016-09-26 11:42:53 PDT
(In reply to comment #5)
> Comment on attachment 289833 [details]
> Fix style issue
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289833&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:575
> > +    var mac = CodeMirror.keyMap["default"] == CodeMirror.keyMap.macDefault;
> 
> Style: strict equality (===) is used almost exclusively throughout the
> codebase. Also we've been preferring `let` over `var` for new code.

Thanks, I will update the patch. I copy the code from CodeMirror, but I will use the detection that is used across the Inspector code:

let mac = WebInspector.Platform.name === "mac";
Comment 7 Tomas Popela 2016-09-26 12:13:52 PDT
Created attachment 289841 [details]
Proposed patch
Comment 8 Build Bot 2016-09-26 13:08:35 PDT
Comment on attachment 289841 [details]
Proposed patch

Attachment 289841 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2149548

New failing tests:
inspector/codemirror/prettyprinting-javascript.html
inspector/codemirror/prettyprinting-css-rules.html
inspector/codemirror/prettyprinting-css.html
Comment 9 Build Bot 2016-09-26 13:08:38 PDT
Created attachment 289851 [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
Comment 10 Build Bot 2016-09-26 13:12:14 PDT
Comment on attachment 289841 [details]
Proposed patch

Attachment 289841 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2149551

New failing tests:
inspector/codemirror/prettyprinting-javascript.html
inspector/codemirror/prettyprinting-css-rules.html
inspector/codemirror/prettyprinting-css.html
Comment 11 Build Bot 2016-09-26 13:12:17 PDT
Created attachment 289852 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 12 Build Bot 2016-09-26 13:22:32 PDT
Comment on attachment 289841 [details]
Proposed patch

Attachment 289841 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2149549

New failing tests:
inspector/codemirror/prettyprinting-css.html
Comment 13 Build Bot 2016-09-26 13:22:35 PDT
Created attachment 289857 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 14 Brian Burg 2016-09-26 15:09:45 PDT
Comment on attachment 289841 [details]
Proposed patch

Tests are failing because WebInspector.Platform is not defined in the test resource.

To fix this, edit Source/WebInspectorUI/UserInterface/Test.html, and include Platform.js right after InspectorFrontendHostStub.js
Comment 15 Tomas Popela 2016-09-26 23:41:58 PDT
(In reply to comment #14)
> Comment on attachment 289841 [details]
> Proposed patch
> 
> Tests are failing because WebInspector.Platform is not defined in the test
> resource.
> 
> To fix this, edit Source/WebInspectorUI/UserInterface/Test.html, and include
> Platform.js right after InspectorFrontendHostStub.js

Thank you Brian, I will.
Comment 16 Tomas Popela 2016-09-27 00:14:03 PDT
Created attachment 289921 [details]
Fix Mac tests
Comment 17 WebKit Commit Bot 2016-09-27 01:40:44 PDT
Comment on attachment 289921 [details]
Fix Mac tests

Rejecting attachment 289921 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 289921, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebInspectorUI/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/2153344
Comment 18 Tomas Popela 2016-09-27 01:54:07 PDT
Created attachment 289925 [details]
Fix the wrong 'reviewed by' phrase
Comment 19 WebKit Commit Bot 2016-09-27 02:29:48 PDT
Comment on attachment 289925 [details]
Fix the wrong 'reviewed by' phrase

Clearing flags on attachment: 289925

Committed r206426: <http://trac.webkit.org/changeset/206426>
Comment 20 WebKit Commit Bot 2016-09-27 02:29:53 PDT
All reviewed patches have been landed.  Closing bug.