Issue 2832073003: Send mouse events for disabled form controls.

dtapuska

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-24 16:24:52 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2832073003/20001

3 years, 8 months ago (2017-04-24 16:25:31 UTC) #2

dtapuska

dtapuska@chromium.org changed reviewers: + jbroman@chromium.org

3 years, 8 months ago (2017-04-24 17:09:20 UTC) #3

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-24 18:17:22 UTC) #5

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/428784)

3 years, 8 months ago (2017-04-24 18:17:23 UTC) #6

jbroman

apologies for the large number of nits https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html File third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html (right): https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html#newcode1 third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:1: <!doctype html> ...

3 years, 8 months ago (2017-04-24 18:19:49 UTC) #7

dtapuska

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html File third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html (right): https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html#newcode1 third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:1: <!doctype html> On 2017/04/24 18:19:49, jbroman wrote: > Should ...

3 years, 8 months ago (2017-04-24 19:14:40 UTC) #8

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html
(right):

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:1:
<!doctype html>
On 2017/04/24 18:19:49, jbroman wrote:
> Should this be shared with web-platform-tests at some point?

Yes I need to get the HTML spec clarification pushed and approved. Then this
will move.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:9:
#target0 {
On 2017/04/24 18:19:48, jbroman wrote:
> Are these properties significant? It's not obvious to me, for instance,
whether
> touch-action affects the result of this test. Any reason not to use default
> styling?

Done.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:25:
<div id="complete-notice">
On 2017/04/24 18:19:49, jbroman wrote:
> How much of this boilerplate is necessary? e.g. complete-notice seems copied
> from pointerevents stuff, but generally we've kept our tests a little simpler.

Done.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:30:
var detected_pointertypes = {};
On 2017/04/24 18:19:49, jbroman wrote:
> unused variable

Ugh; you are correct. These were used in a previous version where I simulated
the events using eventSender but I abandoned that because sending synthetic
events ended up being identical.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:34:
function resetTestState() {
On 2017/04/24 18:19:49, jbroman wrote:
> Unused function?

Done.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:37:
var test = async_test("mouse test");
On 2017/04/24 18:19:49, jbroman wrote:
> Is this test actually async? Event dispatch is synchronous, so this seems like
> this test could just be this:
> 
> let target = document.getElementById('target0');
> let eventList = ['mouseout', 'mouseover', 'mouseenter', 'mousemove',
> 'mousedown', 'mouseup', 'mouseleave'];
> 
> for (let eventName of eventList) {
>     test(() => {
>         let fired = false;
>         on_event(target, eventName, () => fired = true);
>         target.dispatchEvent(new MouseEvent(eventName));
>         assert_true(fired, eventName + ' should be fired');
>     }, eventName + ' should be fired');
> }
> 
> test(() => {
>     on_event(target, 'click', t => t.unreached_func('click should not fire on
> disabled element'));
>     on_event(document.body, 'click', t => t.unreached_func('click should not
> fire on event path'));
>     target.click();
> }, 'click should not be fired on disabled elements or evenet path');

Done.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:42:
detected_eventTypes[event.type] = true;
On 2017/04/24 18:19:49, jbroman wrote:
> nit: This seems to just be used as a counter, right? But because dispatch is
> synchronous, you can just check each event as it happens, right? If not, maybe
a
> comment justifying the added complexity?

Yes sorry I had cloned this from a async one but then abandoned it.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:50:
assert_unreached("click should not fire on disabled element");
On 2017/04/24 18:19:49, jbroman wrote:
> This will appear in the output, but I'm not sure it will cause the test to
fail
> (as exceptions thrown in event handlers don't propagate). This is what
step_func
> is for (though here you can just use unreached_func).

Done.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventPath.cpp (right):

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventPath.cpp:380: for (size_t i = 0; i <
size(); ++i) {
On 2017/04/24 18:19:49, jbroman wrote:
> super-nit: I'd ordinarily suggest a range-based for loop over
> node_event_contexts_, or maybe even std::any, here, but this seems consistent
> with existing code, so it doesn't matter.

Done.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventPath.h (right):

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventPath.h:82: bool
DisabledFormControlExistsInPath();
On 2017/04/24 18:19:49, jbroman wrote:
> nit: const?

Done.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/MouseEvent.cpp:443: bool
send_to_disabled_forms =
On 2017/04/24 18:19:49, jbroman wrote:
> super-nit: send_to_disabled_controls or something like that? This made me
think
> it applied to forms as a whole being disabled.

Done.

https://codereview.chromium.org/2832073003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/MouseEvent.cpp:451: if
(!mouse_event.isTrusted())
On 2017/04/24 18:19:49, jbroman wrote:
> Does the layout test cover the logic beyond this point? It was my
understanding
> that "new MouseEvent" creates a non-trusted event.

Yes that is correct. But the changed logic is *before* this conditional. The
logic *after* this event check is the legacy path.

dtapuska

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-24 19:14:44 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2832073003/40001

3 years, 8 months ago (2017-04-24 19:15:50 UTC) #10

jbroman

lgtm; thanks https://codereview.chromium.org/2832073003/diff/40001/third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html File third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html (right): https://codereview.chromium.org/2832073003/diff/40001/third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html#newcode20 third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:20: }); super-nit: include test name (else you ...

3 years, 8 months ago (2017-04-24 19:50:33 UTC) #11

dtapuska

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-04-24 19:59:26 UTC) #12

dtapuska

https://codereview.chromium.org/2832073003/diff/40001/third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html File third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html (right): https://codereview.chromium.org/2832073003/diff/40001/third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html#newcode20 third_party/WebKit/LayoutTests/fast/forms/mouseevent_disabled_form_control.html:20: }); On 2017/04/24 19:50:32, jbroman wrote: > super-nit: include ...

3 years, 8 months ago (2017-04-24 19:59:30 UTC) #13

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2832073003/60001

3 years, 8 months ago (2017-04-24 20:00:03 UTC) #14

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 8 months ago (2017-04-24 21:30:39 UTC) #15

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-24 21:30:39 UTC) #16

dtapuska

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org Link to the ...

3 years, 8 months ago (2017-04-24 21:40:31 UTC) #18

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2832073003/60001

3 years, 8 months ago (2017-04-24 21:41:33 UTC) #19

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493070031185090, "parent_rev": "a24efc60c4845e9cf6a54b83c32f4616c7164121", "commit_rev": "af6f27363128a72a89cb581f3073c0e068854308"}

3 years, 8 months ago (2017-04-24 21:49:19 UTC) #20

commit-bot: I haz the power

Description was changed from ========== Send mouse events for disabled form controls. Intent to Implement: ...

3 years, 8 months ago (2017-04-24 21:49:35 UTC) #21

commit-bot: I haz the power

3 years, 8 months ago (2017-04-24 21:49:36 UTC) #22

Message was sent while issue was closed.

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/af6f27363128a72a89cb581f3073...

Issue 2832073003: Send mouse events for disabled form controls. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Fix nits #

Patch Set 4 : Fix nits #

Messages