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

xilem_html: Significant refactor and introduce DOM interface traits #141

Merged
merged 10 commits into from
Nov 23, 2023

Conversation

Philipp-M
Copy link
Contributor

This PR refactors xilem_html significantly such that it:

  • Refactors attributes to be managed by a separate view (Attr) that composes e.g. the underlying element view instead of managing all attributes via a Vec directly on the element, this makes the API more "functional/declarative" such that something like set_attr isn't possible anymore, but gives more control over how e.g. el.attr("n", "v").attr("n", "2") should be handled exactly (currently the outermost attribute overwrites inner values (i.e. <el n="2"></el> in this example)), it's also slightly more efficient as benchmarks have shown.

  • Introduces DOM interface traits that roughly map to their equivalently named DOM interface counterparts that inherit from the base interface Element (currently only every interface that inherits from HTMLElement, but it could be easily extended to also include interfaces that inherit from SVGElement).
    There's e.g. a HtmlCanvasElement trait which has the super trait HtmlElement which in turn has Element (the base interface) as super trait.
    Element requires View + ViewMarker + Sealed as super trait bounds, so it should be a little bit more convenient to use (e.g. fn my_el<T>() -> impl Element<T> instead of fn my_el<T>() -> impl View<T> + ViewMarker. Sealed disallows further implementations downstream as safety precaution, since there are a few dynamic type-system "hacks" necessary to avoid long compilation times because of issues like this. In the future it may be feasible to allow implementations downstream.
    Each DOM interface trait has the potential to support only fields that are allowed on each of the element (e.g. width and height on a HtmlCanvasElement). Currently, this is done by using the mentioned Attr view, but could be extended by directly accessing the underlying DOM attributes/methods (and I have already experimented with this here). This needs careful evaluation though (wasm binary size, performance etc.).
    The DOM interface traits have another important function, as they're implemented for all kinds of views (e.g. Attr or event views) recursively until it reaches a concrete element view and all the composing views "take the dom interface type", such that canvas_el.on_click().width().attr() is possible (which was not the case previously).
    So each view (like Attr) can be used as long as the "first/deepest" view in this composition chain is an element view.
    Currently as example width and height is implemented for a HtmlVideoElement and HtmlCanvasElement.
    A follow-up PR will likely add the rest of the attributes that are possible via HTML attributes (or goes the more ambitious and novel way of using DOM attributes (no other major framework I know of goes this route yet AFAIK)).

  • Refactors the element and event views, such that they're more compact and efficient when compiled (e.g. there's no extra name attribute, the view is directly macro-expanded with the relevant string. Same applies to the event views which were also majorly refactored (and with a few bug fixes too). This also replaces the Element View with a CustomElement view, which should support autonomous custom elements ("Web components"). Customized built-in elements are not supported for now, but it shouldn't be too difficult to add support for them.

  • Removes the extra feature flag for typed views, as I don't see a big benefit currently (after benches and usage/experiments). In the future, it maybe makes sense for the extra DOM interfaces, but that needs to be evaluated carefully (to avoid friction for either the user as well as the maintainers of xilem_html).

  • Removes the after_update method for now, but an improved and extended AfterUpdate view is already implemented here (which will result in a PR shortly)

For more (text-wally) information checkout the relevant zulip thread, which may contain interesting information collected along the (longer than planned) course of this PR.

}
}

impl<A: Action> sealed::Sealed for A {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs a separate Sealed trait because of e.g. this

…o avoid inference issues with composed types
* Add ancestor *and* descendent composable macros
* Added a macro to correctly and conveniently implement dom interfaces for interface restricted Views, this is used currently for the Attr view and the event views
@Philipp-M Philipp-M force-pushed the xilem_html-dom-interfaces branch from 50c5214 to 4e1fbce Compare November 7, 2023 20:46
@jplatte
Copy link
Member

jplatte commented Nov 8, 2023

There's no reason Adapt, AdaptState and Memoize can't implement Element, is there? I need at least the AdaptState impl to upgrade to this branch.

edit: The OneOf enums too. Here's my update attempt: jplatte/caniuse.rs@0b5dbbc

@Philipp-M
Copy link
Contributor Author

I have just looked into that. At least Memoize will have issues, as the View::rebuild until the element has to be reached, so this will have unwanted consequences (IMO):

memoize(memo_me, |memo_me| {
    p(memo_me).attr("something", "memo") 
})
.attr("something2", "changes here won't be propagated to the p element, unless memo_me has changed")

This is because the context that is passed in View::rebuild collects the attributes while traversing down the tree and Memoize stops this traversal when the memoized value hasn't changed, thus the traversal doesn't reach the element view, where the actual element updates/diffing is happening.
But with View + ViewMarker instead of Element it can of course still be used (slightly inconvenient though...), so updating without using the interfaces (as return type at least) should still be working, to use .attr() and all the other methods defined on Element for element views that support it, it's just necessary to have the trait in scope (I think this is the only necessity for updating to this branch currently).

Regarding the other Views, I think it should be possible to implement the Element trait for those.
Implementing all the child interface traits should probably also work.
For the OneOf views I think it's necessary to restrict all members to one interface like this:

impl<T, A, V1: Element, V2: Element> Element<T, A> for OneOf2<V1, V2> {}
impl<T, A, V1: HtmlElement, V2: HtmlElement> HtmlElement<T, A> for OneOf2<V1, V2> {}
// probably needs all the bounds that are defined on the View impl as well...

I think Adapt and AdaptState should work as well like this:

// Adapt
impl<ParentT, ParentA, ChildT, ChildA, V, F> Element<ParentT, ParentA>
    for Adapt<ParentT, ParentA, ChildT, ChildA, V, F>
where
    V: Element<ChildT, ChildA>,
    F: Fn(&mut ParentT, AdaptThunk<ChildT, ChildA, V>) -> $crate::MessageResult<ParentA> $( $ss )*,
{
}
// AdaptState
impl<ParentT, ChildT, A, V, F> Element<ParentT, A> for AdaptState<ParentT, ChildT, V, F>
where
    V: Element<ChildT, A>,
    F: Fn(&mut ParentT) -> &mut ChildT,
{
}

I think I'll look into/implement this, when I have more time. But as said, it's of course still possible to use these views, just currently not the extra methods defined on the DOM traits (and as impl return type) on these views.

@jplatte
Copy link
Member

jplatte commented Nov 8, 2023

Yeah, the thing is I need to use .attr on AdaptState and OneOf2, OneOf3 unless I remove some abstractions such that I can apply the attr before wrapping the inner elements in those generic containers, and I don't really want to do that. I think Memoize is not needed and I'm not surprised that it's impossible or would have weird behavior.

* Eliminate associated trait bound for the View, for easier use
* Add additional OneSeqOf view sequences, to avoid removing the ViewMarker super trait bound on Element
* implement all dom interfaces for the OneOf views
@Philipp-M
Copy link
Contributor Author

I have implemented all the DOM traits for Adapt and AdaptState and with a few changes as well for the OneOf views. The OneOfN changes currently reside in it's own branch, as additional OneSeqOfN types were added because the ViewMarker super trait requirement of the Element trait (I didn't want to remove that just for these views, and I also think that OneOf may run into issues when something like this fn v() -> View + ViewMarker { OneOf2::A(v) } is necessary)

I have also (mostly) fixed your caniuse xilem branch here, you may want to check if everything is correct there (specifically src/components/index.rs because it did not compile without initializing a few uninitialized vars, that I haven't checked in detail).

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I've not had a chance to run this yet, but I'm happy to approve anyway, as the Xilem html is experimental

It's not that clear to me why the attribute view doesn't apply the attribute changes itself, instead maintaining the set of current attributes. But I've not dug too deeply into the code

let element: &web_sys::HtmlInputElement = element.dyn_ref().unwrap_throw();
element.set_value(value)
} else if name == "checked" {
let element: &web_sys::HtmlInputElement = element.dyn_ref().unwrap_throw();
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit surprising

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is existing code, that was just copied (from element/mod.rs).
But it's a good point, I have just tried out only using element.set_attribute and it also works for me?

@derekdreery do you have input/context here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, at least for value there indeed seems to be an issue with clearing the value, it just isn't cleared in the input.

I think this is a good motivation to properly support DOM attributes instead of just HTML attributes.

}

fn remove_attribute(element: &web_sys::Element, name: &str) {
// we have to special-case `value` because setting the value using `set_attribute`
Copy link
Member

Choose a reason for hiding this comment

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

Value isn't the key being special cased here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above but with element.remove_attribute

@Philipp-M
Copy link
Contributor Author

Thanks.

The reason why I have decided to collect these attributes instead of setting them directly, is more control over the whole diffing/rebuilding and View::build process (by centralizing them at the concrete element view) and better support for composability of views.
E.g. what should happen when attributes are overwritten with outer views, something like el.attr("class", "1").attr("class", "2") or el.attr("class", "1").attr("class", None) (and especially a switch from Some(..) to None)?
My proposed logic for these cases is defined here.

Collecting also avoids setting the attributes multiple times in the DOM (see examples given above), which should be more efficient (less DOM traffic).

Could also be generally interesting for some kind of Modifier views. I originally wanted to do it with the associated View::State type (ElementState<VS>) which is reused for all kinds of views (e.g. Attr), but this has two disadvantages: long compile times, because of the issue linked in the PR description, and the limitation of being forced to use the same state type (could be done via a trait impl on the View::State type, but code will certainly get more complex this route, and inherently also longer compile times).

I think I will include the changes in Philipp-M@724a5b4, I'm also generally not sure, whether types should have a View impl and a separate ViewSequence impl (that is not covered by the V: View blanket impl). At least with the current architecture, I think we will run into some unwanted issues (e.g. not being able to use impl View + ViewMarker as return type for OneOfN).

I think I'll also reorder the generic attributes of composing views as proposed here.

@Philipp-M Philipp-M linked an issue Nov 22, 2023 that may be closed by this pull request
@Philipp-M
Copy link
Contributor Author

Ok, I'm merging this as it had multiple iterations, and I'm relatively happy with the outcome (little bit more complex than I would've wished, but I guess that's the price to pay for more sophisticated/correct typing with a lot of hierarchical DOM interfaces, avoiding a lot of boilerplate).

@Philipp-M Philipp-M added this pull request to the merge queue Nov 23, 2023
Merged via the queue into linebender:main with commit d40a94f Nov 23, 2023
7 checks passed
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.

xilem_html: called Option::unwrap() on a None value
3 participants