Bug 197452 - Add quirks to emulate undo and redo in hidden editable areas on some websites
Summary: Add quirks to emulate undo and redo in hidden editable areas on some websites
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: 2019-04-30 18:52 PDT by Megan Gardner
Modified: 2019-05-08 16:47 PDT (History)
12 users (show)

See Also:


Attachments
Patch (30.24 KB, patch)
2019-05-02 20:13 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (30.30 KB, patch)
2019-05-02 20:39 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (30.40 KB, patch)
2019-05-03 14:18 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (30.44 KB, patch)
2019-05-03 14:29 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (30.21 KB, patch)
2019-05-03 15:16 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews210 for win-future (13.71 MB, application/zip)
2019-05-03 20:42 PDT, Build Bot
no flags Details
Patch (28.17 KB, patch)
2019-05-06 15:02 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (28.42 KB, patch)
2019-05-06 17:29 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (28.36 KB, patch)
2019-05-06 18:03 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (6.09 MB, application/zip)
2019-05-06 19:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews212 for win-future (13.54 MB, application/zip)
2019-05-07 01:02 PDT, Build Bot
no flags Details
Patch (28.33 KB, patch)
2019-05-07 13:26 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.40 KB, patch)
2019-05-07 17:45 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (19.31 KB, patch)
2019-05-07 18:02 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.60 MB, application/zip)
2019-05-07 21:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews214 for win-future (13.85 MB, application/zip)
2019-05-07 23:45 PDT, Build Bot
no flags Details
Patch (27.73 KB, patch)
2019-05-08 11:18 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (27.49 KB, patch)
2019-05-08 11:35 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (26.42 KB, patch)
2019-05-08 13:20 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (26.57 KB, patch)
2019-05-08 13:38 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (26.61 KB, patch)
2019-05-08 15:55 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 2019-04-30 18:52:40 PDT
Emulate undo for docs
Comment 1 Megan Gardner 2019-05-02 20:13:12 PDT
Created attachment 368878 [details]
Patch
Comment 2 Megan Gardner 2019-05-02 20:39:53 PDT
Created attachment 368882 [details]
Patch
Comment 3 Alex Christensen 2019-05-03 09:29:45 PDT
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.
Comment 4 Megan Gardner 2019-05-03 14:18:04 PDT
Created attachment 368987 [details]
Patch
Comment 5 Alex Christensen 2019-05-03 14:22:49 PDT
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...
Comment 6 Megan Gardner 2019-05-03 14:29:05 PDT
Created attachment 368989 [details]
Patch
Comment 7 Megan Gardner 2019-05-03 15:16:08 PDT
Created attachment 369001 [details]
Patch
Comment 8 Build Bot 2019-05-03 20:42:33 PDT
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
Comment 9 Build Bot 2019-05-03 20:42:37 PDT
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 10 Wenson Hsieh 2019-05-06 12:39:43 PDT
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).
Comment 11 Megan Gardner 2019-05-06 15:02:43 PDT
Created attachment 369172 [details]
Patch
Comment 12 Megan Gardner 2019-05-06 17:29:04 PDT
Created attachment 369209 [details]
Patch
Comment 13 Megan Gardner 2019-05-06 18:03:10 PDT
Created attachment 369214 [details]
Patch
Comment 14 Build Bot 2019-05-06 19:36:35 PDT
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.
Comment 15 Build Bot 2019-05-06 19:36:37 PDT
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 16 Build Bot 2019-05-07 01:02:13 PDT
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
Comment 17 Build Bot 2019-05-07 01:02:15 PDT
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
Comment 18 Alex Christensen 2019-05-07 11:59:01 PDT
(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.
Comment 19 Wenson Hsieh 2019-05-07 12:17:35 PDT
(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?
Comment 20 Megan Gardner 2019-05-07 13:26:34 PDT
Created attachment 369322 [details]
Patch
Comment 21 Megan Gardner 2019-05-07 17:45:09 PDT
Created attachment 369342 [details]
Patch
Comment 22 Wenson Hsieh 2019-05-07 17:51:42 PDT
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.
Comment 23 Megan Gardner 2019-05-07 18:02:40 PDT
Created attachment 369346 [details]
Patch
Comment 24 Simon Fraser (smfr) 2019-05-07 18:58:03 PDT
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 25 Wenson Hsieh 2019-05-07 19:09:23 PDT
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 26 Wenson Hsieh 2019-05-07 19:13:35 PDT
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 27 Build Bot 2019-05-07 21:00:31 PDT
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
Comment 28 Build Bot 2019-05-07 21:00:34 PDT
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 29 Build Bot 2019-05-07 23:45:39 PDT
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
Comment 30 Build Bot 2019-05-07 23:45:42 PDT
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
Comment 31 Megan Gardner 2019-05-08 11:18:03 PDT
Created attachment 369396 [details]
Patch
Comment 32 Megan Gardner 2019-05-08 11:35:48 PDT
Created attachment 369400 [details]
Patch
Comment 33 Alex Christensen 2019-05-08 11:59:26 PDT
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 34 Wenson Hsieh 2019-05-08 12:06:41 PDT
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.
Comment 35 Megan Gardner 2019-05-08 13:20:28 PDT
Created attachment 369413 [details]
Patch
Comment 36 Megan Gardner 2019-05-08 13:38:10 PDT
Created attachment 369416 [details]
Patch
Comment 37 Megan Gardner 2019-05-08 15:55:43 PDT
Created attachment 369440 [details]
Patch for landing
Comment 38 WebKit Commit Bot 2019-05-08 16:46:10 PDT
Comment on attachment 369440 [details]
Patch for landing

Clearing flags on attachment: 369440

Committed r245079: <https://trac.webkit.org/changeset/245079>
Comment 39 WebKit Commit Bot 2019-05-08 16:46:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Radar WebKit Bug Importer 2019-05-08 16:47:33 PDT
<rdar://problem/50602775>