Bug 183166 - [Win][DRT] Implement setSpatialNavigationEnabled.
Summary: [Win][DRT] Implement setSpatialNavigationEnabled.
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: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-27 11:42 PST by Ross Kirsling
Modified: 2018-03-01 13:52 PST (History)
7 users (show)

See Also:


Attachments
Patch (10.44 KB, patch)
2018-02-27 11:46 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2018-02-27 13:16 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (11.59 MB, application/zip)
2018-02-27 14:55 PST, Build Bot
no flags Details
Patch (12.50 KB, patch)
2018-02-27 15:06 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2018-02-28 11:33 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (12.27 KB, patch)
2018-02-28 13:53 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (12.26 KB, patch)
2018-03-01 11:22 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2018-02-27 11:42:20 PST
[WinCairo][DRT] Implement setSpatialNavigationEnabled.
Comment 1 Ross Kirsling 2018-02-27 11:46:36 PST
Created attachment 334697 [details]
Patch
Comment 2 Ross Kirsling 2018-02-27 11:54:53 PST
I'm just focusing on WinCairo here, but let me know if you'd like me to remove the Skip line from AppleWin TestExpectations as well:
https://github.com/WebKit/webkit/blob/master/LayoutTests/platform/win/TestExpectations#L228-L229
Comment 3 Ross Kirsling 2018-02-27 13:16:31 PST
Created attachment 334704 [details]
Patch
Comment 4 Ross Kirsling 2018-02-27 13:17:12 PST
Comment on attachment 334704 [details]
Patch

...figured if I was going to bother to mention it, I should just go ahead and do it.
Comment 5 Ross Kirsling 2018-02-27 14:08:43 PST
The WinCairo failure here is a CMake fluke (no code was changed from the previous patch); I've made a bug for it here:
https://bugs.webkit.org/show_bug.cgi?id=183177
Comment 6 Build Bot 2018-02-27 14:55:14 PST
Comment on attachment 334704 [details]
Patch

Attachment 334704 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6695928

New failing tests:
fast/spatial-navigation/snav-multiple-select-focusring.html
Comment 7 Build Bot 2018-02-27 14:55:24 PST
Created attachment 334707 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 8 Ross Kirsling 2018-02-27 15:06:02 PST
Created attachment 334708 [details]
Patch
Comment 9 Per Arne Vollan 2018-02-28 10:57:32 PST
Comment on attachment 334708 [details]
Patch

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

> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:123
> +    HRESULT isSpatialNavigationEnabled([out, retval] BOOL *enabled);
> +    HRESULT setSpatialNavigationEnabled([in] BOOL enabled);

I think you need to add this to the end of IWebPreferencesPrivate6, or create IWebPreferencesPrivate7, and add these methods there.
Comment 10 Ross Kirsling 2018-02-28 11:33:00 PST
Created attachment 334754 [details]
Patch
Comment 11 Ross Kirsling 2018-02-28 13:53:16 PST
Created attachment 334765 [details]
Patch
Comment 12 Ross Kirsling 2018-02-28 15:02:29 PST
Thanks, I think this should be correct now.
Comment 13 Per Arne Vollan 2018-03-01 10:24:29 PST
Comment on attachment 334765 [details]
Patch

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

> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:224
> +    HRESULT isSpatialNavigationEnabled([out, retval] BOOL *enabled);

I think we usually would use 'spatialNavigationEnabled()' here.
Comment 14 Ross Kirsling 2018-03-01 10:28:22 PST
(In reply to Per Arne Vollan from comment #13)
> Comment on attachment 334765 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334765&action=review
> 
> > Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:224
> > +    HRESULT isSpatialNavigationEnabled([out, retval] BOOL *enabled);
> 
> I think we usually would use 'spatialNavigationEnabled()' here.

I was just mimicking "isXSSAuditorEnabled", but I'm happy to do either.
Comment 15 Ross Kirsling 2018-03-01 11:22:17 PST
Created attachment 334828 [details]
Patch
Comment 16 Per Arne Vollan 2018-03-01 11:26:10 PST
Comment on attachment 334828 [details]
Patch

R=me as long as ews is green.
Comment 17 Don Olmstead 2018-03-01 12:43:28 PST
(In reply to Per Arne Vollan from comment #16)
> Comment on attachment 334828 [details]
> Patch
> 
> R=me as long as ews is green.

On WinCairo we seem to be hitting https://bugs.webkit.org/show_bug.cgi?id=183177 with this patch. We're fine with landing it as is.
Comment 18 WebKit Commit Bot 2018-03-01 13:50:46 PST
Comment on attachment 334828 [details]
Patch

Clearing flags on attachment: 334828

Committed r229143: <https://trac.webkit.org/changeset/229143>
Comment 19 WebKit Commit Bot 2018-03-01 13:50:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-03-01 13:52:09 PST
<rdar://problem/38039052>