Closed Bug 1405087 Opened 7 years ago Closed 6 years ago

JavaScript: The "click ()" event isn`t executing from the script after deleting/setting to "false" the "disabled" prop of the element "input type = submit"

Categories

(Core :: DOM: Events, defect, P3)

Product:
Core
Component:
DOM: Events
Version:
57 Branch
Platform:
x86_64
Windows 10
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: sergeymkr, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(5 files)

Bug 1405087: Don't use -moz-user-input: disabled to decide event handling stuff.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1405087: Nix -moz-user-input: disabled.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1405087: Make nsGenericHTMLElement::IsDisabled not virtual.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1405087: Test that click events are properly dispatched to non-disabled form controls in presence of dynamic changes.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1405087: Also nix -moz-user-input: enabled.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

*English isn`t my native language, so sorry for any inaccuracies.

We have:
 1) a form with a button (<input type = "submit" name = "1" id = "2" value = "press me" disabled>)
2) the simplest script that, after an interval, removes the "disabled" property and presses the button(the commended variants are also working in other browsers):
var fps = 60;
setInterval(function () {
// document.getElementById("2").disabled = false;
// $('#2').prop('disabled', false); //JQ 3.1.1.min
document.getElementById("2").removeAttribute('disabled');

// document.getElementById("2").click();
$("#2").click();

}, 1000 / fps);


Actual results:

- The "disabled" property is removing / setting to "false" with any variant.
- Automatic clicks from the script is not performed, but it is possible to click manually.
- During the step-by-step debugging, the script click is executed!
- After the "disabled" prop is deleted from the source element, automatic click works as expected.




Expected results:

1. After interval the "disabled" prop is removing/setting to "false".
2. Executing click() with the form sending and reloading the page.
3. Repeat.

