Bug 199587 - Typing into a cell in a Google Sheet lags behind by one character
Summary: Typing into a cell in a Google Sheet lags behind by one character
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Daniel Bates
URL: http://docs.google.com/sheets
Keywords: InRadar, PlatformOnly
Depends on:
Blocks: 190571
  Show dependency treegraph
 
Reported: 2019-07-08 14:09 PDT by Daniel Bates
Modified: 2019-07-17 15:29 PDT (History)
15 users (show)

See Also:


Attachments
Test (949 bytes, text/html)
2019-07-08 14:13 PDT, Daniel Bates
no flags Details
For the bots (22.86 KB, patch)
2019-07-11 14:29 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (compile with the correct time limit for simulator and device) (22.86 KB, patch)
2019-07-11 14:54 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
For the bots (22.89 KB, patch)
2019-07-11 14:56 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (32.94 KB, patch)
2019-07-11 16:15 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (32.94 KB, patch)
2019-07-11 16:19 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.70 MB, application/zip)
2019-07-11 18:12 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-07-08 14:09:16 PDT
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).
Comment 1 Daniel Bates 2019-07-08 14:09:32 PDT
<rdar://problem/51616845>
Comment 2 Daniel Bates 2019-07-08 14:12:27 PDT
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.
Comment 3 Daniel Bates 2019-07-08 14:12:53 PDT
<https://github.com/w3c/uievents/issues/238>
Comment 4 Daniel Bates 2019-07-08 14:13:21 PDT
Created attachment 373662 [details]
Test
Comment 5 Ryosuke Niwa 2019-07-08 20:28:49 PDT
(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.
Comment 6 Daniel Bates 2019-07-08 21:48:29 PDT
@ryosuke: If you want to discuss this then talk to me in person and I can explain you what your missing.
Comment 7 Ryosuke Niwa 2019-07-08 22:31:38 PDT
(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.
Comment 8 Ryosuke Niwa 2019-07-08 22:43:46 PDT
(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.
Comment 9 Geoffrey Garen 2019-07-09 10:08:49 PDT
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.
Comment 10 Geoffrey Garen 2019-07-09 10:13:17 PDT
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.
Comment 11 Ryosuke Niwa 2019-07-09 10:48:56 PDT
(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.
Comment 12 Daniel Bates 2019-07-11 14:29:47 PDT
Created attachment 373954 [details]
For the bots
Comment 13 Daniel Bates 2019-07-11 14:54:47 PDT
Created attachment 373960 [details]
For the bots (compile with the correct time limit for simulator and device)
Comment 14 Daniel Bates 2019-07-11 14:56:10 PDT
Created attachment 373961 [details]
For the bots
Comment 15 Daniel Bates 2019-07-11 16:15:51 PDT
Created attachment 373972 [details]
Patch
Comment 16 Daniel Bates 2019-07-11 16:19:00 PDT
Created attachment 373974 [details]
Patch and layout test

Grammar
Comment 17 Build Bot 2019-07-11 18:12:44 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2019-07-11 18:12:46 PDT Comment hidden (obsolete)
Comment 19 Daniel Bates 2019-07-11 22:13:44 PDT
(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.
Comment 20 Brent Fulgham 2019-07-15 11:34:26 PDT
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?
Comment 21 Daniel Bates 2019-07-15 13:39:03 PDT
(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.
Comment 22 Daniel Bates 2019-07-15 13:40:47 PDT
Comment on attachment 373974 [details]
Patch and layout test

Clearing flags on attachment: 373974

Committed r247444: <https://trac.webkit.org/changeset/247444>
Comment 23 Daniel Bates 2019-07-15 13:40:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Ryan Haddad 2019-07-15 20:43:46 PDT
(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
Comment 25 Daniel Bates 2019-07-15 21:20:45 PDT
That sucks. Roll it out.

What did I miss?
Comment 26 Ryan Haddad 2019-07-15 21:27:59 PDT
Reverted r247444 for reason:

Caused two scrolling tests to fail on iOS Simulator

Committed r247472: <https://trac.webkit.org/changeset/247472>
Comment 27 Simon Fraser (smfr) 2019-07-16 18:55:10 PDT
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.
Comment 28 Simon Fraser (smfr) 2019-07-16 18:59:57 PDT
Unified sources :|
Comment 29 Simon Fraser (smfr) 2019-07-16 20:16:37 PDT
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
Comment 30 Daniel Bates 2019-07-16 20:56:21 PDT
Thank you Simon!!!
Comment 31 Simon Fraser (smfr) 2019-07-16 20:56:54 PDT
Will fix via bug 199849.
Comment 32 Maciej Stachowiak 2019-07-16 22:35:42 PDT
Other bug landed, does that mean this patch cannot land again? Should it be resubmitted to EWS?
Comment 33 Daniel Bates 2019-07-17 13:06:10 PDT
Committed r247530: <https://trac.webkit.org/changeset/247530>