Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: restore POST forms that open a new window with target=_blank #21469

Merged
merged 2 commits into from
Dec 11, 2019

Conversation

loc
Copy link
Contributor

@loc loc commented Dec 10, 2019

Description of Change

Fix a regression introduced in #20719 that caused HTML form submissions to not actually POST data when using target=_blank. The original implementation in that PR was in shell/common/native_mate_converters/network_converter.cc. Not sure if the removal of those fields was intentional or an oversight. @zcbenz to advise.

Checklist

Release Notes

Notes: Fixed POST-ing HTML forms with target=_blank.

Restore some of the original conversion logic in order to fix target=_blank post form submissions.
@loc loc requested review from nornagon and zcbenz December 10, 2019 23:14
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 10, 2019
@@ -0,0 +1,8 @@
<html>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please put this fixture in spec-main

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure of the convention, since all the other fixtures used in chromium-spec.ts (via fixturesPath) point to spec/. Should I change this one or all of them in chromium-spec.ts? I guess I don't see the value in separating them if there's a chance they'll be used in both.

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had several converters for the ResourceRequestBody and it seems that I have kept the outdated one when refactoring.

Thanks for fixing this and adding tests!

@zcbenz zcbenz merged commit 5cecc23 into master Dec 11, 2019
@release-clerk
Copy link

release-clerk bot commented Dec 11, 2019

Release Notes Persisted

Fixed POST-ing HTML forms with target=_blank.

@zcbenz zcbenz deleted the loc/fix-post-target-blank branch December 11, 2019 06:46
@trop
Copy link
Contributor

trop bot commented Dec 11, 2019

I have automatically backported this PR to "8-x-y", please check out #21474

@sofianguy sofianguy added this to Fixed in 8.0.0-beta.5 in 8.2.x Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-pr 🌱 PR opened in the last 24 hours
Projects
No open projects
8.2.x
Fixed in 8.0.0-beta.5
Development

Successfully merging this pull request may close these issues.

None yet

3 participants