*In Chrome, Opera, and others, script works as expected. Only Firefox-problem.
Iteration: --- → 57.3 - Sep 19
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Iteration: 57.3 - Sep 19 → ---
Product: Firefox → Core
can you provide a standalone testcase? (HTML file that contains everything)
I tried creating it from your comment, but it works without any issue (it submits the form).
Flags: needinfo?(sergeymkr)
(In reply to Tooru Fujisawa [:arai] from comment #1)
> can you provide a standalone testcase? (HTML file that contains everything)
> I tried creating it from your comment, but it works without any issue (it
> submits the form).

<!DOCTYPE html>
<html lang="en">
<head>
    <title>Title</title>
    <script>
        setTimeout(function () {
            document.getElementById("2").disabled = false;
            //document.getElementById("2").removeAttribute('disabled');

            document.getElementById("2").click();
            }, 3000);
    </script>
</head>
<body>
<form id="formid" name="theform" method="post" action="">
    <input type = "submit" name = "1" id = "2" value = "press me" disabled>
</form>
</body>
</html>


*SetInterval is replaced with SetTimeout, because with the first one click it is executed only at the second iteration, when "disabled" prop is already deleted on the first one.
*Tested in Chrome 61.0.3163.100 64bit, Opera 48.0.2685.32, it works like expected. An issue checked in Firefox Quantum 57.0b4 64bit & Firefox 56.0 64bit.
Flags: needinfo?(sergeymkr)
Component: Untriaged → DOM: Events
Andreas, please investigate here. Thanks!
status-firefox56: --- → affected
status-firefox57: --- → affected
Flags: needinfo?(afarre)
Priority: -- → P3
Assignee: nobody → afarre
Flags: needinfo?(afarre)
This can be a wpt test looking something like this:

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

<script>
  async_test(t => {
    addEventListener('load', t.step_func(() => {
      let e = document.querySelector('input');
      e.disabled = false;
      e.onclick = t.step_func_done(() => {});
      e.click();
    }));

  }, "Foo")
</script>

<input type="button" value="Click" disabled>

Don't know exactly where it should live though.
Just for reference, since I just bisected this, here's a test-case that fails at least back to Firefox 20:

<input type="button" id="b" value="Click" disabled>
<script>
  document.body.offsetTop;
  var e = document.getElementById('b');
  e.removeAttribute('disabled');
  e.onclick = function() { alert("PASS"); };
  e.click();
</script>
Flushing the styles when dispatching the click event totally tanks speedometer, I see a 10 point drop. So that's probably not a good idea.
Hiya,

I am currently looking for bugs to fix as part of my Open Source Development module at Coventry University and I am interested in developing this bug.

Please could you assign this task to me and give me more information.

This is my first bug fix and any help would be appreciated.

Thank you.
Will this issue be fixed?
Ok, I think I have a way to fix this, though it's not extremely orthodox... Still -moz-user-input: none would be broken, but at least -moz-user-input: disabled seems pointless to me, and we can try to nix it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Oh, I did push one without the last commit before :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b026d291a072b7ec26fa24ffeef5a98f5cd4763
Assignee: afarre → emilio
Flags: needinfo?(emilio)
Comment on attachment 8950087 [details]
Bug 1405087: Don't use -moz-user-input: disabled to decide event handling stuff.

https://reviewboard.mozilla.org/r/219356/#review225252

::: dom/html/HTMLOptGroupElement.cpp:51
(Diff revision 2)
>  HTMLOptGroupElement::GetEventTargetParent(EventChainPreVisitor& aVisitor)
>  {
>    aVisitor.mCanHandle = false;
>    // Do not process any DOM events if the element is disabled
>    // XXXsmaug This is not the right thing to do. But what is?
> -  if (HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {
> +  if (IsDisabled() || HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {

Are there cases in which IsDisabled() is false but the HasAttr() tests true?  Presumably only during attr setting before we call AfterSetAttr(), right?  I really don't expect this code is reachable then.

::: dom/html/nsGenericHTMLElement.cpp:2286
(Diff revision 2)
>        break;
>    }
>  
> -  bool disabled = IsDisabled();
> -  if (!disabled && aFrame) {
> -    const nsStyleUserInterface* uiStyle = aFrame->StyleUserInterface();
> +  // FIXME(emilio): This poking at the style of the frame is slightly bogus
> +  // unless we flush before every event, which we don't really want to do.
> +  if (aFrame &&

Why are we doing this check before IsDisabled()?  Which one is more likely to match?  To be cheaper?

I guess in almost all cases they're both going to be false....

::: layout/generic/nsFrame.cpp:6554
(Diff revision 2)
> +  // to date, and they don't!
> +  if (StyleUserInterface()->mUserInput == StyleUserInput::None) {
> +    return true;
> +  }
> +
> +  auto* element = nsGenericHTMLElement::FromContent(GetContent());

Should really be FromContentOrNull, if we're going to expose this on all frames.
Comment on attachment 8950087 [details]
Bug 1405087: Don't use -moz-user-input: disabled to decide event handling stuff.

Would like Olli to ok this.
Attachment #8950087 - Flags: review?(bzbarsky) → review?(bugs)
Comment on attachment 8950087 [details]
Bug 1405087: Don't use -moz-user-input: disabled to decide event handling stuff.

https://reviewboard.mozilla.org/r/219356/#review225268

::: dom/html/HTMLOptGroupElement.cpp:51
(Diff revision 2)
>  HTMLOptGroupElement::GetEventTargetParent(EventChainPreVisitor& aVisitor)
>  {
>    aVisitor.mCanHandle = false;
>    // Do not process any DOM events if the element is disabled
>    // XXXsmaug This is not the right thing to do. But what is?
> -  if (HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {
> +  if (IsDisabled() || HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {

Hmm, yeah, I should kill the HasAttr check.

::: layout/generic/nsFrame.cpp:6554
(Diff revision 2)
> +  // to date, and they don't!
> +  if (StyleUserInterface()->mUserInput == StyleUserInput::None) {
> +    return true;
> +  }
> +
> +  auto* element = nsGenericHTMLElement::FromContent(GetContent());

Fair enough :)
Comment on attachment 8950087 [details]
Bug 1405087: Don't use -moz-user-input: disabled to decide event handling stuff.

https://reviewboard.mozilla.org/r/219356/#review225264

In particular, this is changing behavior in various cases when @disabled is set but also `-moz-user-input: enabled` is set.  I don't know whether we care about those cases in practice.  It's also changing behavior when @disabled is not set but `-moz-user-input: disabled` is.

Looking around a bit, `-moz-user-input: disabled` is used in https://github.com/poroussel/notation/blob/9b99596daeb2a54f96ae17cd36166a1278fe512f/static/css/base.css#L62-L68 (though the lack of leading '=' there may mean it does not work) or https://github.com/sleeplessor/porkin/blob/89fc90d7c70a1eedf1973e53f3ca8b24215a7772/digiagent/src/main/webapp/styles/main.css#L211-L213 (same deal?).  

Mauybe this is OK to remove...
Attachment #8950087 - Flags: review?(bzbarsky)
Attachment #8950087 - Flags: review?(bzbarsky)
Comment on attachment 8950088 [details]
Bug 1405087: Nix -moz-user-input: disabled.

https://reviewboard.mozilla.org/r/219358/#review225286

There's a non-obvious mention at https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/dom/interfaces/base/nsIDOMWindowUtils.idl#1698 that should go away.

::: commit-message-e6a3c:3
(Diff revision 2)
> +Bug 1405087: Nix -moz-user-input: disabled. r?bz
> +
> +It does nothing, and there's only one use of it anyway.

This needs at least some indication of what uses outside our tree look like...
Attachment #8950088 - Flags: review?(bzbarsky) → review+
Comment on attachment 8950089 [details]
Bug 1405087: Make nsGenericHTMLElement::IsDisabled not virtual.

https://reviewboard.mozilla.org/r/219360/#review225318

r=me.  I guess we could have done this in bug 1375599 when we started using the event state, not the attribute...
Attachment #8950089 - Flags: review?(bzbarsky) → review+
Comment on attachment 8950090 [details]
Bug 1405087: Test that click events are properly dispatched to non-disabled form controls in presence of dynamic changes.

https://reviewboard.mozilla.org/r/219362/#review225320

::: commit-message-0690f:1
(Diff revision 2)
> +Bug 1405087: Test that click events are properly dispatched to non-disabled form controls in presence of dynamic changes. r?bz

Maybe add a note here, or in the test, that the test will time out when it fails?
Attachment #8950090 - Flags: review?(bzbarsky) → review+
Comment on attachment 8950102 [details]
Bug 1405087: Also nix -moz-user-input: enabled.

https://reviewboard.mozilla.org/r/219376/#review225324

I was going to ask :paul to take a look, but....

Again, need to have some info in commit message about out-of-tree usage.
Attachment #8950102 - Flags: review?(bzbarsky) → review+
Comment on attachment 8950087 [details]
Bug 1405087: Don't use -moz-user-input: disabled to decide event handling stuff.

https://reviewboard.mozilla.org/r/219356/#review225834
Attachment #8950087 - Flags: review?(bugs) → review+
Keywords: site-compat
Comment on attachment 8950087 [details]
Bug 1405087: Don't use -moz-user-input: disabled to decide event handling stuff.

Sigh, mozreview... will land manually when trees reopen.
Attachment #8950087 - Flags: review?(bzbarsky) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/20305af66e39
Don't use -moz-user-input: disabled to decide event handling stuff. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3e305428625d
Nix -moz-user-input: disabled. r=bz
https://hg.mozilla.org/integration/autoland/rev/f0b997d20591
Make nsGenericHTMLElement::IsDisabled not virtual. r=bz
https://hg.mozilla.org/integration/autoland/rev/aad5349b3458
Test that click events are properly dispatched to non-disabled form controls in presence of dynamic changes. r=bz
https://hg.mozilla.org/integration/autoland/rev/cb1d0640b36e
Also nix -moz-user-input: enabled. r=bz
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9549 for changes under testing/web-platform/tests
Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.
Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.
Depends on: 1461706
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: