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

Forms #1915

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Forms #1915

wants to merge 14 commits into from

Conversation

ForbesLindesay
Copy link
Contributor

Support navigation triggered by forms. At the moment, this is only for when the method is “get”. I think implementing “post” is fairly high priority, but “dialog” is less urgent.

This is a fairly minimal session history implementation.  Next Step is
to add some ability for SessionHistory to report when the `window`
object has changed.
I’ve clearly identified which parts of the spec have not yet been
implemented.
This isn’t necessarily a very accurate implementation of the spec, as
implementing the spec got very confusing.  It is an initial attempt to
get some kind of navigation working, and it also includes a simple test
for the API.
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

notImplemented("url scheme " + scheme, window);
}
// https://html.spec.whatwg.org/#submit-mutate-action
function mutateActionURL() {
Copy link
Member

Choose a reason for hiding this comment

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

Would be ideal to avoid nested functions where possible, moving these to top-level of the module or class and passing in parameters as appropriate

}
}
_getAttributeForSubmission(name, submitter) {
return submitter.hasAttribute('form' + name) ? submitter.getAttribute('form' + name) : this.getAttribute(name);
Copy link
Member

Choose a reason for hiding this comment

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

Somewhat nicer to write as

const formAttribute = submitter.getAttribute("form" + name);
return formAttribute === null ? this.getAttribute(name) : formAttribute;

@@ -21,6 +21,23 @@ exports.evaluateJavaScriptURL = (window, urlRecord) => {
return undefined;
};

// https://html.spec.whatwg.org/#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name
exports.chooseBrowsingContext = (currentWindow, name) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use this in navigation from a elements too?

@@ -7,6 +7,12 @@ const { domSymbolTree } = require("../helpers/internal-constants");
const createHTMLCollection = require("../../living/html-collection").create;
const notImplemented = require("../../browser/not-implemented");
const { reflectURLAttribute } = require("../../utils");
const { constructTheFormDataSet } = require("../xhr/FormData-impl");
Copy link
Member

Choose a reason for hiding this comment

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

Ideally let's move this to ../xhr/helpers/formdata.js or similar (i.e. -impl classes should not generally contain helper utilities)

const form = this;
const document = this._ownerDocument;
const window = document._defaultView;

Copy link
Member

Choose a reason for hiding this comment

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

Check for a browsing context by checking if window is truthy.


if (!flags.submittedFromSubmit) {
const ev = document.createEvent("HTMLEvents");
ev.initEvent("submit", true, true);
Copy link
Member

Choose a reason for hiding this comment

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

The modern way to do this is

const ev = Event.createImpl(["submit", { bubbles: true, cancelable: true }], {});

@jacobbogers
Copy link

Wow this is an oldy, what is the status? any way I can help out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants