Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2261393002:
Fixed the behaviour of form submit to Firefox: form does not submit once it has been removed. (Closed)
|
Created:
4 years, 4 months ago by lunalu1 Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed the behaviour of form submit to match the standard: no submit is taken place when the form is detached from the document (e.g., form is removed).
One additional layout test (form-removed-from-document.html) is added to verify the change made above.
Two layout tests has been modified to match the behaviour of submitting form (when from is detached from/attached to the document) in Firefox.
BUG=586749
Committed: https://crrev.com/aaad992eef8e8c1e879f72afd9dabb05d3855e67
Cr-Commit-Position: refs/heads/master@{#414590}
Patch Set 1 #Patch Set 2 : Reformat #Patch Set 3 : Test update #Patch Set 4 : Remove form stats #
Total comments: 6
Patch Set 5 : Layout test modification #Patch Set 6 : Code review change #
Total comments: 2
Patch Set 7 : Code review change #
Total comments: 8
Patch Set 8 : Code review change #
Total comments: 2
Patch Set 9 : "Final nit update" #
Messages
Total messages: 43 (19 generated)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Description was changed from ========== Fixed the behaviour of form submit to Firefox: form does not submit once it has been removed. BUG=586749 ========== to ========== Fixed the behaviour of form submit to match Firefox: no submit is taken place when the form is detached from the document (e.g., form is removed). One additional layout test (form-removed-from-document.html) is added to verify the change made above. Two layout tests has been modified to match the behaviour of submitting form (when from is detached from/attached to the document) in Firefox. BUG=586749 ==========
lunalu@chromium.org changed reviewers: + iclelland@chromium.org
This looks pretty good -- I thought of one more layout test that we should include: Can you write one that creates a form in JavaScript and then tries to submit it, when it has never been attached to the document? (This will make it clear that the behaviour in the tests that you changed isn't supported) https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-cross-site-post.html (right): https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-cross-site-post.html:5: <body></body> Is this required? I think that the body element will exist even if the tags aren't present in the source HTML file. https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-same-site-post.html (right): https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-same-site-post.html:5: <body></body> (Same comment as previous file) https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFormElement.cpp:355: // This matches the behaviour in fireFox. Nit: "Firefox" Also, you could put a reference to the bug here in case someone wants to see context for the change.
On 2016/08/23 17:46:23, iclelland wrote: > This looks pretty good -- I thought of one more layout test that we should > include: Can you write one that creates a form in JavaScript and then tries to > submit it, when it has never been attached to the document? > > (This will make it clear that the behaviour in the tests that you changed isn't > supported) > > https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-cross-site-post.html > (right): > > https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-cross-site-post.html:5: > <body></body> > Is this required? I think that the body element will exist even if the tags > aren't present in the source HTML file. > > https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-same-site-post.html > (right): > > https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-same-site-post.html:5: > <body></body> > (Same comment as previous file) > > https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right): > > https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLFormElement.cpp:355: // This matches the > behaviour in fireFox. > Nit: "Firefox" > > Also, you could put a reference to the bug here in case someone wants to see > context for the change. Changes have made to the review comments. Please review again. Thanks
lunalu@chromium.org changed reviewers: + tkent@chromium.org
Hi, I made some changes to fix the issue crbug.com/586749. Could you please help me review the changes? Thanks https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-cross-site-post.html (right): https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-cross-site-post.html:5: <body></body> On 2016/08/23 17:46:22, iclelland wrote: > Is this required? I think that the body element will exist even if the tags > aren't present in the source HTML file. Yes. Otherwise I will get the error "Cannot read property 'appendChild' of null" when running the test. https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-same-site-post.html (right): https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-same-site-post.html:5: <body></body> On 2016/08/23 17:46:22, iclelland wrote: > (Same comment as previous file) Done. https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/2261393002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFormElement.cpp:355: // This matches the behaviour in fireFox. On 2016/08/23 17:46:23, iclelland wrote: > Nit: "Firefox" > > Also, you could put a reference to the bug here in case someone wants to see > context for the change. Done.
Thanks, this looks good -- LGTM, just one small nit in the new test. https://codereview.chromium.org/2261393002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html (right): https://codereview.chromium.org/2261393002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html:6: </body> Could you put this line below the script (to move the script inside the body?)
On 2016/08/24 15:41:57, iclelland wrote: > Thanks, this looks good -- LGTM, just one small nit in the new test. > > https://codereview.chromium.org/2261393002/diff/100001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html > (right): > > https://codereview.chromium.org/2261393002/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html:6: > </body> > Could you put this line below the script (to move the script inside the body?) Done. Thanks.
https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html (right): https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html:19: function redirect() { assert_true(false); } Use assert_unreached('description'). https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (left): https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFormElement.cpp:355: UseCounter::count(document(), UseCounter::FormSubmissionNotInDocumentTree); Need to remove FormSubmissionNotInDocumentTree from core/frame/UseCounter.h. https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFormElement.cpp:354: // Do not submit if the form is not attached to the document. I don't think this comment and the CL description should refer to Firefox particularly because the new behavior matches to the standard and Edge too. I recommend to add a comment to the beginning of this function like: // https://html.spec.whatwg.org/multipage/forms.html#form-submission-algorithm // 2. If form document is not connected, has no associated browsing context, or // its active sandboxing flag set has its sandboxed forms browsing context flag // set, then abort these steps without doing anything. https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFormElement.cpp:356: // See crbug.com/586749. Remove this line. The bug won't be so helpful after this CL.
https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFormElement.cpp:354: // Do not submit if the form is not attached to the document. On 2016/08/25 00:52:54, tkent wrote: > I don't think this comment and the CL description should refer to Firefox > particularly because the new behavior matches to the standard and Edge too. > > I recommend to add a comment to the beginning of this function like: > > // https://html.spec.whatwg.org/multipage/forms.html#form-submission-algorithm > // 2. If form document is not connected, has no associated browsing context, or > // its active sandboxing flag set has its sandboxed forms browsing context flag > // set, then abort these steps without doing anything. Agreed -- that's better. (I looked for that kind of text at w3.org, but https://www.w3.org/TR/html5/forms.html#form-submission-algorithm doesn't include it) Safari also appears to implement this behaviour.
lunalu@chromium.org changed reviewers: + asvitkine@chromium.org
Hi asvikine, I made some changes in the histograms.xml file. Please review the changes. Thanks, lunalu https://codereview.chromium.org/2261393002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html (right): https://codereview.chromium.org/2261393002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html:6: </body> On 2016/08/24 15:41:57, iclelland wrote: > Could you put this line below the script (to move the script inside the body?) Done. https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html (right): https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/submit-form-not-attached-to-document.html:19: function redirect() { assert_true(false); } On 2016/08/25 00:52:54, tkent wrote: > Use assert_unreached('description'). Done. https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFormElement.cpp (right): https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFormElement.cpp:354: // Do not submit if the form is not attached to the document. On 2016/08/25 00:52:54, tkent wrote: > I don't think this comment and the CL description should refer to Firefox > particularly because the new behavior matches to the standard and Edge too. > > I recommend to add a comment to the beginning of this function like: > > // https://html.spec.whatwg.org/multipage/forms.html#form-submission-algorithm > // 2. If form document is not connected, has no associated browsing context, or > // its active sandboxing flag set has its sandboxed forms browsing context flag > // set, then abort these steps without doing anything. Done. https://codereview.chromium.org/2261393002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFormElement.cpp:356: // See crbug.com/586749. On 2016/08/25 00:52:54, tkent wrote: > Remove this line. The bug won't be so helpful after this CL. > Done.
lgtm
Description was changed from ========== Fixed the behaviour of form submit to match Firefox: no submit is taken place when the form is detached from the document (e.g., form is removed). One additional layout test (form-removed-from-document.html) is added to verify the change made above. Two layout tests has been modified to match the behaviour of submitting form (when from is detached from/attached to the document) in Firefox. BUG=586749 ========== to ========== Fixed the behaviour of form submit to match the standard: no submit is taken place when the form is detached from the document (e.g., form is removed). One additional layout test (form-removed-from-document.html) is added to verify the change made above. Two layout tests has been modified to match the behaviour of submitting form (when from is detached from/attached to the document) in Firefox. BUG=586749 ==========
lunalu@chromium.org changed reviewers: + jwd@chromium.org - asvitkine@chromium.org
Hi I didn't realize that asvitkine was OOO. jwd, could you please review the change I made to the histograms.xml file? Thanks
LGTM with nit. https://codereview.chromium.org/2261393002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2261393002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79082: - <int value="1399" label="FormSubmissionNotInDocumentTree"/> Please prefix the label with OBSOLETE_ instead of removing it.
https://codereview.chromium.org/2261393002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2261393002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79082: - <int value="1399" label="FormSubmissionNotInDocumentTree"/> On 2016/08/25 17:53:31, jwd wrote: > Please prefix the label with OBSOLETE_ instead of removing it. Done.
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iclelland@chromium.org, tkent@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2261393002/#ps160001 (title: ""Final nit update"")
The CQ bit was unchecked by lunalu@chromium.org
The CQ bit was checked by lunalu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by tkent@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Fixed the behaviour of form submit to match the standard: no submit is taken place when the form is detached from the document (e.g., form is removed). One additional layout test (form-removed-from-document.html) is added to verify the change made above. Two layout tests has been modified to match the behaviour of submitting form (when from is detached from/attached to the document) in Firefox. BUG=586749 ========== to ========== Fixed the behaviour of form submit to match the standard: no submit is taken place when the form is detached from the document (e.g., form is removed). One additional layout test (form-removed-from-document.html) is added to verify the change made above. Two layout tests has been modified to match the behaviour of submitting form (when from is detached from/attached to the document) in Firefox. BUG=586749 Committed: https://crrev.com/aaad992eef8e8c1e879f72afd9dabb05d3855e67 Cr-Commit-Position: refs/heads/master@{#414590} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/aaad992eef8e8c1e879f72afd9dabb05d3855e67 Cr-Commit-Position: refs/heads/master@{#414590}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
FYI, I'm attempting to manually revert this in https://codereview.chromium.org/2409863002
Message was sent while issue was closed.
On 2016/10/10 23:31:58, Lei Zhang wrote: > FYI, I'm attempting to manually revert this in > https://codereview.chromium.org/2409863002 And revert it on M54 as well: https://codereview.chromium.org/2405973003
Message was sent while issue was closed.
ojan@chromium.org changed reviewers: + ojan@chromium.org
Message was sent while issue was closed.
Did this have an intent to ship or intent to remove? I can't seem to find it.
Message was sent while issue was closed.
On 2016/10/10 23:46:02, ojan wrote: > Did this have an intent to ship or intent to remove? I can't seem to find it. From the original bug report, it seemed like a small enough corner case, with negligible use, that an intent wasn't required (https://bugs.chromium.org/p/chromium/issues/detail?id=586749#c10). The fix was to match the spec, and to match Firefox behaviour in the same circumstance, although it seems now like the rules that Gecko uses might be more subtle than originally believed.
Message was sent while issue was closed.
Ah, I missed that. Thanks. I was just looking for more explanation on the change. On Mon, Oct 10, 2016 at 8:09 PM <iclelland@chromium.org> wrote: > On 2016/10/10 23:46:02, ojan wrote: > > Did this have an intent to ship or intent to remove? I can't seem to > find it. > > From the original bug report, it seemed like a small enough corner case, > with > negligible use, that an intent wasn't required > (https://bugs.chromium.org/p/chromium/issues/detail?id=586749#c10). The > fix was > to match the spec, and to match Firefox behaviour in the same circumstance, > although it seems now like the rules that Gecko uses might be more subtle > than > originally believed. > > https://codereview.chromium.org/2261393002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Ah, I missed that. Thanks. I was just looking for more explanation on the change. On Mon, Oct 10, 2016 at 8:09 PM <iclelland@chromium.org> wrote: > On 2016/10/10 23:46:02, ojan wrote: > > Did this have an intent to ship or intent to remove? I can't seem to > find it. > > From the original bug report, it seemed like a small enough corner case, > with > negligible use, that an intent wasn't required > (https://bugs.chromium.org/p/chromium/issues/detail?id=586749#c10). The > fix was > to match the spec, and to match Firefox behaviour in the same circumstance, > although it seems now like the rules that Gecko uses might be more subtle > than > originally believed. > > https://codereview.chromium.org/2261393002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
