Bug 212060 - Hide password echo when screen is being captured.
Summary: Hide password echo when screen is being captured.
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: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-18 20:45 PDT by Megan Gardner
Modified: 2020-05-20 15:34 PDT (History)
7 users (show)

See Also:


Attachments
Patch (12.74 KB, patch)
2020-05-18 20:58 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (18.33 KB, patch)
2020-05-19 18:20 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (18.89 KB, patch)
2020-05-19 20:04 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.28 KB, patch)
2020-05-20 11:56 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (19.28 KB, patch)
2020-05-20 13:25 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (19.30 KB, patch)
2020-05-20 14:57 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2020-05-18 20:45:25 PDT
Hide password echo when screen is being captured.
Comment 1 Megan Gardner 2020-05-18 20:58:24 PDT
Created attachment 399708 [details]
Patch
Comment 2 Megan Gardner 2020-05-18 20:58:42 PDT
<rdar://problem/47653578>
Comment 3 Tim Horton 2020-05-18 21:15:50 PDT
Comment on attachment 399708 [details]
Patch

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

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56
> +    bool screenIsCaptured = document().editor().client()->isScreenCaptured();

IMO the editor client method should be named similarly to the setting (shouldEchoPassword or something)

> Source/WebKit/UIProcess/ios/WKContentView.mm:172
> +    _page->setIsScreenCaptured([[UIScreen mainScreen] isCaptured]);

I’m not sure it makes sense to mirror the main screen’s state, we should send the message (with the right window’s state) when we’re parented instead.

> Source/WebKit/UIProcess/ios/WKContentView.mm:778
> +    _page->setIsScreenCaptured([[UIScreen mainScreen] isCaptured]);

Should always be using the WebView or content view screen
Comment 4 Tim Horton 2020-05-18 21:25:30 PDT
Also, nothing is pushing the right state in the process swap or crash case. You should add a bit to WebPageCreationParameters that, if parented, provides the current window’s state.
Comment 5 Megan Gardner 2020-05-19 18:20:40 PDT
Created attachment 399797 [details]
Patch
Comment 6 Megan Gardner 2020-05-19 20:04:54 PDT
Created attachment 399801 [details]
Patch
Comment 7 Wenson Hsieh 2020-05-19 21:26:44 PDT
Comment on attachment 399801 [details]
Patch

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

> Source/WebCore/page/EditorClient.h:74
> +    virtual bool surpressPasswordEcho() const { return false; };

Nit - `suppressPasswordEcho`.

> Source/WebKitLegacy/ios/DefaultDelegates/WebDefaultUIKitDelegate.m:271
> +- (BOOL)surpressPasswordEcho

Nit - `suppressPasswordEcho`.
Comment 8 Wenson Hsieh 2020-05-19 21:37:17 PDT
Comment on attachment 399801 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentView.mm:776
> +- (void)_screenCapturedDidChange:(NSNotification*)notification

Nit - space before the *.

> Source/WebKit/WebProcess/WebPage/WebPage.h:2002
> +    WebCore::FloatSize m_screenCaptured;

I think you meant to delete this.
Comment 9 Wenson Hsieh 2020-05-19 21:46:27 PDT
Comment on attachment 399801 [details]
Patch

Oh also — is UIScreenCapturedDidChangeNotification dispatched when we’re unparented or parented? (e.g. what happens if the screen is being captured in a WKWebView, but then the web view is removed from the view hierarchy and added later after screen capture has ended?)
Comment 10 Simon Fraser (smfr) 2020-05-19 22:07:23 PDT
Comment on attachment 399801 [details]
Patch

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

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56
> +    bool suppressPasswordEcho = document().editor().client()->surpressPasswordEcho();

shouldSuppressPasswordEcho

> Source/WebKit/UIProcess/WebPageProxy.h:722
> +    void setIsScreenCaptured(bool);

setScreenIsBeingCaptured.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1583
> +void WebPageProxy::setIsScreenCaptured(bool screenMirroring)

The dissonance between "captured" and "mirroring" is confusing.

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebEditorClientIOS.mm:122
> +bool WebEditorClient::surpressPasswordEcho() const

shouldSurpressPasswordEcho

> Source/WebKit/WebProcess/WebPage/WebPage.h:670
> +    bool isScreenCaptured() const { return m_isScreenCaptured; }
> +    void setIsScreenCaptured(bool);

isScreenBeingCaptured?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4281
> +void WebPage::setIsScreenCaptured(bool captured)

I would say "being captured" here. We're not capturing the screen, someone else is. "captured" is also vague here (easily confused with pointer capture). Can we say "being mirrored" instead?

> Source/WebKitLegacy/ios/WebView/WebUIKitDelegate.h:131
> +- (BOOL)surpressPasswordEcho;

Sounds like a verb. Should be isPasswordEchoSurpressed.

> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:833
> +bool WebEditorClient::surpressPasswordEcho() const

isPasswordEchoSurpressed

> Source/WebKitLegacy/mac/WebView/WebViewData.h:248
> +    BOOL surpressPasswordEcho;

isPasswordEchoSurpressed?
Comment 11 Megan Gardner 2020-05-20 11:56:04 PDT
Created attachment 399867 [details]
Patch
Comment 12 Megan Gardner 2020-05-20 13:25:45 PDT
Created attachment 399883 [details]
Patch for landing
Comment 13 Simon Fraser (smfr) 2020-05-20 13:54:48 PDT
Comment on attachment 399883 [details]
Patch for landing

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

> Source/WebKit/WebProcess/WebPage/WebPage.h:669
> +    bool screenIsCaptured() const { return m_screenIsCaptured; }

Missing a "being" here.
Comment 14 Megan Gardner 2020-05-20 14:57:41 PDT
Created attachment 399896 [details]
Patch for landing
Comment 15 EWS 2020-05-20 15:34:04 PDT
Committed r261966: <https://trac.webkit.org/changeset/261966>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399896 [details].