Bug 199587 – Typing into a cell in a Google Sheet lags behind by one character

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Steps to reproduce:

(You may have to request the desktop site)

1. Visit <http://docs.google.com/sheets> and create a new sheet or open an existing one. (On iOS 12, you may have more success by visiting <https://drive.google.com> and opening an existing sheet from there).
2. Tap a cell and type 'a'.

Then the formula bar is empty. But it should show 'a'.

Note, if you type 'b' then the cell updates and show the value 'a'. But it should show 'ab'.

(Similarly experiment, run through the above steps but after opening a sheet, tap a cell, then tap the formula bar then type 'c'. Then the cell's value should show a 'c'. But it does not).

[Daniel Bates]

<rdar://problem/51616845>

[Daniel Bates]

So, the cause of this bug....Google Sheets is listening for the wrong event in my opinion. They are listening for keypress and scheduling a timer to fetch the value of the cell (or formula bar) and update the formula bar (or cell). They assume that the DOM will be updated (i.e. text will be inserted/deleted) in the same event loop iteration as the keypress. This is ***not*** guaranteed by any spec. as far as I can tell. There is an event Google could listen for that is dispatched exactly when a DOM update occurs as a result of a text insertion/delete: the input event.

[Daniel Bates]

<https://github.com/w3c/uievents/issues/238>

[Daniel Bates]

Created attachment 373662 [details]
Test

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #2)
> They assume that the DOM will be updated (i.e. text will be inserted/deleted) in the same event loop iteration as the keypress. This is ***not*** guaranteed by any spec. as far as I can tell.

https://w3c.github.io/uievents/#event-type-keydown
The default action of the keydown event depends upon the key:

"If the key is associated with a character, the default action MUST be to dispatch a beforeinput event followed by an input event. In the case where the key which is associated with multiple characters (such as with a macro or certain sequences of dead keys), the default action MUST be to dispatch one set of beforeinput / input events for each character"

The default action of an event happens synchronously unless otherwise stated: https://w3c.github.io/uievents/#event-flow-default-cancel

For example, the default action of a click event might be of the activation behavior:
https://w3c.github.io/uievents/#event-type-click 

Such an activation MUST happen synchronously, or else would cause a bunch of web compatibility issues.

If the UI events specification is ambiguous as to whether this should happen synchronously or not, then I'd argue that's a bug of the specification. As far as I know both key event's default actions and click event's default actions need to happen synchronously or a bunch of things would break.

[Daniel Bates]

@ryosuke: If you want to discuss this then talk to me in person and I can explain you what your missing.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #6)
> @ryosuke: If you want to discuss this then talk to me in person and I can
> explain you what your missing.

Please rely here in the Bugzilla so that others can follow.

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #7)
> (In reply to Daniel Bates from comment #6)
> > @ryosuke: If you want to discuss this then talk to me in person and I can
> > explain you what your missing.
> 
> Please rely here in the Bugzilla so that others can follow.

Also note that my statement that it's not web compatible to execute the default action of click & keydown events asynchronously is based on my own experience working in WebKit & Chromium. In fact, there was an attempt made to make key events sent asynchronously from Chromium's browser process to its renderer process about 8 years ago, and resulted in many compatibility issues so we had to revert such a code change. So if there is any ambiguity in the specification, that's a bug with the specification, not with Google Sheet.

[Geoffrey Garen]

I think the discussion about standards here is a distraction. When a standard omits a detail but website compatibility requires that detail (as in this case), we need to implement that detail.

WebKit's project goals document explains this need clearly <https://webkit.org/project/>:

Compatibility

For users browsing the web, compatibility with their existing sites is essential. We strive to maintain and improve compatibility with existing web content, sometimes even at the expense of standards. We use regression testing to maintain our compatibility gains.

[Geoffrey Garen]

Here are some options for a compatible implementation, not necessarily in any order:

1. Queue up the key event(s) in the UI Process or Web Process until we’re ready to dispatch them in the same runloop as the input event;

2. Do sync IPC from the Web Process to wait for the text change instead of using a queue (*);

3. Make any timer scheduled by keydown/keyup delay until after max(timeout, did the input event fire?).

(*) Wenson said:

On that note, there’s already a sync call to the UI process for -_interpretKeyEvent:. We currently invoke the delayed IPC completion handler immediately after calling interpretKeyEvents in the UI process, but one could imagine hanging onto the completion handler and invoking it when we get a call to -insertText: (if we added an input string), or immediately if we don’t expect a subsequent call to -insertText. We would then return the input string as an argument in the sync IPC completion handler, and we’d immediately trigger text insertion if needed, upon receiving this string in the web process.

And I say:

It looks like we get this behavior right for input events, so we might be able to piggyback on or copy the implementation or input events to get this behavior right for key events.

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #10)
> Here are some options for a compatible implementation, not necessarily in
> any order:
> 
> 1. Queue up the key event(s) in the UI Process or Web Process until we’re
> ready to dispatch them in the same runloop as the input event;

I don't think this approach would work. When the script calls preventDefault() on the keydown event, we're not supposed to be calling _interpretKeyEvent in the UI process unless we can undo all the side effects of _interpretKeyEvent after the fact (which I don't believe is possible). Yet we can't fire input event until we know what _interpretKeyEvent would do.

[Daniel Bates]

Created attachment 373954 [details]
For the bots

[Daniel Bates]

Created attachment 373960 [details]
For the bots (compile with the correct time limit for simulator and device)

[Daniel Bates]

Created attachment 373961 [details]
For the bots

[Daniel Bates]

Created attachment 373972 [details]
Patch

[Daniel Bates]

Created attachment 373974 [details]
Patch and layout test

Grammar

[EWS Watchlist]

Comment on attachment 373974 [details]
Patch and layout test

Attachment 373974 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12719929

New failing tests:
scrollingcoordinator/ios/fixed-in-overflow-scroll-scrolling-tree.html
fast/scrolling/ios/scrollbar-hiding.html

[EWS Watchlist]

Created attachment 373988 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[Daniel Bates]

(In reply to Build Bot from comment #17)
> Comment on attachment 373974 [details]
> Patch and layout test
> 
> Attachment 373974 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/12719929
> 
> New failing tests:
> scrollingcoordinator/ios/fixed-in-overflow-scroll-scrolling-tree.html
> fast/scrolling/ios/scrollbar-hiding.html

These tests are flaky (famous last words 😀). Tests have nothing to do with any of the code paths touched. Both these test pass on my machine.

[Brent Fulgham]

Comment on attachment 373974 [details]
Patch and layout test

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

This seems like a reasonable approach to this issue. I'm not sure if we should open this up for 'stash' as well (who suffers from the same issue). We may find that many sites need this quirk (or maybe this quirk needs to be standard behavior).

> Source/WebCore/ChangeLog:14
> +        of the document.

Was the timeout supposed to wait for the longest timeout registered on the page?

> Source/WebCore/page/Quirks.cpp:362
> +    return equalLettersIgnoringASCIICase(url.host(), "docs.google.com") && url.path().startsWithIgnoringASCIICase("/spreadsheets/");

Should we do this for stash, too?

[Daniel Bates]

(In reply to Brent Fulgham from comment #20)
> Comment on attachment 373974 [details]
> Patch and layout test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373974&action=review
> 
> This seems like a reasonable approach to this issue. I'm not sure if we
> should open this up for 'stash' as well (who suffers from the same issue).

There's no one stash (now called BitBucket Server) web site 🙃 It's a deployed service. Kinda hard (not impossible, but hard) to quirkily this.

> We may find that many sites need this quirk (or maybe this quirk needs to be
> standard behavior).
> 
> > Source/WebCore/ChangeLog:14
> > +        of the document.
> 
> Was the timeout supposed to wait for the longest timeout registered on the
> page?
> 

I don't get what you're asking. What timeout? Are you asking if the exceeded maximum holding tank timeout should be equal to the longest timeout the page scheduled on keydown and keypress? If so, the answer is no because:

It's excessive, Google schedules up to a 5 minute timer on keydown/keypress. That's a long time to prevent holding tank timers from executing their actions if for some reason (a 🐞) the keyboard did not call back to WebKit to tell it to insert or delete something.

> > Source/WebCore/page/Quirks.cpp:362
> > +    return equalLettersIgnoringASCIICase(url.host(), "docs.google.com") && url.path().startsWithIgnoringASCIICase("/spreadsheets/");
> 
> Should we do this for stash, too?

Not feasible. See first remark.

[Daniel Bates]

Comment on attachment 373974 [details]
Patch and layout test

Clearing flags on attachment: 373974

Committed r247444: <https://trac.webkit.org/changeset/247444>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to Daniel Bates from comment #19)
> (In reply to Build Bot from comment #17)
> > Comment on attachment 373974 [details]
> > Patch and layout test
> > 
> > Attachment 373974 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> > Output: https://webkit-queues.webkit.org/results/12719929
> > 
> > New failing tests:
> > scrollingcoordinator/ios/fixed-in-overflow-scroll-scrolling-tree.html
> > fast/scrolling/ios/scrollbar-hiding.html
> 
> These tests are flaky (famous last words 😀). Tests have nothing to do with
> any of the code paths touched. Both these test pass on my machine.
Both of these tests are consistently failing on the bots now that this change has landed:
https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/r247464%20(4634)/results.html

[Daniel Bates]

That sucks. Roll it out.

What did I miss?

[Ryan Haddad]

Reverted r247444 for reason:

Caused two scrolling tests to fail on iOS Simulator

Committed r247472: <https://trac.webkit.org/changeset/247472>

[Simon Fraser (smfr)]

For some reason, with this patch, when ScrollingTreeOverflowScrollingNode::dumpProperties() calls:
    ts << "overflow scrolling node";

the compiler generates code that outputs an identity TransformOperations:

WebCore`WebCore::ScrollingTreeOverflowScrollingNode::dumpProperties:
    0x1264ce4c0 <+0>:   pushq  %rbp
    0x1264ce4c1 <+1>:   movq   %rsp, %rbp
    0x1264ce4c4 <+4>:   subq   $0x40, %rsp
    0x1264ce4c8 <+8>:   movq   %rdi, -0x8(%rbp)
    0x1264ce4cc <+12>:  movq   %rsi, -0x10(%rbp)
    0x1264ce4d0 <+16>:  movl   %edx, -0x14(%rbp)
    0x1264ce4d3 <+19>:  movq   -0x8(%rbp), %rsi
    0x1264ce4d7 <+23>:  movq   -0x10(%rbp), %rdi
    0x1264ce4db <+27>:  leaq   -0x28(%rbp), %rax
    0x1264ce4df <+31>:  movq   %rdi, -0x30(%rbp)
    0x1264ce4e3 <+35>:  movq   %rax, %rdi
    0x1264ce4e6 <+38>:  movl   $0x1, %edx
    0x1264ce4eb <+43>:  movq   %rsi, -0x38(%rbp)
    0x1264ce4ef <+47>:  movl   %edx, %esi
    0x1264ce4f1 <+49>:  callq  0x1267b57d0               ; WebCore::TransformOperations::TransformOperations at TransformOperations.cpp:33
->  0x1264ce4f6 <+54>:  movq   -0x30(%rbp), %rdi
    0x1264ce4fa <+58>:  leaq   -0x28(%rbp), %rsi
    0x1264ce4fe <+62>:  callq  0x1267b63b0               ; WebCore::operator<< at TransformOperations.cpp:131
    0x1264ce503 <+67>:  leaq   -0x28(%rbp), %rdi
    0x1264ce507 <+71>:  movq   %rax, -0x40(%rbp)
    0x1264ce50b <+75>:  callq  0x124b42840               ; WebCore::TransformOperations::~TransformOperations at TransformOperations.h:34
    0x1264ce510 <+80>:  movq   -0x38(%rbp), %rax

Very weird.

[Simon Fraser (smfr)]

Unified sources :|

[Simon Fraser (smfr)]

We need:

diff --git a/Source/WebCore/page/scrolling/ScrollingTreeOverflowScrollingNode.cpp b/Source/WebCore/page/scrolling/ScrollingTreeOverflowScrollingNode.cpp
index 92376691d74843ee4a67e51365b0f9c6ec720d57..b50fed71246968b5f9257964bd917607bfea8ad2 100644
--- a/Source/WebCore/page/scrolling/ScrollingTreeOverflowScrollingNode.cpp
+++ b/Source/WebCore/page/scrolling/ScrollingTreeOverflowScrollingNode.cpp
@@ -30,6 +30,7 @@
 
 #include "ScrollingStateTree.h"
 #include "ScrollingTree.h"
+#include <wtf/text/TextStream.h>
 
 namespace WebCore {
 
diff --git a/Source/WebCore/platform/graphics/transforms/TransformOperations.h b/Source/WebCore/platform/graphics/transforms/TransformOperations.h
index 1f88d8171c17ff8205a1afe0f42a8f9c524ded80..f8256a7f4d5ab7bb18e6af446030379369821d1d 100644
--- a/Source/WebCore/platform/graphics/transforms/TransformOperations.h
+++ b/Source/WebCore/platform/graphics/transforms/TransformOperations.h
@@ -34,7 +34,7 @@ namespace WebCore {
 class TransformOperations {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    TransformOperations(bool makeIdentity = false);
+    explicit TransformOperations(bool makeIdentity = false);
     
     bool operator==(const TransformOperations& o) const;
     bool operator!=(const TransformOperations& o) const

[Daniel Bates]

Thank you Simon!!!

[Simon Fraser (smfr)]

Will fix via bug 199849.

[Maciej Stachowiak]

Other bug landed, does that mean this patch cannot land again? Should it be resubmitted to EWS?

[Daniel Bates]

Committed r247530: <https://trac.webkit.org/changeset/247530>