Hide password echo when screen is being captured.
Created attachment 399708 [details] Patch
<rdar://problem/47653578>
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
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.
Created attachment 399797 [details] Patch
Created attachment 399801 [details] Patch
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 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 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 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?
Created attachment 399867 [details] Patch
Created attachment 399883 [details] Patch for landing
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.
Created attachment 399896 [details] Patch for landing
Committed r261966: <https://trac.webkit.org/changeset/261966> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399896 [details].
Comment on attachment 399867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399867&action=review > Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56 > + bool shouldSuppressPasswordEcho = document().editor().client()->shouldSuppressPasswordEcho(); I think this should be added to the passwordEchoEnabled expression, rather than put in a separate boolean. > Source/WebKitLegacy/mac/WebView/WebView.mm:1528 > + if ([[self _UIKitDelegateForwarder] respondsToSelector:@selector(shouldSuppressPasswordEcho)]) > + _private->shouldSuppressPasswordEcho = [[self _UIKitDelegateForwarder] shouldSuppressPasswordEcho]; I think this code should be removed. > Source/WebKitLegacy/mac/WebView/WebViewData.h:248 > + BOOL shouldSuppressPasswordEcho; I think this should be removed. It’s set but never read.
Post commit comments address in https://trac.webkit.org/changeset/262272/webkit