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

Is the order of rendering macro "items" consistent? #193

Closed
rebo opened this issue Aug 9, 2019 · 9 comments · Fixed by #254
Labels
bug

Comments

@rebo
Copy link

@rebo rebo commented Aug 9, 2019

I have a "toggleable" li![]. When clicked it renders a "highlighted" component, when clicked again it renders the normal component.

However when clicking to go back to normal the items are rendered in opposite order.

I.e. the below initially renders a_string before the span!.

li![
    class![C.my_1, C.py_1, C.px_2, C.rounded],
    a_string , span![class![C.mx_4, C.italic, C.font_bold],"foo"],
    simple_ev(Ev::Click, Msg::ImprovementListing(Include(position)))
]

on re-rendering after clicking again renders the span! before a_string.

Funnily enough if I put a_string in its own span![] then everything seems to work okay.

Not sure if this is a bug or something I am not fully understanding.

By the way, awesome, awesome framework!

@David-OConnor David-OConnor added the bug label Aug 10, 2019
@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Aug 10, 2019

This isn't intended behavior; rendering bug.

@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Aug 10, 2019

Can you post enough code to reproduce this standalone? Was unable to by modifying the quickstart's view fn to this:

fn view(model: &Model) -> impl View<Msg> {
    vec![
    if model.val < 3 || model.val > 5 {
        li![
            class!["A", "B", "C"],
            "should render before" , span![class!["D", "E"],"should render after"],
            // simple_ev(Ev::Click, Msg::ImprovementListing(Include(position)))
            simple_ev(Ev::Click, Msg::Increment)
        ]
    } else { empty![] },
    button![
        simple_ev(Ev::Click, Msg::Increment),
    ]
    ]
}
@rebo

This comment has been minimized.

Copy link
Author

@rebo rebo commented Aug 10, 2019

No problem, its embedded in a large app at the moment so ill chuck stuff out and get a minimal example working shortly.

@jayson-lennon

This comment has been minimized.

Copy link

@jayson-lennon jayson-lennon commented Sep 29, 2019

I'm having this issue as well and was able to eventually track it down to the use of text nodes that are within the same type of containing element. Here's a fully working example to repro:

#[macro_use]
extern crate seed;
use seed::prelude::*;

#[derive(Clone)]
enum Page {
    Home,
    Foo,
}

struct Model {
    page: Page,
}

impl Default for Model {
    fn default() -> Self {
        Self { page: Page::Home }
    }
}

#[derive(Clone)]
enum Msg {
    ShowPage(Page),
}

fn update(msg: Msg, model: &mut Model, _: &mut impl Orders<Msg>) {
    match msg {
        Msg::ShowPage(page) => model.page = page,
    }
}

// The first render of all views are correct.

// On the second and all subsequent renders, "text node" will be placed at a
// position corresponding to the number of child nodes that are present in the
// previous view. In this example, view_home contains 4 child nodes, so it will
// be placed as the fourth element when view_foo is re-rendered.
// This causes subsequent renders to be rendered as such:
//   div
//     div 1
//     div 2
//     div 3
//     text node
//     div 4
fn view_foo() -> Node<Msg> {
    p![
        div!["foo: div 1"],
        "text node",
        div!["foo: div 2"],
        div!["foo: div 3"],
        div!["foo: div 4"],
    ]
}

// When view_home is rendered following view_foo, the element that corresponds
// to the position of "text node" in the initial rendering of view_foo will be
// placed as the last sibling element.
// This causes subsequent renders to be ordered as:
//    div
//      a
//      div 2
//      div 3
//      div 1
fn view_home() -> Node<Msg> {
    p![
        a![attrs! { At::Href => "/foo" }, "foo"],
        div!["home: div 1"],
        div!["home: div 2"],
        div!["home: div 3"],
    ]
}

fn view(model: &Model) -> impl View<Msg> {
    match model.page {
        Page::Home => view_home(),
        Page::Foo => view_foo(),
    }
}

fn routes(url: seed::Url) -> Option<Msg> {
    Some(match url.path.get(0).map(String::as_str) {
        Some("foo") => Msg::ShowPage(Page::Foo),
        _ => Msg::ShowPage(Page::Home),
    })
}

#[wasm_bindgen(start)]
pub fn render() {
    seed::App::build(|_, _| Model::default(), update, view)
        .routes(routes)
        .finish()
        .run();
}

It appears that text nodes are being placed at the end of all siblings, and they are being positioned based on the number of siblings in the previous render.

However, this only occurs when the containing element is of the same type (a div in this case). If the parent element is changed to something like a p in only one of the view functions in the example, then the issue disappears. If no text nodes are used at all, then rendering is consistent.

The errant behavior can be triggered with or without the use of the browser history buttons.

Thanks for all the work on this framework. It composes really well and feels very Rusty :)

@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Oct 5, 2019

Thank you for the detailed breakdown and diagnosis! I apologize for taking so long to address this. I've modified your example to allow a button to re-render the foo page:

#[macro_use]
extern crate seed;
use seed::prelude::*;

#[derive(Clone)]
enum Page {
    Home,
    Foo,
}

struct Model {
    page: Page,
    val: u32,
}

impl Default for Model {
    fn default() -> Self {
        Self { page: Page::Home, val: 0 }
    }
}

#[derive(Clone)]
enum Msg {
    ShowPage(Page),
    Increment,
}

fn update(msg: Msg, model: &mut Model, _: &mut impl Orders<Msg>) {
    match msg {
        Msg::ShowPage(page) => model.page = page,
        Msg::Increment => model.val += 1,
    }
}

// The first render of all views are correct.

// On the second and all subsequent renders, "text node" will be placed at a
// position corresponding to the number of child nodes that are present in the
// previous view. In this example, view_home contains 4 child nodes, so it will
// be placed as the fourth element when view_foo is re-rendered.
// This causes subsequent renders to be rendered as such:
//   div
//     div 1
//     div 2
//     div 3
//     text node
//     div 4
fn view_foo(val: u32) -> Node<Msg> {
    p![
        div!["clickme", simple_ev(Ev::Click, Msg::Increment)],
        div!["foo: div 1"],
        "text node",
        div!["foo: div 2"],
        div!["foo: div 3"],
        div![val.to_string()],
    ]
}

// When view_home is rendered following view_foo, the element that corresponds
// to the position of "text node" in the initial rendering of view_foo will be
// placed as the last sibling element.
// This causes subsequent renders to be ordered as:
//    div
//      a
//      div 2
//      div 3
//      div 1
fn view_home() -> Node<Msg> {
    p![
        a![attrs! { At::Href => "/foo" }, "foo"],
        div!["home: div 1"],
        div!["home: div 2"],
        div!["home: div 3"],
    ]
}

fn view(model: &Model) -> impl View<Msg> {
    match model.page {
        Page::Home => view_home(),
        Page::Foo => view_foo(model.val),
    }
}

fn routes(url: seed::Url) -> Option<Msg> {
    Some(match url.path.get(0).map(String::as_str) {
        Some("foo") => Msg::ShowPage(Page::Foo),
        _ => Msg::ShowPage(Page::Home),
    })
}

#[wasm_bindgen(start)]
pub fn render() {
    seed::App::build(|_, _| Init::new(Model::default()), update, view)
        .routes(routes)
        .finish()
        .run();
}

Clicking the button should trigger the bug, right? I'm unable to get the elements to render out of order. What should I do to trigger the re-render that breaks the order? Thank you.

@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Oct 5, 2019

Does @AlterionX 's fix work for you? Merged, but not released.

@jayson-lennon

This comment has been minimized.

Copy link

@jayson-lennon jayson-lennon commented Oct 16, 2019

I tried the fix by @AlterionX and the behavior remains. I also noticed that I forgot to mention the steps needed to reproduce, apologies about that!

The example you provided does not trigger the bug. I have only been able to trigger the bug with a manual route change or by using the browser history buttons. Steps to reproduce:

  1. Load initial route (/)
  2. Click on the 'foo' link
  3. Click the browser back button. Elements on / are now out of order.
  4. Click the browser forward button. Elements on /foo are now out of order.

I am also able to get this to occur without the browser buttons by modifying line 55 of your edit of the sample code:
div!["clickme", simple_ev(Ev::Click, Msg::ShowPage(Page::Home))],
instead of
div!["clickme", simple_ev(Ev::Click, Msg::Increment)],

The buttons can then be used to toggle between routes and exhibit the bug.
I tested under Chrome and Firefox and am using the master branch of Seed.

Thanks again for all the hard work!

@MartinKavik

This comment has been minimized.

Copy link
Member

@MartinKavik MartinKavik commented Oct 20, 2019

I had similar problems with markdown (raw!) today. Could you test #254 with your projects / examples, please?

@jayson-lennon

This comment has been minimized.

Copy link

@jayson-lennon jayson-lennon commented Oct 20, 2019

Tested #254 with my project and rendering is now behaving as expected. Looks like this is fixed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.