REGRESSION (r230523): Caps lock indicator not shown in password field

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

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.

[Radar WebKit Bug Importer]

<rdar://problem/44844121>

[Daniel Bates]

This regressed in <https://trac.webkit.org/changeset/230523/> when we enabled WindowServer blocking. We only enable WindowServer blocking in macOS Mojave.

[Per Arne Vollan]

Created attachment 351100 [details]
Patch

[Daniel Bates]

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.

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 351254 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 351275 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 351401 [details]
Patch

[Daniel Bates]

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.

[Per Arne Vollan]

(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.

[Daniel Bates]

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.

[Daniel Bates]

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>.

[Per Arne Vollan]

Created attachment 352055 [details]
Patch

[Daniel Bates]

You disagreed about adding more tests?

[Per Arne Vollan]

(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 :)

[Per Arne Vollan]

Created attachment 352709 [details]
Patch

[Daniel Bates]

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.

[Per Arne Vollan]

Created attachment 353612 [details]
Patch

[Per Arne Vollan]

(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.

[Tim Horton]

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?

[Per Arne Vollan]

(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 :)

[Per Arne Vollan]

Created attachment 353644 [details]
Patch

[Per Arne Vollan]

Created attachment 353647 [details]
Patch

[Per Arne Vollan]

Created attachment 353648 [details]
Patch

[Per Arne Vollan]

Created attachment 353650 [details]
Patch

[Tim Horton]

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?

[Per Arne Vollan]

Created attachment 353892 [details]
Patch

[Per Arne Vollan]

(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.

[Per Arne Vollan]

Created attachment 353901 [details]
Patch

[Per Arne Vollan]

Created attachment 353902 [details]
Patch

[Per Arne Vollan]

Created attachment 353907 [details]
Patch

[Per Arne Vollan]

Created attachment 353917 [details]
Patch

[Ryosuke Niwa]

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()?

[Per Arne Vollan]

Created attachment 353972 [details]
Patch

[Per Arne Vollan]

(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.

[WebKit Commit Bot]

Comment on attachment 353972 [details]
Patch

Clearing flags on attachment: 353972

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