Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1840993002:
Reland "Add InputConnection-to-JavaScript tests for key input" (Closed)
|
Created:
4 years, 8 months ago by Changwan Ryu Modified:
4 years, 7 months ago CC:
chongz, chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo, yosin_UTC9, yabinh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland "Add InputConnection-to-JavaScript tests for key input"
Fixed flakiness by fitting all the elements into a single page.
I suspect that scrolling in DOMUtils#scrollNodeIntoView isn't working
correctly. The same issue was investigated by yabinh@ to enable
FocusedEditableTextFieldZoomTest at some point.
>
>commitText() is in effect almost the same as calling setComposingText() and
>finishComposingText() in a batch edit.
>
>However, one difference is that the latter triggers composition events
>in the DOM. This test clarifies the difference.
>
>This is the first Java-to-JavaScript test for key input system.
>
>BUG=578016
>
>Review-Url: https://codereview.chromium.org/1840993002
>Cr-Commit-Position: refs/heads/master@{#391176}
BUG=578016, 608656
Committed: https://crrev.com/05c378251e5bffa1567b182ab05fa05f4aea732b
Cr-Commit-Position: refs/heads/master@{#393132}
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase #Patch Set 3 : added more event types #Patch Set 4 : fixed flakiness #
Messages
Total messages: 32 (13 generated)
changwan@chromium.org changed reviewers: + aelias@chromium.org, tedchoc@chromium.org
https://codereview.chromium.org/1840993002/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (right): https://codereview.chromium.org/1840993002/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1089: public void testCommitTextDoesNotTriggerCompositionEvents() throws Throwable { Before we lock the current behavior into a test, is there something like a spec document basis for believing that this (non-)eventing behavior is the most correct? Looking back at https://codereview.chromium.org/1593533002/, I felt strongly that we shouldn't send 2 IPCs, but that doesn't mean I think it would necessarily be wrong to send 2 events from the one commit IPC on the Blink side.
On 2016/03/30 06:30:51, aelias wrote: > https://codereview.chromium.org/1840993002/diff/1/content/public/android/java... > File > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java > (right): > > https://codereview.chromium.org/1840993002/diff/1/content/public/android/java... > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1089: > public void testCommitTextDoesNotTriggerCompositionEvents() throws Throwable { > Before we lock the current behavior into a test, is there something like a spec > document basis for believing that this (non-)eventing behavior is the most > correct? > > Looking back at https://codereview.chromium.org/1593533002/, I felt strongly > that we shouldn't send 2 IPCs, but that doesn't mean I think it would > necessarily be wrong to send 2 events from the one commit IPC on the Blink side. I see. According to W3C UI Events 6.3.3 Dead Keys (https://www.w3.org/TR/uievents/#keys-dead), in response to any dead key press, composition events must be dispatched by the user agent. I'll change the renderer to dispatch composition events for commitText().
On 2016/03/31 07:01:39, Changwan Ryu wrote: > On 2016/03/30 06:30:51, aelias wrote: > > > https://codereview.chromium.org/1840993002/diff/1/content/public/android/java... > > File > > > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java > > (right): > > > > > https://codereview.chromium.org/1840993002/diff/1/content/public/android/java... > > > content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:1089: > > public void testCommitTextDoesNotTriggerCompositionEvents() throws Throwable { > > Before we lock the current behavior into a test, is there something like a > spec > > document basis for believing that this (non-)eventing behavior is the most > > correct? > > > > Looking back at https://codereview.chromium.org/1593533002/, I felt strongly > > that we shouldn't send 2 IPCs, but that doesn't mean I think it would > > necessarily be wrong to send 2 events from the one commit IPC on the Blink > side. > I see. According to W3C UI Events 6.3.3 Dead Keys > (https://www.w3.org/TR/uievents/#keys-dead), > in response to any dead key press, composition events must be dispatched by the > user agent. > > I'll change the renderer to dispatch composition events for commitText(). Sorry for reviving this old patch. On a second thought, I think we should not dispatch composition events for commitText(). We have been, though implicitly, guiding website developers to listen to the dead key events and check the input box value themselves for changes. On Android, currently setComposingText(), commitText() and deleteSurroundingText() emit dead keys before and after editing the text. commitText() may actually be replaced by setComposingText() and finishComposingText() so it can also emit composition events, but emitting composition event for deleteSurroundingText() does not make any sense. In this sense, we should stop dead key events for commitText() and deleteSurroundingText(), and ask web developers to listen to 'edit' or other signals, and therefore we should not dispatch composition events for commitText().
OK. At a higher level, your mental model implies that composition events are for displaying temporary UI (highlights/underlines/menus) for the ongoing composition, but they are not needed for commitText and deleteSurroundingText which are instantaneous changes. That makes sense to me, lgtm.
On 2016/04/29 02:05:50, aelias wrote: > OK. At a higher level, your mental model implies that composition events are > for displaying temporary UI (highlights/underlines/menus) for the ongoing > composition, but they are not needed for commitText and deleteSurroundingText > which are instantaneous changes. That makes sense to me, lgtm. Thanks. It turns out that there are other changes (e.g. deletion behavior change) that can potentially have dependency on this CL. So let me merge this as is and consider removing dead keys in a separate CL. tedchoc@, could you take a look at ContentShellTestBase.java?
On 2016/05/02 07:54:40, Changwan Ryu wrote: ContentShellTestBase.java - lgtm
The CQ bit was checked by changwan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org Link to the patchset: https://codereview.chromium.org/1840993002/#ps40001 (title: "added more event types")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840993002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840993002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840993002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840993002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add InputConnection-to-JavaScript tests for key input commitText() is in effect almost the same as calling setComposingText() and finishComposingText() in a batch edit. However, one difference is that the latter triggers composition events in the DOM. This test clarifies the difference. This is the first Java-to-JavaScript test for key input system. BUG=578016 ========== to ========== Add InputConnection-to-JavaScript tests for key input commitText() is in effect almost the same as calling setComposingText() and finishComposingText() in a batch edit. However, one difference is that the latter triggers composition events in the DOM. This test clarifies the difference. This is the first Java-to-JavaScript test for key input system. BUG=578016 Committed: https://crrev.com/b2d5ef4470d26f1f5e5d02fefca18af139e4a744 Cr-Commit-Position: refs/heads/master@{#391176} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b2d5ef4470d26f1f5e5d02fefca18af139e4a744 Cr-Commit-Position: refs/heads/master@{#391176}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1943233002/ by henrika@chromium.org. The reason for reverting is: Breaks tests on Android, see e.g. https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test....
Message was sent while issue was closed.
Description was changed from ========== Add InputConnection-to-JavaScript tests for key input commitText() is in effect almost the same as calling setComposingText() and finishComposingText() in a batch edit. However, one difference is that the latter triggers composition events in the DOM. This test clarifies the difference. This is the first Java-to-JavaScript test for key input system. BUG=578016 Committed: https://crrev.com/b2d5ef4470d26f1f5e5d02fefca18af139e4a744 Cr-Commit-Position: refs/heads/master@{#391176} ========== to ========== Reland "Add InputConnection-to-JavaScript tests for key input" Fixed flakiness by fitting all the elements into a single page. I suspect that scrolling in DOMUtils#scrollNodeIntoView isn't working correctly. The same issue was investigated by yabinh@ to enable FocusedEditableTextFieldZoomTest at some point. > >commitText() is in effect almost the same as calling setComposingText() and >finishComposingText() in a batch edit. > >However, one difference is that the latter triggers composition events >in the DOM. This test clarifies the difference. > >This is the first Java-to-JavaScript test for key input system. > >BUG=578016 > >Review-Url: https://codereview.chromium.org/1840993002 >Cr-Commit-Position: refs/heads/master@{#391176} BUG=578016, 608656 ==========
The CQ bit was checked by changwan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, aelias@chromium.org Link to the patchset: https://codereview.chromium.org/1840993002/#ps60001 (title: "fixed flakiness")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840993002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840993002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840993002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840993002/60001
Message was sent while issue was closed.
Description was changed from ========== Reland "Add InputConnection-to-JavaScript tests for key input" Fixed flakiness by fitting all the elements into a single page. I suspect that scrolling in DOMUtils#scrollNodeIntoView isn't working correctly. The same issue was investigated by yabinh@ to enable FocusedEditableTextFieldZoomTest at some point. > >commitText() is in effect almost the same as calling setComposingText() and >finishComposingText() in a batch edit. > >However, one difference is that the latter triggers composition events >in the DOM. This test clarifies the difference. > >This is the first Java-to-JavaScript test for key input system. > >BUG=578016 > >Review-Url: https://codereview.chromium.org/1840993002 >Cr-Commit-Position: refs/heads/master@{#391176} BUG=578016, 608656 ========== to ========== Reland "Add InputConnection-to-JavaScript tests for key input" Fixed flakiness by fitting all the elements into a single page. I suspect that scrolling in DOMUtils#scrollNodeIntoView isn't working correctly. The same issue was investigated by yabinh@ to enable FocusedEditableTextFieldZoomTest at some point. > >commitText() is in effect almost the same as calling setComposingText() and >finishComposingText() in a batch edit. > >However, one difference is that the latter triggers composition events >in the DOM. This test clarifies the difference. > >This is the first Java-to-JavaScript test for key input system. > >BUG=578016 > >Review-Url: https://codereview.chromium.org/1840993002 >Cr-Commit-Position: refs/heads/master@{#391176} BUG=578016, 608656 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Reland "Add InputConnection-to-JavaScript tests for key input" Fixed flakiness by fitting all the elements into a single page. I suspect that scrolling in DOMUtils#scrollNodeIntoView isn't working correctly. The same issue was investigated by yabinh@ to enable FocusedEditableTextFieldZoomTest at some point. > >commitText() is in effect almost the same as calling setComposingText() and >finishComposingText() in a batch edit. > >However, one difference is that the latter triggers composition events >in the DOM. This test clarifies the difference. > >This is the first Java-to-JavaScript test for key input system. > >BUG=578016 > >Review-Url: https://codereview.chromium.org/1840993002 >Cr-Commit-Position: refs/heads/master@{#391176} BUG=578016, 608656 ========== to ========== Reland "Add InputConnection-to-JavaScript tests for key input" Fixed flakiness by fitting all the elements into a single page. I suspect that scrolling in DOMUtils#scrollNodeIntoView isn't working correctly. The same issue was investigated by yabinh@ to enable FocusedEditableTextFieldZoomTest at some point. > >commitText() is in effect almost the same as calling setComposingText() and >finishComposingText() in a batch edit. > >However, one difference is that the latter triggers composition events >in the DOM. This test clarifies the difference. > >This is the first Java-to-JavaScript test for key input system. > >BUG=578016 > >Review-Url: https://codereview.chromium.org/1840993002 >Cr-Commit-Position: refs/heads/master@{#391176} BUG=578016, 608656 Committed: https://crrev.com/05c378251e5bffa1567b182ab05fa05f4aea732b Cr-Commit-Position: refs/heads/master@{#393132} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/05c378251e5bffa1567b182ab05fa05f4aea732b Cr-Commit-Position: refs/heads/master@{#393132} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
