Bug 197452 – Add quirks to emulate undo and redo in hidden editable areas on some websites

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Emulate undo for docs

[Megan Gardner]

Created attachment 368878 [details]
Patch

[Megan Gardner]

Created attachment 368882 [details]
Patch

[Alex Christensen]

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.

[Megan Gardner]

Created attachment 368987 [details]
Patch

[Alex Christensen]

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

[Megan Gardner]

Created attachment 368989 [details]
Patch

[Megan Gardner]

Created attachment 369001 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

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

[Megan Gardner]

Created attachment 369172 [details]
Patch

[Megan Gardner]

Created attachment 369209 [details]
Patch

[Megan Gardner]

Created attachment 369214 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alex Christensen]

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

[Wenson Hsieh]

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

[Megan Gardner]

Created attachment 369322 [details]
Patch

[Megan Gardner]

Created attachment 369342 [details]
Patch

[Wenson Hsieh]

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.

[Megan Gardner]

Created attachment 369346 [details]
Patch

[Simon Fraser (smfr)]

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.

[Wenson Hsieh]

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?

[Wenson Hsieh]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Megan Gardner]

Created attachment 369396 [details]
Patch

[Megan Gardner]

Created attachment 369400 [details]
Patch

[Alex Christensen]

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.

[Wenson Hsieh]

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.

[Megan Gardner]

Created attachment 369413 [details]
Patch

[Megan Gardner]

Created attachment 369416 [details]
Patch

[Megan Gardner]

Created attachment 369440 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 369440 [details]
Patch for landing

Clearing flags on attachment: 369440

Committed r245079: <https://trac.webkit.org/changeset/245079>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50602775>