Bug 53296

Created attachment 80438 [details] Testcase

This should be a super-easy fix. Mathias, wanna give it a whirl? :)

I call dibs as per comment #2.

Created attachment 80564 [details] Proposed patch Needs review.

Comment on attachment 80564 [details] Proposed patch Looks reasonable. All that it's missing to be a good patch is: a) Layout test -- use your reduction to create one. Read up layout tests on webkit.org/blog. b) ChangeLog. -- use Tools/Scripts/webkit-patch to upload the patch. It will ensure your patch has the proper log.

Created attachment 80632 [details] Revised patch Am I doing this right?

Comment on attachment 80632 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=80632&action=review > Source/WebCore/ChangeLog:5 > + Implicit form submission shouldnât work when the default button is disabled https://bugs.webkit.org/show_bug.cgi?id=53296 What's that odd character in shouldn't? Bug URL should go on a different line. > Source/WebCore/ChangeLog:6 > + There is usually a mention of a test here. Like so: Test: fast/forms/implicit-submission.html > Source/WebCore/html/HTMLFormElement.cpp:188 > + if (formElement->getAttribute(typeAttr).lower() == "submit" || formElement->hasLocalName(buttonTag)) { This is horrible. We shouldn't need to reach into attributes at this level. Why did you change this? > LayoutTests/ChangeLog:5 > + Implicit form submission shouldnât work when the default button is disabled https://bugs.webkit.org/show_bug.cgi?id=53296 Same here. > LayoutTests/fast/forms/implicit-submission.html:-19 > - [ "Text input and text area and a disabled submit, text input focused", "!text,textarea,-submit", "y" ], // match IE, not FF. Aha. This is interesting. So this change may break sites, expecting IE behavior?

Comment on attachment 80632 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=80632&action=review >> Source/WebCore/html/HTMLFormElement.cpp:188 >> + if (formElement->getAttribute(typeAttr).lower() == "submit" || formElement->hasLocalName(buttonTag)) { > > This is horrible. We shouldn't need to reach into attributes at this level. Why did you change this? I didn’t change this — it’s new code. Why did I write it like this? I simply don’t know of a better way. There’s no `isSubmitButton()` method. Should I add one? Any suggestions? >> LayoutTests/fast/forms/implicit-submission.html:-19 >> - [ "Text input and text area and a disabled submit, text input focused", "!text,textarea,-submit", "y" ], // match IE, not FF. > > Aha. This is interesting. So this change may break sites, expecting IE behavior? The spec states that if the only submit button in a form is disabled, it shouldn’t submit implicitly. If there are no submit buttons at all, implicit submission should still work. This patch would make it so. So yes, it would break sites that rely on IE-style behavior for forms that only have disabled submit buttons. (Not that it matters, but that sure sounds like an edge case to me.) For this particular case, it would instead match Firefox and Opera’s behavior, which happens to conform to the HTML spec and also makes much more sense. Would this change be acceptable or not?

Comment on attachment 80632 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=80632&action=review >>> Source/WebCore/html/HTMLFormElement.cpp:188 >>> + if (formElement->getAttribute(typeAttr).lower() == "submit" || formElement->hasLocalName(buttonTag)) { >> >> This is horrible. We shouldn't need to reach into attributes at this level. Why did you change this? > > I didn’t change this — it’s new code. > Why did I write it like this? I simply don’t know of a better way. There’s no `isSubmitButton()` method. Should I add one? Any suggestions? Yes -- you can plumb HTMLInputElement::isSubmitButton() and remove the use of isSuccessfulSubmitButton altogether. Just check for disabled(). Also, you don't need two variables here. Increment when found, decrement if disabled. You only need to make sure that there's at least one non-disabled submit button. >>> LayoutTests/fast/forms/implicit-submission.html:-19 >>> - [ "Text input and text area and a disabled submit, text input focused", "!text,textarea,-submit", "y" ], // match IE, not FF. >> >> Aha. This is interesting. So this change may break sites, expecting IE behavior? > > The spec states that if the only submit button in a form is disabled, it shouldn’t submit implicitly. If there are no submit buttons at all, implicit submission should still work. This patch would make it so. > > So yes, it would break sites that rely on IE-style behavior for forms that only have disabled submit buttons. (Not that it matters, but that sure sounds like an edge case to me.) For this particular case, it would instead match Firefox and Opera’s behavior, which happens to conform to the HTML spec and also makes much more sense. > > Would this change be acceptable or not? I think the change is acceptable, but we should watch for the reactions from the Web and be ready to revert this if it proves drastic. Also, would love to see a test added with multiple submit buttons (some disabled), just to test your newly added code.

Comment on attachment 80632 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=80632&action=review Thanks for your feedback! I’ll tweak the code as per your suggestions and submit another patch later tonight. >>>> LayoutTests/fast/forms/implicit-submission.html:-19 >>>> - [ "Text input and text area and a disabled submit, text input focused", "!text,textarea,-submit", "y" ], // match IE, not FF. >>> >>> Aha. This is interesting. So this change may break sites, expecting IE behavior? >> >> The spec states that if the only submit button in a form is disabled, it shouldn’t submit implicitly. If there are no submit buttons at all, implicit submission should still work. This patch would make it so. >> >> So yes, it would break sites that rely on IE-style behavior for forms that only have disabled submit buttons. (Not that it matters, but that sure sounds like an edge case to me.) For this particular case, it would instead match Firefox and Opera’s behavior, which happens to conform to the HTML spec and also makes much more sense. >> >> Would this change be acceptable or not? > > I think the change is acceptable, but we should watch for the reactions from the Web and be ready to revert this if it proves drastic. > > Also, would love to see a test added with multiple submit buttons (some disabled), just to test your newly added code. I believe there’s already a test for this: “Multiple text inputs and multiple submits, first submit disabled”. Let me know if I’m missing something.

I think that a somewhat deeper look is needed here. The HTML spec is more a reflection of reality than a mandate, and if it doesn't match IE and WebKit (engines with largest market share in desktop and mobile space), maybe it just needs to change. Some questions that I would like to see answers for: - We have a regression test for this behavior. Was it added as part of fixing a real life bug? This test is pretty new, so perhaps Dimitri will remember where this particular subtest came from. - Was this ever discussed on WhatWG or W3C HTML5 mailing lists? Forms are pretty common on web sites, and we've learned that even edge cases matter a lot.

(In reply to comment #11) > I think that a somewhat deeper look is needed here. The HTML spec is more a reflection of reality than a mandate, and if it doesn't match IE and WebKit (engines with largest market share in desktop and mobile space), maybe it just needs to change. Some questions that I would like to see answers for: > - We have a regression test for this behavior. Was it added as part of fixing a real life bug? This test is pretty new, so perhaps Dimitri will remember where this particular subtest came from. I added this test while testing the edge cases of implicit form submission. It wasn't part of the bug per se. > - Was this ever discussed on WhatWG or W3C HTML5 mailing lists? > > Forms are pretty common on web sites, and we've learned that even edge cases matter a lot. I agree with Alexey here. I've learned it the hard way :) Even though "no implicit submission on disabled inputs" makes good logical sense.

Comment on attachment 80632 [details] Revised patch View in context: https://bugs.webkit.org/attachment.cgi?id=80632&action=review >>>> Source/WebCore/html/HTMLFormElement.cpp:188 >>>> + if (formElement->getAttribute(typeAttr).lower() == "submit" || formElement->hasLocalName(buttonTag)) { >>> >>> This is horrible. We shouldn't need to reach into attributes at this level. Why did you change this? >> >> I didn’t change this — it’s new code. >> Why did I write it like this? I simply don’t know of a better way. There’s no `isSubmitButton()` method. Should I add one? Any suggestions? > > Yes -- you can plumb HTMLInputElement::isSubmitButton() and remove the use of isSuccessfulSubmitButton altogether. Just check for disabled(). > > Also, you don't need two variables here. Increment when found, decrement if disabled. You only need to make sure that there's at least one non-disabled submit button. …or none at all, in which case implicit submission should still work. This is why I used two variables. Am I missing something?

Created attachment 80699 [details] Revised patch

Created attachment 80702 [details] Patch

Comment on attachment 80702 [details] Patch I think you forgot to change HTMLInputElement to plumb isSubmitButton, too? Also, before this patch can land, you need to persuade Alexey that it should :)

(In reply to comment #16) > Also, before this patch can land, you need to persuade Alexey that it should :) I think Alexey’s feedback is critically important and I think we must do what he asks before making this change.

Comment on attachment 80702 [details] Patch I didn't meant to r+ this. It was an accident! :)

Dimitri explained that the test case doesn't have long or interesting history. Besides searching html5 discussion archives, a good thing to do is to search bugzilla.mozilla.org for related bugs (including WONTFIX and INVALID). If there are actual sites broken in Firefox due to this, a spec change is the most straightforward answer. If you do the research and nothing comes up, this looks like a fine change to me. + [ "Single text input with submit disabled", "!text,-submit", "n" ], // match spec I would have either said that we're matching HTML5 and Firefox instead of IE, or make no comment at all. Matching relevant specifications doesn't need further explanation unless there are complications!

Created attachment 80799 [details] Patch

Searching for “implicit submission” in Mozilla’s bug tracker doesn’t seem to help, but https://bugzilla.mozilla.org/buglist.cgi?quicksearch=submit+enter returns some very interesting results. Especially: * https://bugzilla.mozilla.org/show_bug.cgi?id=190563 * https://bugzilla.mozilla.org/show_bug.cgi?id=104449 * https://bugzilla.mozilla.org/show_bug.cgi?id=104211

Comment on attachment 80799 [details] Patch I am not sure why the code change in HTMLInputElement.cpp and .h is necessary.

(In reply to comment #22) > (From update of attachment 80799 [details]) > I am not sure why the code change in HTMLInputElement.cpp and .h is necessary. Ok, I see the conflicts now. :)

Comment on attachment 80799 [details] Patch Marking r- based on Ap's commentary. This patch hasn't been touched in over 3 months and likely not longer applies.

Both Chrome and Firefox disable implicit form submission when the default button is disabled, I'd argue that Webkit should follow suit.

(In reply to Daniel Compton from comment #25) > Both Chrome and Firefox disable implicit form submission when the default > button is disabled, I'd argue that Webkit should follow suit. This doesn't seem to trivially reproduce with the linked test case http://krijnhoetmer.nl/stuff/html/form-submit-disabled-button/, and likewise https://wpt.fyi/results/html/semantics/forms/form-submission-0/implicit-submission.optional.html?run_id=5191742251335680&run_id=5172360246722560&run_id=5152321699315712 seems to test this and be passing. Do you have a test case where this still happens?

Pretty sure this was fixed a long time ago.

> This doesn't seem to trivially reproduce with the linked test case http://krijnhoetmer.nl/stuff/html/form-submit-disabled-button/ I can reproduce this in Safari 16.3 but not in Safari 16.4 or Safari STP 167 (16.4). It looks like this was fixed in August 2022 with https://github.com/WebKit/WebKit/commit/7f2b47eeff03586875c526e4e5763c8a52c033f8 via https://bugs.webkit.org/show_bug.cgi?id=243594. I'll make sure to test on the newest Safari next time. Thanks, and sorry for the noise!

*** This bug has been marked as a duplicate of bug 243594 ***