Closed Bug 1627520 Opened 5 years ago Closed 5 years ago

Use keydown instead of keypress for autocomplete event listeners.

Categories

(Toolkit :: Autocomplete, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.

This is so that we can avoid needing mozSystemGroup (to get keypress for
non-printable keys), which in turn prevents racing with the native key event
listeners, see the second patch in bug 1624657.

This turned out to be a bit tricky, because we need to guarantee the ordering of
the search one-offs handling in the searchbar with the usual autocomplete-input
handling. We could try to move this to the popup subclass but this is already a
bigger patch than what I'd like.

We can also revert the customElements.js change I did in bug 1624657, as the bug
is no longer relevant.

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b92904ec121f Use keydown instead of keypress for autocomplete event listeners. r=mak,Gijs
Backout by btara@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f465e225928f Backed out changeset b92904ec121f for browser_searchbar_keyboard_navigation.js failures CLOSED TREE

Backed out changeset b92904ec121f (bug 1627520) for browser_searchbar_keyboard_navigation.js failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-browser-chrome-e10s-11%2Cm%28bc11%29&fromchange=89d574cfc905fe83fdcff233eba03c0c83dcc571&tochange=f465e225928f903b53b533596f9d77fc95563074&selectedJob=297535671

Backout link: https://hg.mozilla.org/integration/autoland/rev/f465e225928f903b53b533596f9d77fc95563074

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297535671&repo=autoland&lineNumber=13895

....
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - Leaving test bound test_alt_down
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - Entering test bound test_alt_up
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no one-off button should be selected after closing the panel - 
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - Buffered messages logged at 13:29:00
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no one-off button should be selected - 
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the textfield value should be unmodified - 
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the one-off button #7 should be selected - 
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the one-off button #6 should be selected - 
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.469Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the one-off button #5 should be selected - 
[task 2020-04-14T13:29:00.470Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.470Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the one-off button #4 should be selected - 
[task 2020-04-14T13:29:00.478Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the one-off button #3 should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the one-off button #2 should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the one-off button #1 should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no one-off button should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the last one-off button should be selected - 
[task 2020-04-14T13:29:00.480Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | the settings item should be selected - 
[task 2020-04-14T13:29:00.481Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no one-off should be selected anymore - 
[task 2020-04-14T13:29:00.481Z] 13:29:00     INFO - Leaving test bound test_alt_up
[task 2020-04-14T13:29:00.481Z] 13:29:00     INFO - Entering test bound test_accel_down
[task 2020-04-14T13:29:00.481Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | Default engine should have changed - 
[task 2020-04-14T13:29:00.481Z] 13:29:00     INFO - Buffered messages finished
[task 2020-04-14T13:29:00.481Z] 13:29:00     INFO - TEST-UNEXPECTED-FAIL | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - Got 0, expected -1
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - Stack trace:
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:test_is:1302
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - chrome://mochitests/content/browser/browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js:test_accel_down:444
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1044
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:10
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:909
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:918
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | Default engine should have changed - 
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - TEST-UNEXPECTED-FAIL | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - Got 1, expected -1
[task 2020-04-14T13:29:00.482Z] 13:29:00     INFO - Stack trace:
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:test_is:1302
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - chrome://mochitests/content/browser/browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js:test_accel_down:444
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1044
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1079
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:909
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:918
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - GECKO(1859) | [Child 1862: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 20 (0x11c346800) [pid = 1862] [serial = 62] [outer = 0x0] [url = http://mochi.test:8888/browser/browser/components/search/test/browser/searchTelemetryAd.html?s=test&abc=ff]
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - GECKO(1859) | [Child 1862: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 19 (0x13eb6bc00) [pid = 1862] [serial = 68] [outer = 0x0] [url = about:blank]
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - GECKO(1859) | [Child 1862: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 18 (0x11c349400) [pid = 1862] [serial = 65] [outer = 0x0] [url = about:blank]
[task 2020-04-14T13:29:00.483Z] 13:29:00     INFO - GECKO(1859) | [Child 1862: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0x13eb6a800 == 3 [pid = 1862] [id = {94cb9fbc-c6ba-b845-a9c2-df4df11d216a}] [url = https://example.com/?q=query]
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | Default engine should have changed - 
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - TEST-UNEXPECTED-FAIL | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - Got 2, expected -1
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - Stack trace:
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:test_is:1302
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - chrome://mochitests/content/browser/browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js:test_accel_down:444
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1044
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1079
[task 2020-04-14T13:29:00.484Z] 13:29:00     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:909
[task 2020-04-14T13:29:00.485Z] 13:29:00     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:918
[task 2020-04-14T13:29:00.485Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | Default engine should have changed - 
[task 2020-04-14T13:29:00.485Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | no suggestion should be selected - 
[task 2020-04-14T13:29:00.486Z] 13:29:00     INFO - GECKO(1859) | [Child 1861: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0x10ee68000 == 2 [pid = 1861] [id = {065248b5-e626-b649-924e-d1706522d895}] [url = about:blank]
[task 2020-04-14T13:29:00.486Z] 13:29:00     INFO - TEST-PASS | browser/components/search/test/browser/browser_searchbar_keyboard_navigation.js | Default engine should have changed - 
[task 2020-04-14T13:29:00.486Z] 13:29:00     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-04-14T13:29:00.486Z] 13:29:00     INFO - TEST-UNEXPECTED-FAIL
...
Flags: needinfo?(emilio)

Revised a bit the patch. Thanks.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e46624dd8da6 Use keydown instead of keypress for autocomplete event listeners. r=mak,Gijs
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Regressions: 1630765
Blocks: 1633287

Fixing the regression/blocking confusion.

Orthogonal: Emilio, is it worth having a follow-up to convert the places and tree handlers to use keydown, too? What would be the benefit/drawbacks? :-)

No longer blocks: 1633287
Flags: needinfo?(emilio)
Regressions: 1633287

Perhaps, yeah. The benefits are not having to use mozSystemGroup and thus not interacting with the system event listeners, behaving more like the rest of the web.

I don't plan to work on that in the short term, your call if it's worth having the follow-up on file.

Flags: needinfo?(emilio)
Regressions: 1633562
No longer regressions: 1633562
No longer regressions: 1633287
Blocks: 1633695

Emilio, does the autocomplete code that was changed here run for web content autocomplete? If so, should we ask QE to do additional testing for the autocomplete behaviour on websites that are listed in the prefs Masayuki mentioned in bug 1633695 comment 1 (and maybe others)? It sounds like changing which event listener we use for autocomplete may have web-observable effects...

Flags: needinfo?(emilio)

(In reply to :Gijs (he/him) from comment #10)

Emilio, does the autocomplete code that was changed here run for web content autocomplete? If so, should we ask QE to do additional testing for the autocomplete behaviour on websites that are listed in the prefs Masayuki mentioned in bug 1633695 comment 1 (and maybe others)? It sounds like changing which event listener we use for autocomplete may have web-observable effects...

Technically the satchel/ bits do run on content (for stuff like <datalist> and autocomplete). However I'm not sure to which point it can be a behavior change: It doesn't look like that code looks at .defaultPrevented so web content couldn't cancel the event (they can with autocomplete="off" anyhow, I guess), and all those don't change anything the page can really observe afaict. But sanity checking those sites could be valuable I guess? Do you know how to ask QA to check them out? I just tested and the autocomplete popup on icloud.com (which is the only site in that pref masayuki mentions) works fine.

Flags: needinfo?(emilio) → needinfo?(gijskruitbosch+bugs)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

(In reply to :Gijs (he/him) from comment #10)

Emilio, does the autocomplete code that was changed here run for web content autocomplete? If so, should we ask QE to do additional testing for the autocomplete behaviour on websites that are listed in the prefs Masayuki mentioned in bug 1633695 comment 1 (and maybe others)? It sounds like changing which event listener we use for autocomplete may have web-observable effects...

Technically the satchel/ bits do run on content (for stuff like <datalist> and autocomplete). However I'm not sure to which point it can be a behavior change: It doesn't look like that code looks at .defaultPrevented so web content couldn't cancel the event (they can with autocomplete="off" anyhow, I guess), and all those don't change anything the page can really observe afaict. But sanity checking those sites could be valuable I guess? Do you know how to ask QA to check them out? I just tested and the autocomplete popup on icloud.com (which is the only site in that pref masayuki mentions) works fine.

If we wanted to ask QA to do regression testing, we'd need to file a PI ticket. However, between your comment and looking at the web compat issues listed as "see also" on https://bugzilla.mozilla.org/show_bug.cgi?id=968056 I'm not sure it's necessary. Your call. :-)

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)

Yeah, I'm moderately confident it won't be.

Flags: needinfo?(emilio)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: