Bug 190056 - REGRESSION (r230523): Caps lock indicator not shown in password field
Summary: REGRESSION (r230523): Caps lock indicator not shown in password field
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh macOS 10.14
: P1 Normal
Assignee: Per Arne Vollan
URL: data:text/html,<input%20type="password">
Keywords: InRadar, Regression
Depends on: 184451
Blocks: 190565
  Show dependency treegraph
 
Reported: 2018-09-27 14:34 PDT by Daniel Bates
Modified: 2018-11-09 11:42 PST (History)
14 users (show)

See Also:


Attachments
Patch (10.38 KB, patch)
2018-09-28 11:59 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.07 KB, patch)
2018-10-01 09:51 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.12 MB, application/zip)
2018-10-01 10:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.64 MB, application/zip)
2018-10-01 10:52 PDT, Build Bot
no flags Details
Patch (15.94 KB, patch)
2018-10-01 11:12 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.43 MB, application/zip)
2018-10-01 13:42 PDT, Build Bot
no flags Details
Patch (15.83 KB, patch)
2018-10-02 09:47 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.36 KB, patch)
2018-10-11 10:28 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.27 KB, patch)
2018-10-18 11:30 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.58 KB, patch)
2018-11-01 09:52 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.15 KB, patch)
2018-11-01 14:32 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.27 KB, patch)
2018-11-01 15:01 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.22 KB, patch)
2018-11-01 15:10 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.33 KB, patch)
2018-11-01 15:30 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (23.12 KB, patch)
2018-11-05 11:34 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.08 KB, patch)
2018-11-05 12:47 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.48 KB, patch)
2018-11-05 13:05 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (26.05 KB, patch)
2018-11-05 14:03 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (26.35 KB, patch)
2018-11-05 16:01 PST, Per Arne Vollan
rniwa: review+
Details | Formatted Diff | Diff
Patch (26.22 KB, patch)
2018-11-06 10:19 PST, Per Arne Vollan
no flags 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-09-27 14:34:14 PDT
Seen on macOS Mojave using system WebKit.

Steps to reproduce:

1. Visit <data:text/html,<input%20type="password">>.
2. Click in the password field to focus.
3. Press the Caps Lock key on the keyboard.

Then the Caps Lock indicator (a white up arrow on a gray background) is visible on the right-side of the password field. But is not visible.
Comment 1 Radar WebKit Bug Importer 2018-09-27 14:35:11 PDT
<rdar://problem/44844121>
Comment 2 Daniel Bates 2018-09-27 15:28:41 PDT
This regressed in <https://trac.webkit.org/changeset/230523/> when we enabled WindowServer blocking. We only enable WindowServer blocking in macOS Mojave.
Comment 3 Per Arne Vollan 2018-09-28 11:59:42 PDT
Created attachment 351100 [details]
Patch
Comment 4 Daniel Bates 2018-09-28 12:47:42 PDT
Comment on attachment 351100 [details]
Patch

This is not correct. Caps lock state could have changed when Safari is in the background and a person may just focus the password field when they make Safari the foreground app.
Comment 5 Per Arne Vollan 2018-09-28 12:50:53 PDT
(In reply to Daniel Bates from comment #4)
> Comment on attachment 351100 [details]
> Patch
> 
> This is not correct. Caps lock state could have changed when Safari is in
> the background and a person may just focus the password field when they make
> Safari the foreground app.

Ah, I see. Perhaps we can send the modifier state from the UI process when becoming the foreground app? Thanks for reviewing!
Comment 6 Per Arne Vollan 2018-10-01 09:51:30 PDT
Created attachment 351254 [details]
Patch
Comment 7 Build Bot 2018-10-01 10:50:28 PDT
Comment on attachment 351254 [details]
Patch

Attachment 351254 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9411881

New failing tests:
fast/events/detect-caps-lock.html
Comment 8 Build Bot 2018-10-01 10:50:30 PDT
Created attachment 351267 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 9 Build Bot 2018-10-01 10:52:01 PDT
Comment on attachment 351254 [details]
Patch

Attachment 351254 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9412006

New failing tests:
fast/events/detect-caps-lock.html
Comment 10 Build Bot 2018-10-01 10:52:03 PDT
Created attachment 351269 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 11 Per Arne Vollan 2018-10-01 11:12:25 PDT
Created attachment 351275 [details]
Patch
Comment 12 Build Bot 2018-10-01 13:42:06 PDT
Comment on attachment 351275 [details]
Patch

Attachment 351275 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9414386

New failing tests:
media/range-extract-contents-crash.html
Comment 13 Build Bot 2018-10-01 13:42:08 PDT
Created attachment 351293 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 14 Per Arne Vollan 2018-10-02 09:47:26 PDT
Created attachment 351401 [details]
Patch
Comment 15 Daniel Bates 2018-10-10 15:39:04 PDT
Comment on attachment 351401 [details]
Patch

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

This patch looks reasonable. A WebKit2 owner will need to review it. It would be good to add some more tests to cover the cases when the caps lock state changes when the view is not active (frontmost). Here are some more cases we should add tests for:

1. Press Caps Lock key when view is active, make view inactive, make view active again, press Caps Lock key.
2. Press Caps Lock key in inactive view, make view active, press Caps Lock key.
3. Press Caps Lock key when view is active, press Caps Lock key when view is active.
4. Press Caps Lock key when view is inactive, press Caps Lock key when view inactive.

> Source/WebCore/platform/PlatformKeyboardEvent.h:214
> +        static std::optional<OptionSet<Modifier>> m_currentModifiers;

Please add a comment to explain that we are using std::optional to support Legacy WebKit. Only in a Modern WebKit app will m_currentModifiers have a contained value.

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:719
> +void WebPageProxy::updateModifiers()

I do not see the need for this function as there is exactly one caller. Unless you envision future callers I suggest that we remove this function and inline its implementation into the call site. If you do envision future callers then I suggest we rename this function as its name is ambiguous as to whether it updates the modifier flags of a currently being processed event from the event stream or the state of the keyboard (which accounts for keys that have persistent state such as Caps Lock). This function does the latter. Throughout the existing code we seem to use the prefix "current" to describe such "global" state; => maybe call this function updateCurrentModifierState.

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:529
> +    UpdateModifiers(uint32_t modifiers)

For your consideration I suggest we name this SetCurrentModifierState() or UpdateCurrentModifierState() so as to be consistent with the naming of PlatformKeyboardEvent::{set, get}CurrentModifierState().

> LayoutTests/fast/events/detect-caps-lock.html:1
> +<html><head></head>

I do not see the need for this to use quirks mode. Please add <!DOCTYPE html> above this line.
Comment 16 Per Arne Vollan 2018-10-10 15:42:24 PDT
(In reply to Daniel Bates from comment #15)
> Comment on attachment 351401 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351401&action=review
> 
> This patch looks reasonable. A WebKit2 owner will need to review it. It
> would be good to add some more tests to cover the cases when the caps lock
> state changes when the view is not active (frontmost). Here are some more
> cases we should add tests for:
> 
> 1. Press Caps Lock key when view is active, make view inactive, make view
> active again, press Caps Lock key.
> 2. Press Caps Lock key in inactive view, make view active, press Caps Lock
> key.
> 3. Press Caps Lock key when view is active, press Caps Lock key when view is
> active.
> 4. Press Caps Lock key when view is inactive, press Caps Lock key when view
> inactive.
> 
> > Source/WebCore/platform/PlatformKeyboardEvent.h:214
> > +        static std::optional<OptionSet<Modifier>> m_currentModifiers;
> 
> Please add a comment to explain that we are using std::optional to support
> Legacy WebKit. Only in a Modern WebKit app will m_currentModifiers have a
> contained value.
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:719
> > +void WebPageProxy::updateModifiers()
> 
> I do not see the need for this function as there is exactly one caller.
> Unless you envision future callers I suggest that we remove this function
> and inline its implementation into the call site. If you do envision future
> callers then I suggest we rename this function as its name is ambiguous as
> to whether it updates the modifier flags of a currently being processed
> event from the event stream or the state of the keyboard (which accounts for
> keys that have persistent state such as Caps Lock). This function does the
> latter. Throughout the existing code we seem to use the prefix "current" to
> describe such "global" state; => maybe call this function
> updateCurrentModifierState.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:529
> > +    UpdateModifiers(uint32_t modifiers)
> 
> For your consideration I suggest we name this SetCurrentModifierState() or
> UpdateCurrentModifierState() so as to be consistent with the naming of
> PlatformKeyboardEvent::{set, get}CurrentModifierState().
> 
> > LayoutTests/fast/events/detect-caps-lock.html:1
> > +<html><head></head>
> 
> I do not see the need for this to use quirks mode. Please add <!DOCTYPE
> html> above this line.

Thanks for reviewing, Dan! I will update the patch.
Comment 17 Daniel Bates 2018-10-10 20:32:21 PDT
Comment on attachment 351401 [details]
Patch

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

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:721
> +    m_process->send(Messages::WebPage::UpdateModifiers([NSEvent modifierFlags]), m_pageID);    

Nit: There is some trailing whitespace at the end of this line. Please remove.
Comment 18 Daniel Bates 2018-10-10 20:41:52 PDT
Comment on attachment 351401 [details]
Patch

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

>>> Source/WebCore/platform/PlatformKeyboardEvent.h:214
>>> +        static std::optional<OptionSet<Modifier>> m_currentModifiers;
>> 
>> Please add a comment to explain that we are using std::optional to support Legacy WebKit. Only in a Modern WebKit app will m_currentModifiers have a contained value.
> 
> Thanks for reviewing, Dan! I will update the patch.

Nit: We prefix static member variables with "s_" instead of "m_" by <https://webkit.org/code-style-guidelines/#names-data-members>.
Comment 19 Per Arne Vollan 2018-10-11 10:28:15 PDT
Created attachment 352055 [details]
Patch
Comment 20 Daniel Bates 2018-10-11 20:06:08 PDT
You disagreed about adding more tests?
Comment 21 Per Arne Vollan 2018-10-12 08:25:19 PDT
(In reply to Daniel Bates from comment #20)
> You disagreed about adding more tests?

Not at all, I was looking for a way to deactivate/activate the view in WebKitTestRunner. Perhaps the best way is to use the event sender to generate a click inside/outside the view? I will look into this :)
Comment 22 Per Arne Vollan 2018-10-18 11:30:14 PDT
Created attachment 352709 [details]
Patch
Comment 23 Daniel Bates 2018-10-31 10:58:42 PDT
Comment on attachment 352709 [details]
Patch

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

The GTK, WinCairo, and WPE bots are not happy with this change.

> LayoutTests/fast/events/detect-caps-lock-expected.txt:11
> +This test verifies that the function WebCore::currentCapsLockState() returns true when Caps Lock is on.
> +
> +
> +CapsLock is on.
> +CapsLock is not on.
> +CapsLock is on.
> +CapsLock is not on.
> +CapsLock is on.
> +CapsLock is not on.
> +CapsLock is on.
> +CapsLock is not on.

Thank you for adding more sub tests. Can we make this output more meaningful? I cannot makes heads or tails as to what this output means without reading the actual test code. We should make it the output easy to understand to expedite triaging in case of failure.
Comment 24 Per Arne Vollan 2018-11-01 09:52:17 PDT
Created attachment 353612 [details]
Patch
Comment 25 Per Arne Vollan 2018-11-01 09:53:43 PDT
(In reply to Daniel Bates from comment #23)
> Comment on attachment 352709 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352709&action=review
> 
> The GTK, WinCairo, and WPE bots are not happy with this change.
> 
> > LayoutTests/fast/events/detect-caps-lock-expected.txt:11
> > +This test verifies that the function WebCore::currentCapsLockState() returns true when Caps Lock is on.
> > +
> > +
> > +CapsLock is on.
> > +CapsLock is not on.
> > +CapsLock is on.
> > +CapsLock is not on.
> > +CapsLock is on.
> > +CapsLock is not on.
> > +CapsLock is on.
> > +CapsLock is not on.
> 
> Thank you for adding more sub tests. Can we make this output more
> meaningful? I cannot makes heads or tails as to what this output means
> without reading the actual test code. We should make it the output easy to
> understand to expedite triaging in case of failure.

Thanks for reviewing, Dan! I have updated the patch according to your comments.
Comment 26 Tim Horton 2018-11-01 13:29:46 PDT
Comment on attachment 353612 [details]
Patch

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

> Source/WebCore/platform/mac/KeyEventMac.mm:236
> +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)

It feels pretty weird that this is all ifdef'd. Can we not just keep track of the state everywhere and only read it on platforms where we need it?
Comment 27 Per Arne Vollan 2018-11-01 13:32:58 PDT
(In reply to Tim Horton from comment #26)
> Comment on attachment 353612 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353612&action=review
> 
> > Source/WebCore/platform/mac/KeyEventMac.mm:236
> > +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
> It feels pretty weird that this is all ifdef'd. Can we not just keep track
> of the state everywhere and only read it on platforms where we need it?

Definitely, I'll update the patch. Thanks for reviewing :)
Comment 28 Per Arne Vollan 2018-11-01 14:32:59 PDT
Created attachment 353644 [details]
Patch
Comment 29 Per Arne Vollan 2018-11-01 15:01:08 PDT
Created attachment 353647 [details]
Patch
Comment 30 Per Arne Vollan 2018-11-01 15:10:35 PDT
Created attachment 353648 [details]
Patch
Comment 31 Per Arne Vollan 2018-11-01 15:30:13 PDT
Created attachment 353650 [details]
Patch
Comment 32 Tim Horton 2018-11-02 16:11:41 PDT
Comment on attachment 353650 [details]
Patch

Hmm, there's still a lot of #ifdefs here. Is there a reason that this isn't just cross platform code?
Comment 33 Per Arne Vollan 2018-11-05 11:34:14 PST
Created attachment 353892 [details]
Patch
Comment 34 Per Arne Vollan 2018-11-05 11:35:23 PST
(In reply to Tim Horton from comment #32)
> Comment on attachment 353650 [details]
> Patch
> 
> Hmm, there's still a lot of #ifdefs here. Is there a reason that this isn't
> just cross platform code?

Thanks for reviewing, Tim! I am attempting to move more to cross-platform in latest patch.
Comment 35 Per Arne Vollan 2018-11-05 12:47:29 PST
Created attachment 353901 [details]
Patch
Comment 36 Per Arne Vollan 2018-11-05 13:05:40 PST
Created attachment 353902 [details]
Patch
Comment 37 Per Arne Vollan 2018-11-05 14:03:41 PST
Created attachment 353907 [details]
Patch
Comment 38 Per Arne Vollan 2018-11-05 16:01:33 PST
Created attachment 353917 [details]
Patch
Comment 39 Ryosuke Niwa 2018-11-05 16:23:46 PST
Comment on attachment 353917 [details]
Patch

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

r=me with the following comments addressed

> Source/WebCore/platform/mac/KeyEventMac.mm:264
> +    if (s_currentModifiers) {

It's a bit subtle that this variable is set in WebContent process and it's not in UI process.
I would have preferred to have two set of functions but I think that would have too many dependencies to update.
Why don't we add a helper function like PlatformKeyboardEvent::currentStateOfModifierKeys() which has this logic
as I'm suggesting below with a comment saying that this is set only in WebContent process.

> Source/WebKit/UIProcess/WebPageProxy.cpp:8113
> +    PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);
> +    capsLockKey = PlatformKeyboardEvent::currentCapsLockState();

Can we add a new helper function to PlatformKeyboardEvent which returns OptionSet<PlatformEvent::Modifier>?
Specifically, we'd call GetCurrentKeyModifiers() twice here so so it would be nice to avoid that.
e.g. PlatformKeyboardEvent::currentStateOfModifierKeys()?
Comment 40 Per Arne Vollan 2018-11-06 10:19:32 PST
Created attachment 353972 [details]
Patch
Comment 41 Per Arne Vollan 2018-11-06 13:34:44 PST
(In reply to Ryosuke Niwa from comment #39)
> Comment on attachment 353917 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353917&action=review
> 
> r=me with the following comments addressed
> 
> > Source/WebCore/platform/mac/KeyEventMac.mm:264
> > +    if (s_currentModifiers) {
> 
> It's a bit subtle that this variable is set in WebContent process and it's
> not in UI process.
> I would have preferred to have two set of functions but I think that would
> have too many dependencies to update.
> Why don't we add a helper function like
> PlatformKeyboardEvent::currentStateOfModifierKeys() which has this logic
> as I'm suggesting below with a comment saying that this is set only in
> WebContent process.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:8113
> > +    PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey);
> > +    capsLockKey = PlatformKeyboardEvent::currentCapsLockState();
> 
> Can we add a new helper function to PlatformKeyboardEvent which returns
> OptionSet<PlatformEvent::Modifier>?
> Specifically, we'd call GetCurrentKeyModifiers() twice here so so it would
> be nice to avoid that.
> e.g. PlatformKeyboardEvent::currentStateOfModifierKeys()?

Thanks for reviewing! I have updated the patch accordingly.
Comment 42 WebKit Commit Bot 2018-11-06 13:58:37 PST
Comment on attachment 353972 [details]
Patch

Clearing flags on attachment: 353972

Committed r237886: <https://trac.webkit.org/changeset/237886>