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

1.5.1 issue, nested if.bind click.delegate does not fire after parent container has been hidden and re-shown #317

Closed
discoaaron opened this Issue Oct 4, 2017 · 10 comments

Comments

Projects
None yet
6 participants
@discoaaron
Contributor

discoaaron commented Oct 4, 2017

I'm submitting a bug report

  • Library Version:
    1.5.1

Please tell us about your environment:

  • Operating System:
    Windows 10

  • Node Version:
    7.4.0

  • NPM Version:
    4.0.5
  • JSPM OR Webpack AND Version
    webpack 3.3.0
  • Browser:
    Chrome 61.0.3163.100
    Firefox 56.0 (32-bit)

  • Language:
    TypeScript

Current behaviour:
Nested if.bind click delegate does not fire when parent container is hidden and re-shown

Expected/desired behavior:
nested if.bind click delegate should fire when parent container is re-shown

Setup
Below is a clone of the aurelia-skeleton repo with only changes to welcome.ts, welcome.html and package.json. I was unable to use gist due to the need to change the areulia-templating dependancy, sorry!
https://github.com/discoaaron/skeleton-typescript-webpack-issue-example

** Expected Behaviour steps **
npm install
npm start

navigate to http://localhost:8080/
Click test - alert shown
Click Toggle (div is hidden)
Click Toggle (div is shown)
Click test - alert shown (as expected)

Erroneous behaviour steps
Update aurelia-templating to latest
change
"aurelia-templating-resources": "1.4.0"
to
"aurelia-templating-resources": "^1.4.0"

stop watch task (if still running)
npm update
npm start

Click test - alert shown
Click Toggle (div is hidden)
Click Toggle (div is shown)
Click test - alert not shown! (not expected, this should have displayed like normal)

I've done some debugging and this is what I have so far

  • the click.delegate doesn't get called
  • debugging the event-manager.js, handleDelegatedEvent function shows that target.delegatedCallbacks is null/ empty when it shouldn't be.
  • I attempted to debug if.js and the newly created if-core.js but this was about the point where I'm unsure what I'm looking at.
@jsobell

This comment has been minimized.

Show comment
Hide comment
@jsobell

jsobell Oct 4, 2017

This may be a bug, but remember that if.bind doesn't hide an element, it removes it from the DOM.
If you want to hide and show it, use show.bind. I think you'll find you find your code then works.

jsobell commented Oct 4, 2017

This may be a bug, but remember that if.bind doesn't hide an element, it removes it from the DOM.
If you want to hide and show it, use show.bind. I think you'll find you find your code then works.

@discoaaron

This comment has been minimized.

Show comment
Hide comment
@discoaaron

discoaaron Oct 5, 2017

Contributor

Yeah, thanks!

For some cases in our application switching things to show.bind works fine without any other changes. However this issue is present in many places in our application and switching some instances to show.bind required other changes to the view models, which is why we decided to report it as a bug.

If no fix is found (or if this is not really a bug!) we'll just have to switch everything over and make the necessary code changes (since we don't reeeally want to be behind on core aurelia versions). But fingers crossed you guys can figure it out!

If there's anything our team can do to help, let me know!

Contributor

discoaaron commented Oct 5, 2017

Yeah, thanks!

For some cases in our application switching things to show.bind works fine without any other changes. However this issue is present in many places in our application and switching some instances to show.bind required other changes to the view models, which is why we decided to report it as a bug.

If no fix is found (or if this is not really a bug!) we'll just have to switch everything over and make the necessary code changes (since we don't reeeally want to be behind on core aurelia versions). But fingers crossed you guys can figure it out!

If there's anything our team can do to help, let me know!

@zenorbi

This comment has been minimized.

Show comment
Hide comment
@zenorbi

zenorbi Oct 5, 2017

Looks like a duplicate of #315

zenorbi commented Oct 5, 2017

Looks like a duplicate of #315

@discoaaron

This comment has been minimized.

Show comment
Hide comment
@discoaaron

discoaaron Oct 5, 2017

Contributor

Ahh yes! I think so!

Contributor

discoaaron commented Oct 5, 2017

Ahh yes! I think so!

@jamiepollard

This comment has been minimized.

Show comment
Hide comment
@jamiepollard

jamiepollard Oct 6, 2017

Looks like when the ViewFactory isn't caching then unbind isn't setting the nested items showing state to false correctly anymore. Changing the following seems to fix (although the logic can be re-written to clean it up). This means that when _show is called again to re-render the view, this.showing will be false again and the bindings will be set up correctly.

   IfCore.prototype.unbind = function unbind() {
    if (this.view === null) {
      return;
    }

    this.view.unbind();

    if (!this.viewFactory.isCaching) {
      this.showing = false; // adding this in
      return;
    }

    if (this.showing) {
      this.showing = false;
      this.viewSlot.remove(this.view, true, true);
    } else {
      this.view.returnToCache();
    }

    this.view = null;
  };

Could do with someone else independently verifying this corrects the issue in their environment? Maybe @jods4 can verify as there was a big re-write at this commit (8b0131a)

jamiepollard commented Oct 6, 2017

Looks like when the ViewFactory isn't caching then unbind isn't setting the nested items showing state to false correctly anymore. Changing the following seems to fix (although the logic can be re-written to clean it up). This means that when _show is called again to re-render the view, this.showing will be false again and the bindings will be set up correctly.

   IfCore.prototype.unbind = function unbind() {
    if (this.view === null) {
      return;
    }

    this.view.unbind();

    if (!this.viewFactory.isCaching) {
      this.showing = false; // adding this in
      return;
    }

    if (this.showing) {
      this.showing = false;
      this.viewSlot.remove(this.view, true, true);
    } else {
      this.view.returnToCache();
    }

    this.view = null;
  };

Could do with someone else independently verifying this corrects the issue in their environment? Maybe @jods4 can verify as there was a big re-write at this commit (8b0131a)

@zenorbi

This comment has been minimized.

Show comment
Hide comment
@zenorbi

zenorbi Oct 7, 2017

@jamiepollard This fixes the problem in our internal project. I hope this or another patch lands soon. Thank you for providing a quick fix.

zenorbi commented Oct 7, 2017

@jamiepollard This fixes the problem in our internal project. I hope this or another patch lands soon. Thank you for providing a quick fix.

@powerbuoy

This comment has been minimized.

Show comment
Hide comment
@powerbuoy

powerbuoy Oct 11, 2017

I've just updated to the latest version of Aurelia (including all packages). Pretty much every if.bind I had in my app is now broken. How do you apply the IfCore fix? Just adding the code to app.js gives me errors (IfCore is not defined - pretty obvious I guess).

powerbuoy commented Oct 11, 2017

I've just updated to the latest version of Aurelia (including all packages). Pretty much every if.bind I had in my app is now broken. How do you apply the IfCore fix? Just adding the code to app.js gives me errors (IfCore is not defined - pretty obvious I guess).

@gavinaiken

This comment has been minimized.

Show comment
Hide comment
@gavinaiken

gavinaiken Oct 18, 2017

Do you know when the fix for this is going to be released? This is a pretty serious bug that completely breaks any aurelia app using if.bind. Surprised it has been broken so long, anyone new coming to aurelia and getting the latest version might not know to go and get the 1.4 version of the library which works fine. I know it's not always easy to jump on things immediately but in the past aurelia regressions in new releases were always fixed within a day or two, it's a little concerning that recently things seem to have slowed down a lot. Apologies if this isn't an appropriate place to raise that concern...

gavinaiken commented Oct 18, 2017

Do you know when the fix for this is going to be released? This is a pretty serious bug that completely breaks any aurelia app using if.bind. Surprised it has been broken so long, anyone new coming to aurelia and getting the latest version might not know to go and get the 1.4 version of the library which works fine. I know it's not always easy to jump on things immediately but in the past aurelia regressions in new releases were always fixed within a day or two, it's a little concerning that recently things seem to have slowed down a lot. Apologies if this isn't an appropriate place to raise that concern...

@discoaaron

This comment has been minimized.

Show comment
Hide comment
@discoaaron

discoaaron Oct 18, 2017

Contributor

Hey, please see the above PR I submitted last week #324
The change itself has been reviewed approved, another contributor helped me to add tests today which are pending review, following this it will hopefully be merged and released shortly thereafter.

Contributor

discoaaron commented Oct 18, 2017

Hey, please see the above PR I submitted last week #324
The change itself has been reviewed approved, another contributor helped me to add tests today which are pending review, following this it will hopefully be merged and released shortly thereafter.

@gavinaiken

This comment has been minimized.

Show comment
Hide comment
@gavinaiken

gavinaiken Oct 18, 2017

Sorry, I should have checked the PR, I see there's been lots of activity on there. Was just a bit concerned that the PR was referenced 7 days ago above but it was still waiting for a release.

gavinaiken commented Oct 18, 2017

Sorry, I should have checked the PR, I see there's been lots of activity on there. Was just a bit concerned that the PR was referenced 7 days ago above but it was still waiting for a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment