Emulate undo for docs
Created attachment 368878 [details] Patch
Created attachment 368882 [details] Patch
Comment on attachment 368882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368882&action=review > Source/WebCore/platform/PlatformKeyboardEvent.h:62 > + , m_isSyntheticEvent(false) This should be replaced by an initializer list in the header. bool m_isSyntheticEvent { false }; > Source/WebKit/UIProcess/ios/WKContentView.mm:544 > + if (_isUsingQuirkyNSUndoManager && WebCore::Quirks::shouldEmulateUndoRedoInHiddenEditableAreasForHost(URL({ }, _page->currentURL()).host())) { If the WebCore::Quirks function returns false, we should probably set _isUsingQuirkyNSUndoManager to false until next time we receive a message from a quirky website so we don't do extra work every time we request the undoManager.
Created attachment 368987 [details] Patch
Comment on attachment 368987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368987&action=review Does this need to include WebKitAdditions somewhere? > Source/WebCore/page/EventHandler.cpp:3159 > + bool shiftKey, ctrlKey, altKey, metaKey; > + keyEvent.getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey); I don't understand the effect of this change. > Source/WebCore/platform/PlatformKeyboardEvent.h:62 > + , m_isSyntheticEvent(false) initializer list...
Created attachment 368989 [details] Patch
Created attachment 369001 [details] Patch
Comment on attachment 369001 [details] Patch Attachment 369001 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12095442 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
Created attachment 369040 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369001&action=review > Source/WebCore/dom/Document.cpp:3166 > + if (quirks().shouldEmulateUndoRedoInHiddenEditableAreas()) > + page()->chrome().client().setNeedsQuirkyNSUndoManager(settings().needsSiteSpecificQuirks()); I'm a bit confused by this. Wouldn't settings().needsSiteSpecificQuirks() always be true here if quirks().shouldEmulateUndoRedoInHiddenEditableAreas() is true? So this seems equivalent to: if (quirks().shouldEmulateUndoRedoInHiddenEditableAreas()) page()->chrome().client().setNeedsQuirkyNSUndoManager(true); I suppose I don't quite understand why this isn't just something like... page()->chrome().client().setNeedsQuirkyNSUndoManager(quirks().shouldEmulateUndoRedoInHiddenEditableAreas()); > Source/WebCore/page/Page.cpp:1397 > + chrome().client().setNeedsQuirkyNSUndoManager(m_settings->needsSiteSpecificQuirks()); I think this is also a bit confusing, due to the names. Was this intended to be either of these? chrome().client().setNeedsQuirkyNSUndoManager(quirks().shouldEmulateUndoRedoInHiddenEditableAreas()); ...or: chrome().client().setNeedsSiteSpecificQuirks(quirks().needsSiteSpecificQuirks()); > Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:496 > + m_isSyntheticEvent = false; Nit - I don't think this is necessary, since you already initialize m_isSyntheticEvent to false. > Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:809 > + m_isSyntheticEvent = false; (This too) > Source/WebKit/UIProcess/WebPageProxy.cpp:6736 > +bool WebPageProxy::isCurrentURLHost(const String& base) const > +{ > + if (URL({ }, currentURL()).host() == base) > + return true; > + return false; > +} I might be missing something, but I don't see where this is used? > Source/WebKit/UIProcess/ios/WKContentView.mm:177 > +@property (readonly, weak) WKContentView* contentView; Nit - * on the other side. > Source/WebKit/UIProcess/ios/WKContentView.mm:181 > +- (instancetype)initWithContentView:(WKContentView*)contentView (Here too) > Source/WebKit/UIProcess/ios/WKContentView.mm:544 > + if (_isUsingQuirkyNSUndoManager && WebCore::Quirks::shouldEmulateUndoRedoInHiddenEditableAreasForHost(URL({ }, _page->currentURL()).host())) { I think you can remove this UI-process-side URL host name check altogether if you plumbed the value of quirks().shouldEmulateUndoRedoInHiddenEditableAreas() through to _isUsingQuirkyNSUndoManager (and then maybe rename _isUsingQuirkyNSUndoManager to _shouldEmulateUndoRedoInHiddenEditableAreas).
Created attachment 369172 [details] Patch
Created attachment 369209 [details] Patch
Created attachment 369214 [details] Patch
Comment on attachment 369214 [details] Patch Attachment 369214 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12118470 Number of test failures exceeded the failure limit.
Created attachment 369221 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369214 [details] Patch Attachment 369214 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12120840 New failing tests: http/tests/css/filters-on-iframes.html
Created attachment 369254 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
(In reply to Wenson Hsieh from comment #10) > I think you can remove this UI-process-side URL host name check altogether > if you plumbed the value of > quirks().shouldEmulateUndoRedoInHiddenEditableAreas() through to > _isUsingQuirkyNSUndoManager (and then maybe rename > _isUsingQuirkyNSUndoManager to _shouldEmulateUndoRedoInHiddenEditableAreas). I disagree. I think we should not rely on the WebProcess to turn off this UIProcess-side quirk, especially with process swapping. We don't want to get stuck in a state with this on for a non-quirks-needing website, and we don't want to require every load to send a message to update this state. I would believe it if the PasteRTFD.ImageElementUsesBlobURLInHTML API test failure were unrelated.
(In reply to Alex Christensen from comment #18) > (In reply to Wenson Hsieh from comment #10) > > I think you can remove this UI-process-side URL host name check altogether > > if you plumbed the value of > > quirks().shouldEmulateUndoRedoInHiddenEditableAreas() through to > > _isUsingQuirkyNSUndoManager (and then maybe rename > > _isUsingQuirkyNSUndoManager to _shouldEmulateUndoRedoInHiddenEditableAreas). > > I disagree. I think we should not rely on the WebProcess to turn off this > UIProcess-side quirk, especially with process swapping. We don't want to > get stuck in a state with this on for a non-quirks-needing website, and we > don't want to require every load to send a message to update this state. Sure, I could see the check being solely in the UI process, and that would be okay. The thing that concerns me is that there are checks in both the web process and the UI process currently, and it's not clear to me what the lifecycle of the quirk in the UI process is. I think it would be clearer if it was either the web process or the UI process driving this, not both. Ultimately, I think it would be easiest just to piggy-back off of FocusedElementInformation and send a flag over to the UI process indicating that we want a "quirky undo manager". That way, the lifecycle of the quirk is strictly confined to the input session, and ends when focus is lost. This means we wouldn't ever need to worry about this quirk bleeding across navigations. > > I would believe it if the PasteRTFD.ImageElementUsesBlobURLInHTML API test > failure were unrelated. Isn't this just because of the missing null check for Page?
Created attachment 369322 [details] Patch
Created attachment 369342 [details] Patch
Comment on attachment 369342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369342&action=review r=mews. You'll also want to do this: _focusedElementInformation.needsQuirkyUndoManager = false; ...in -[WKContentView _elementDidBlur]. > Source/WebKit/UIProcess/PageClient.h:508 > + Nit - extra newline.
Created attachment 369346 [details] Patch
Comment on attachment 369346 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369346&action=review > Source/WebCore/platform/PlatformKeyboardEvent.h:132 > + bool isSyntheticEvent() { return m_isSyntheticEvent; } const > Source/WebCore/platform/PlatformKeyboardEvent.h:133 > + void setSyntheticEvent() { m_isSyntheticEvent = true; } setIsSyntheticEvent (you're not setting an event) > Source/WebKit/Shared/FocusedElementInformation.h:139 > + bool needsQuirkyUndoManager { false }; "Quirky" doesn't describe what it does at all (maybe we'll need a different kind of quirky one in future). Not sure of a better name tho. > Source/WebKit/UIProcess/ios/WKContentView.mm:207 > +- (void)undo > +{ > + [self.contentView generateSyntheticUndoRedo:YES]; > +} > + > +- (void)redo > +{ > + [self.contentView generateSyntheticUndoRedo:NO]; > +} The YES/NO are awkward. Maybe just have to methods. > Source/WebKit/WebProcess/WebPage/WebPage.h:1224 > + void generateSyntheticUndoRedo(bool); The bool here is not great either.
Comment on attachment 369346 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369346&action=review >> Source/WebKit/Shared/FocusedElementInformation.h:139 >> + bool needsQuirkyUndoManager { false }; > > "Quirky" doesn't describe what it does at all (maybe we'll need a different kind of quirky one in future). Not sure of a better name tho. Perhaps something along the lines of, shouldSynthesizeKeyEventsForUndoAndRedo?
Comment on attachment 369346 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369346&action=review >> Source/WebKit/UIProcess/ios/WKContentView.mm:207 >> +} > > The YES/NO are awkward. Maybe just have to methods. Agreed. An alternative is to use an enum class — then, when we tackle <rdar://problem/46430520>, we could simply add Bold/Italic/Underline to the enum class, alongside Undo and Redo.
Comment on attachment 369346 [details] Patch Attachment 369346 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12129743 New failing tests: svg/repaint/remove-border-property-on-root.html
Created attachment 369356 [details] Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369342 [details] Patch Attachment 369342 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12130618 New failing tests: svg/repaint/remove-border-property-on-root.html
Created attachment 369362 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 369396 [details] Patch
Created attachment 369400 [details] Patch
Comment on attachment 369400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369400&action=review > Source/WebKit/Shared/SyntheticEditingCommandType.h:30 > +enum class SyntheticEditingCommandType : uint64_t { Nit: This seems excessive. It could be bool now, or uint8_t to plan for future expansion.
Comment on attachment 369400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369400&action=review > Source/WebCore/page/EventHandler.cpp:3159 > + bool shiftKey, ctrlKey, altKey, metaKey; > + keyEvent.getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey); Are shiftKey/ctrlKey/altKey/metaKey used anywhere? > Source/WebCore/platform/PlatformKeyboardEvent.h:133 > + void setIsSyntheticEvent() { m_isSyntheticEvent = true; } Super minor nit — extra space after the () > Source/WebKit/UIProcess/WebPageProxy.h:554 > + bool isCurrentURLHost(const String& base) const; Nit - still an unused function declaration.
Created attachment 369413 [details] Patch
Created attachment 369416 [details] Patch
Created attachment 369440 [details] Patch for landing
Comment on attachment 369440 [details] Patch for landing Clearing flags on attachment: 369440 Committed r245079: <https://trac.webkit.org/changeset/245079>
All reviewed patches have been landed. Closing bug.
<rdar://problem/50602775>