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).
<rdar://problem/51616845>
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.
<https://github.com/w3c/uievents/issues/238>
Created attachment 373662 [details] Test
(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.
@ryosuke: If you want to discuss this then talk to me in person and I can explain you what your missing.
(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.
(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.
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.
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.
(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.
Created attachment 373954 [details] For the bots
Created attachment 373960 [details] For the bots (compile with the correct time limit for simulator and device)
Created attachment 373961 [details] For the bots
Created attachment 373972 [details] Patch
Created attachment 373974 [details] Patch and layout test Grammar
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
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
(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 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?
(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 on attachment 373974 [details] Patch and layout test Clearing flags on attachment: 373974 Committed r247444: <https://trac.webkit.org/changeset/247444>
All reviewed patches have been landed. Closing bug.
(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
That sucks. Roll it out. What did I miss?
Reverted r247444 for reason: Caused two scrolling tests to fail on iOS Simulator Committed r247472: <https://trac.webkit.org/changeset/247472>
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.
Unified sources :|
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
Thank you Simon!!!
Will fix via bug 199849.
Other bug landed, does that mean this patch cannot land again? Should it be resubmitted to EWS?
Committed r247530: <https://trac.webkit.org/changeset/247530>