Skip to content

Implement form submissions#188

Merged
nicoburns merged 4 commits intoDioxusLabs:mainfrom
kokoISnoTarget:form_submissions
May 20, 2025
Merged

Implement form submissions#188
nicoburns merged 4 commits intoDioxusLabs:mainfrom
kokoISnoTarget:form_submissions

Conversation

@kokoISnoTarget
Copy link
Copy Markdown
Contributor

@kokoISnoTarget kokoISnoTarget commented Feb 28, 2025

Implements basic form submissions (fixes #178)
I tried following the specification, where it made sense.

Things that are missing are:

@nicoburns
Copy link
Copy Markdown
Member

Just a note that I going to be offline for next 3-4 days. Will review when I'm back!

@nicoburns
Copy link
Copy Markdown
Member

I have rebased this on top of latest main and force pushed

@nicoburns
Copy link
Copy Markdown
Member

Have rebased and force pushed again. Sorry for the delay in review. Will get to this soon!

@nicoburns nicoburns force-pushed the form_submissions branch from 58d4b73 to 6252cfd Compare May 7, 2025 19:12
@nicoburns nicoburns force-pushed the form_submissions branch 2 times, most recently from 638eb10 to aeb27b4 Compare May 20, 2025 12:45
Copy link
Copy Markdown
Member

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

@kokoISnoTarget I've left you some review comments. However, in the interest of 1. getting this merged and 2. avoiding conflicts, I'm going to merge this as-is and iterate on top of it.

Comment on lines +207 to +209
for id in self.form_nodes.borrow().iter() {
doc.reset_form_owner(*id);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

html5ever actually computes the form associations for us (https://docs.rs/html5ever/latest/html5ever/interface/trait.TreeSink.html#method.associate_with_form). Perhaps we should use that?

(not sure though, as we'll need to implement this ourselves for dioxus-native anyway)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't actually see this, but we need to reset all form controls anyways because html5ever does not find associations using the form attr:

<button form="form1">Test</button>
<form id="form1">
</form>


#[derive(Debug, Clone)]
pub enum DocumentResource {
String(String),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the purpose of the String variant? It never seems to be used? Perhaps url and document_resource should just be a blitz_traits::net::Request?

pub fn new_with_root(doc: &'a BaseDocument, root: usize) -> Self {
TreeTraverser {
doc,
stack: vec![root],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Consider pre-allocating more stack space. Perhaps a capacity of 20?

Comment on lines +125 to +132

#[derive(Clone)]
/// An pre-order tree traverser for a [BaseDocument](crate::document::BaseDocument).
pub struct TreeTraverser<'a> {
doc: &'a BaseDocument,
stack: Vec<usize>,
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we move these "traversers" into their own traversal module please?

These also duplicate the method starting with iter_children_mut, iter_subtree_mut, next_node, etc in document.rs. The approach is slightly different, but we'll probably want to deduplicate this at some point. Could you please review those methods and let me know any thoughts you have on how we might unify these APIs.


pub changed: HashSet<usize>,

pub controls_to_form: HashMap<usize, usize>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a doc comment on this field please? I assume the key is the NodeId of the form control and the value is the NodeId of the form itself?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you add a doc comment on this field please?

Sure thing.

I assume the key is the NodeId of the form control and the value is the NodeId of the form itself?

You assumed correctly.

Comment on lines +340 to +356
pub struct NavigationHandler(String);
impl NavigationHandler {
pub fn boxed(url: impl Into<String>) -> Box<Self> {
Box::new(Self(url.into()))
}
}
impl NetHandler<Resource> for NavigationHandler {
fn bytes(self: Box<Self>, doc_id: usize, bytes: Bytes, callback: SharedCallback<Resource>) {
callback.call(
doc_id,
Ok(Resource::Navigation {
url: self.0,
document: bytes,
}),
);
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think blitz-dom should handle the network call when navigating to a new document. Not least because a custom NavigationProvider may want to implement their own loading logic which is different to that implemented in the NetProvider.

The way I think this should work is that blitz-dom just calls into the navigation provider with the "request" to navigate (be that from clicking on a link or submitting a form), and then the navigation provider can do whatever it likes. In practice, the navigation provider will have it's own reference to the (same) network provider which it can use to fetch the document.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea, this should be removed anyways, as it's a leftover from a test implementation.

enum FormMethod {
Get,
Post,
Dialog,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the "dialog" method? Can't find any references to this.

Copy link
Copy Markdown
Contributor Author

@kokoISnoTarget kokoISnoTarget May 20, 2025

Choose a reason for hiding this comment

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

@nicoburns nicoburns merged commit 5f75fe6 into DioxusLabs:main May 20, 2025
9 checks passed
nicoburns pushed a commit that referenced this pull request May 20, 2025
* remove: net navigation handler

* add comment to explain Document controls_to_form

* remove unnecessary DocumentResource

* clippy
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.

Implement form submission

2 participants