Normalize behaviour of space vs. return for activating elements #22782

jdm · 2019-01-30T00:33:10Z

Right now, Document::dispatch_key_event triggers different behaviour for elements if you activate them with spacebar or return. Spacebar performs a synthetic click activation, while Return triggers implicit form submission. This means that pressing Return with an anchor element active does not navigate to the link, and special cases the behaviour for text input elements and buttons. I propose the following:

remove the implicit_submission method from the Activatable trait
remove the implicit_submission implementation for HTMLButtonElement entirely
make the implicit_submission implementation for HTMLInputElement a regular method
treat the Space and Return cases the same in dispatch_key_event; remove any reference to implicit submission

This should allow implicit submissions of forms from text inputs to continue working, since there is code in HTMLInputElement::handle_event to recognize the Return key already. Activating buttons with the Return key should also work as expected, since they will receive a synthetic click event.

Code: components/script/dom/activation.rs, components/script/dom/document.rs, components/script/dom/htmlinputelement.rs, components/script/htmlbuttonelement.rs

Some tests that might be relevant:

./mach test-wpt tests/wpt/web-platform-tests/html/semantics/forms/the-input-element/
./mach test-wpt tests/wpt/web-platform-tests/html/semantics/forms/attributes-common-to-form-controls/
./mach test-wpt tests/wpt/web-platform-tests/html/semantics/forms/form-submission-0/

highfive · 2019-01-30T00:33:11Z

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

nishanthkarthik · 2019-01-30T17:00:40Z

@highfive assign me

highfive · 2019-01-30T17:00:42Z

Hey @nishanthkarthik! Thanks for your interest in working on this issue. It's now assigned to you!

nishanthkarthik · 2019-01-30T17:01:05Z

@jdm This is my first issue on servo. Let me give it a shot!

jdm · 2019-02-13T22:40:30Z

@nishanthkarthik Have you made progress?

nishanthkarthik · 2019-02-14T01:20:21Z

I'm on the test cases. I expect to send a PR by this weekend

nishanthkarthik · 2019-02-16T11:17:22Z

@jdm I tried dispatchEvent with keydown event this way.

<!doctype html>
<meta charset="utf-8">
<title>Click action on label</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>

<div id="test"></div>
<a id="anch" href="about:blank">test</a>

<script>
async_test(function(t) {
        document.querySelector("#test").innerHTML = '<iframe src=html-anchor-navigation.html \
            id="foo" name="foo"></iframe>'

        var loaded = false;
        var iframe = document.getElementById('foo');
        iframe.onload = t.step_func(function() {
            if (!loaded) {
                loaded = true;
                var ev = new KeyboardEvent('keydown', { code: "Space" });
                var elem = iframe.contentDocument.getElementById('anc');
                elem.focus();
                elem.dispatchEvent(ev);
            } else {
                t.done();
            }
        });
    }, "space keydown should navigate away");
</script>

When I manually press space, it navigates away. It does not, when it is done through dispatchEvent. I also tried using testdriver.js to send the event, but it doesn't support sending it to a node in an iframe.

jdm · 2019-03-05T19:08:35Z

Oh, right. That makes sense, because there is no code in htmlanchorelement.rs to recognize key events and act on them.

That being said, I just tested Firefox and Chrome, and they don't actually invoke links on space, so we should not deviate from their behaviour. However, they do activate a <button> element on space, so we should make sure we still support that.

jdm · 2019-03-05T19:09:49Z

I think the right choice may be to add custom handle_event implementations for elements like <button> that should recognize both space and enter: https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/dom/html/HTMLButtonElement.cpp#233-251

That will allow us to write automated tests that verify that property.

stevenloppe · 2019-05-22T22:16:14Z

@highfive: assign me

highfive · 2019-05-22T22:16:17Z

Hey @stevenloppe! Thanks for your interest in working on this issue. It's now assigned to you!

dralley · 2019-06-24T18:13:37Z

@stevenloppe Are you working on this?

jdm · 2019-07-12T20:52:49Z

Unassigning due to lack of activity.

Ersikan · 2019-07-19T13:50:16Z

@highfive: assign me

highfive · 2019-07-19T13:50:18Z

Hey @Ersikan! Thanks for your interest in working on this issue. It's now assigned to you!

jdm · 2019-10-10T18:23:37Z

Unassigning due to lack of activity.

garasubo · 2019-10-11T06:26:14Z

@highfive assign me

highfive · 2019-10-11T06:26:17Z

Hey @garasubo! Thanks for your interest in working on this issue. It's now assigned to you!


        [WIP] solve servo#22782

TODO: write tests for my change


        [WIP] solve servo#22782

TODO: write tests for my change


        [WIP] solve servo#22782

TODO: write tests for my change


        [WIP] solve servo#22782

TODO: write tests for my change


        Auto merge of #24489 - garasubo:refactor-submission, r=<try>

Refactor submission This is my first contribution, so please correct me if I'm wrong I refactored the code as described #22782. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #22782 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #24489 - garasubo:refactor-submission, r=nox

Refactor submission This is my first contribution, so please correct me if I'm wrong I refactored the code as described #22782. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #22782 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Auto merge of #24489 - garasubo:refactor-submission, r=nox

Refactor submission This is my first contribution, so please correct me if I'm wrong I refactored the code as described #22782. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #22782 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___


        Re-add missing commit in PR servo#24489

Fix servo#22782

garasubo · 2019-10-25T07:18:25Z

I'm sorry, but I found that one of commits were not included in the previous PR. I resubmit another PR to add this commit #24546. Could you take a look?


        Fix the condition to keep original logic

Address this PR comment: https://github.com/servo/servo/pull/24489/files#diff-30a18e04d7e0b66aafdf192e416cad44R1472 This commit was failed to add in PR servo#24489. Issue: servo#22782


        Auto merge of #24546 - garasubo:refactor-submission, r=paulrouget

Re-add missing commit in PR #24489 Fix #22782 I'm sorry, but I failed to add one of commits during the squash phase in #24489. Could you review and merge this? ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

jdm added A-content/dom E-easy I-refactor labels Jan 30, 2019

jdm mentioned this issue Jan 30, 2019

[WIP] script: Partially implement sequential focus navigation #20075

Closed

0 of 4 tasks complete

highfive added the C-assigned label Jan 30, 2019

nishanthkarthik mentioned this issue Feb 16, 2019

[WIP] Modify criteria for synthetic click event on Enter/Space #22904

Closed

0 of 5 tasks complete

jdm added the C-has open PR label Mar 20, 2019

ferjm removed C-assigned C-has open PR labels May 15, 2019

highfive added the C-assigned label May 22, 2019

jdm removed the C-assigned label Jul 12, 2019

highfive added the C-assigned label Jul 19, 2019

jdm removed the C-assigned label Oct 10, 2019

highfive added the C-assigned label Oct 11, 2019

garasubo added a commit to garasubo/servo that referenced this issue Oct 12, 2019

[WIP] solve servo#22782

2b42b0c

TODO: write tests for my change

garasubo added a commit to garasubo/servo that referenced this issue Oct 18, 2019

[WIP] solve servo#22782

658ca3e

TODO: write tests for my change

garasubo added a commit to garasubo/servo that referenced this issue Oct 18, 2019

[WIP] solve servo#22782

4791c64

TODO: write tests for my change

garasubo added a commit to garasubo/servo that referenced this issue Oct 18, 2019

[WIP] solve servo#22782

7ce4008

TODO: write tests for my change

garasubo mentioned this issue Oct 18, 2019

Refactor submission #24489

Merged

2 of 5 tasks complete

bors-servo closed this in e265303 Oct 23, 2019

garasubo added a commit to garasubo/servo that referenced this issue Oct 25, 2019

Re-add missing commit in PR servo#24489

48bbb46

Fix servo#22782

garasubo mentioned this issue Oct 25, 2019

Re-add missing commit in PR #24489 #24546

Merged

2 of 5 tasks complete

servo / servo

Normalize behaviour of space vs. return for activating elements #22782

Normalize behaviour of space vs. return for activating elements #22782

jdm commented Jan 30, 2019 •

edited

highfive commented Jan 30, 2019

nishanthkarthik commented Jan 30, 2019

highfive commented Jan 30, 2019

nishanthkarthik commented Jan 30, 2019

jdm commented Feb 13, 2019

nishanthkarthik commented Feb 14, 2019

nishanthkarthik commented Feb 16, 2019

jdm commented Mar 5, 2019

jdm commented Mar 5, 2019

stevenloppe commented May 22, 2019

highfive commented May 22, 2019

dralley commented Jun 24, 2019

jdm commented Jul 12, 2019

Ersikan commented Jul 19, 2019

highfive commented Jul 19, 2019

jdm commented Oct 10, 2019

garasubo commented Oct 11, 2019

highfive commented Oct 11, 2019

garasubo commented Oct 25, 2019

servo / servo

Join GitHub today

Normalize behaviour of space vs. return for activating elements #22782

Normalize behaviour of space vs. return for activating elements #22782

Comments

jdm commented Jan 30, 2019 • edited

highfive commented Jan 30, 2019

nishanthkarthik commented Jan 30, 2019

highfive commented Jan 30, 2019

nishanthkarthik commented Jan 30, 2019

jdm commented Feb 13, 2019

nishanthkarthik commented Feb 14, 2019

nishanthkarthik commented Feb 16, 2019

jdm commented Mar 5, 2019

jdm commented Mar 5, 2019

stevenloppe commented May 22, 2019

highfive commented May 22, 2019

dralley commented Jun 24, 2019

jdm commented Jul 12, 2019

Ersikan commented Jul 19, 2019

highfive commented Jul 19, 2019

jdm commented Oct 10, 2019

garasubo commented Oct 11, 2019

highfive commented Oct 11, 2019

garasubo commented Oct 25, 2019

jdm commented Jan 30, 2019 •

edited