Bug 190565 - [iOS] Draw caps lock indicator in password fields
Summary: [iOS] Draw caps lock indicator in password fields
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
: 191491 (view as bug list)
Depends on: 190056 191475
Blocks: 190571 191969 197724
  Show dependency treegraph
 
Reported: 2018-10-14 15:13 PDT by Daniel Bates
Modified: 2019-05-08 16:26 PDT (History)
5 users (show)

See Also:


Attachments
Work-in-progress patch (16.11 KB, patch)
2018-10-14 21:50 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (34.63 KB, patch)
2018-11-01 16:19 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (34.49 KB, patch)
2018-11-06 14:05 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (35.66 KB, patch)
2018-11-06 15:11 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (36.48 KB, patch)
2018-11-07 09:52 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (40.32 KB, patch)
2018-11-07 11:33 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (35.25 KB, patch)
2018-11-07 12:08 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (35.25 KB, patch)
2018-11-08 16:28 PST, Daniel Bates
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-10-14 15:13:37 PDT
We should show the caps lock indicator in a focused password field when caps lock is enabled so as to match iOS platform conventions.

We need to take care to track caps lock state changes when switching between apps and with respect to changes in keyboard availability.
Comment 1 Daniel Bates 2018-10-14 15:15:33 PDT
The patch for bug #190056 adds a new WebKit message that the UI process can send to the web process when modifier key state has changes. We should make use of this to update caps lock state in Modern WebKit.
Comment 2 Daniel Bates 2018-10-14 21:50:02 PDT
Created attachment 352298 [details]
Work-in-progress patch

Work-in-progress patch. This patch depends on the patch for bug #190056 and bug #190487 (in order) and some UIKit changes. Still need to listen for changes to hardware keyboard availability and update caps lock state. Need to verify correctness in legacy WebKit and Modern WebKit when caps lock state changes when switching between Safari and another app. Here are some app-switching test cases to consider assuming caps lock is initially disabled:

1. Press the caps lock, open Safari, focus on password field and ensure that the caps lock indicator is visible.
2. Open Safari, focus on password field, press caps lock, go to Home screen, press caps lock, open Safari and ensure that the caps lock indicator is not visible.
3. Open Safari, focus on password field, go to Home screen, press caps lock, open Safari and ensure that the caps lock indicator is visible.
4. Open Safari, focus on password field, press caps lock, go to app switcher, press caps lock, switch to Safari and ensure that the caps lock indicator is not visible.
5. Open Safari, focus on password field, go to app switcher, press caps lock, switch to Safari and ensure that the caps lock indicator is visible.
6. Open Safari, focus on password field, press caps lock, go to app switcher and switch to another app, press caps lock, go to app switcher and switch to Safari and ensure that the caps lock indicator is not visible.
7. Open Safari, focus on password field, go to app switcher and switch to another app, press caps lock, go to app switcher and switch to Safari and ensure that the caps lock indicator is visible.
Comment 3 Radar WebKit Bug Importer 2018-10-14 21:51:15 PDT
<rdar://problem/45262343>
Comment 4 Daniel Bates 2018-11-01 16:19:22 PDT
Created attachment 353659 [details]
Patch

This patch will fail to apply because it depends on the patch for bug #190056.
Comment 5 Daniel Bates 2018-11-06 14:05:27 PST
Created attachment 354002 [details]
Patch

Re-uploading patch now for EWS processing now that the patch for bug #190056 landed.
Comment 6 Daniel Bates 2018-11-06 15:11:28 PST
Created attachment 354015 [details]
Patch
Comment 7 Daniel Bates 2018-11-07 09:52:50 PST
Created attachment 354102 [details]
Patch
Comment 8 Daniel Bates 2018-11-07 11:33:45 PST
Created attachment 354114 [details]
Patch
Comment 9 Daniel Bates 2018-11-07 12:08:20 PST
Created attachment 354125 [details]
Patch
Comment 10 Daniel Bates 2018-11-08 16:28:35 PST
Created attachment 354290 [details]
Patch
Comment 11 Dean Jackson 2018-11-09 10:28:49 PST
Comment on attachment 354290 [details]
Patch

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

> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:33
> +#import <wtf/MainThread.h>

Why did you need this?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3761
> +    bool isHardwareKeyboardEvent = !!event._hidEvent;
> +    if (isHardwareKeyboardEvent && ((UIPhysicalKeyboardEvent *)event)._inputFlags & kUIKeyboardInputModifierFlagsChanged)
> +        _page->updateCurrentModifierState();

Why do we only want this with the hardware keyboard? I feel like we should also do it if you've activated capslock on the software keyboard.
Comment 12 Daniel Bates 2018-11-09 11:03:14 PST
Comment on attachment 354290 [details]
Patch

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

>> Source/WebCore/platform/cocoa/KeyEventCocoa.mm:33
>> +#import <wtf/MainThread.h>
> 
> Why did you need this?

Oops! Will remove before landing.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3761
>> +        _page->updateCurrentModifierState();
> 
> Why do we only want this with the hardware keyboard? I feel like we should also do it if you've activated capslock on the software keyboard.

We also want this for the software keyboard however we need to do more work to get shift state changes. Filed bug #191475 for this.
Comment 13 Daniel Bates 2018-11-09 11:10:37 PST
Committed r238047: <https://trac.webkit.org/changeset/238047>
Comment 15 Daniel Bates 2018-11-09 11:35:58 PST
(In reply to Ryan Haddad from comment #14)
> (In reply to Daniel Bates from comment #13)
> > Committed r238047: <https://trac.webkit.org/changeset/238047>
> 
> This change broke iOS builds:
> https://build.webkit.org/builders/Apple%20iOS%2012%20Release%20%28Build%29/
> builds/1177/steps/compile-webkit/logs/stdio

We do not seem to be regenerating derived sources following the change to WebPage.messages.in. Filed bug #191477.
Comment 16 Ryan Haddad 2018-11-09 17:30:32 PST
This change introduced layout test failures on iOS, as documented in https://bugs.webkit.org/show_bug.cgi?id=191491
Comment 17 Ryan Haddad 2018-11-09 17:40:28 PST
Reverted r238047 for reason:

Introduced layout test failures on iOS simulator.

Committed r238062: <https://trac.webkit.org/changeset/238062>
Comment 18 Ryan Haddad 2018-11-10 08:43:35 PST
(In reply to Ryan Haddad from comment #17)
> Reverted r238047 for reason:
> 
> Introduced layout test failures on iOS simulator.
> 
> Committed r238062: <https://trac.webkit.org/changeset/238062>
These are the tests that started failing with this change:

fast/css/text-overflow-input.html [ Failure ]
fast/forms/basic-inputs.html [ Failure ]
fast/forms/input-appearance-height.html [ Failure ]
fast/forms/input-value.html [ Failure ]
fast/forms/placeholder-pseudo-style.html [ Failure ]
tables/mozilla_expected_failures/bugs/bug92647-1.html [ Failure ]

Diffs can be seen here: https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Release%20WK2%20(Tests)/r238048%20(865)/results.html
Comment 19 Ryan Haddad 2018-11-10 08:43:45 PST
*** Bug 191491 has been marked as a duplicate of this bug. ***
Comment 20 Daniel Bates 2018-11-11 13:33:02 PST
Committed r238078: <https://trac.webkit.org/changeset/238078>