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

error: this.$refs.sideNav is undefined #5372

Closed
indirectlylit opened this issue Apr 15, 2019 · 9 comments · Fixed by #8317
Closed

error: this.$refs.sideNav is undefined #5372

indirectlylit opened this issue Apr 15, 2019 · 9 comments · Fixed by #8317
Assignees
Labels
bug Behavior is wrong or broken

Comments

@indirectlylit
Copy link
Contributor

Observed behavior

Observed in Sentry

This event has only occurred one time, so it does not appear to be prevalent.

It appears that some event & timing logic is causing us to try and access a $ref when it doesn't exist.

    // ...
    watch: {
      navShown(isShown) {
        this.$nextTick(() => {
          if (isShown) {
            window.addEventListener('focus', this.containFocus, true);
            this.previouslyFocusedElement = document.activeElement;
            this.$refs.sideNav.focus();
          } else {
            window.removeEventListener('focus', this.containFocus, true);
            this.previouslyFocusedElement.focus();
          }
        });
      },
    },
    methods: {
      // ...
      containFocus(event) {
        if (event.target === window) {
          return event;
        }
        if (!this.$refs.sideNav.contains(event.target)) {
          this.$refs.toggleButton.$el.focus();
        }
        return event;
      },
    },
    // ...

It's possible that this relates to unsafe combined use of $watch, $nextTick, and window.addEventListener.

Expected behavior

The event logic should be defined in such a way that we don't try to access undefined references. If that is not possible, we should at least and a conditional to check for existence before access

User-facing consequences

none known

Errors and logs

TypeError: this.$refs.sideNav is undefined

Steps to reproduce

unknown

Context

0.12.1

@jonboiser
Copy link
Contributor

Similar errors have happened elsewhere in the app, hence placing the kinds of guards mentioned in #5266 over time. For various reasons, these errors happen unpredictably, hence the suggestion in #5266

if (this.$refs.modal && !this.$refs.modal.contains(document.activeElement)) {

@indirectlylit
Copy link
Contributor Author

Could this be a Vue bug? Or are we not respecting the component lifecycle?

My understanding is that as long as we do the following, we shouldn't encounter undefined refs:

  • $nextTick inside mounted to ensure children have also mounted
  • v-show instead of v-if
  • tear down event handlers on or before beforeDestroy

I see what you mean that checking for undefined is a common pattern that could be applied everywhere, but it also makes me feel like maybe we're doing something "wrong"?

@MisRob
Copy link
Member

MisRob commented Apr 16, 2019

Parent component waits for all children to be mounted before it mounts itself so if I can understand that right, $nextTick inside mounted might be unnecessary?

Agree that it would be worth investigating what's happening at this particular place in detail to be sure what's the problem exactly at first (and hopefully this will help us to understand all similar problems if anything like that occurs in the future again) - I cannot understand this behaviour either yet. Will try if I am able to replicate this somehow.

@MisRob
Copy link
Member

MisRob commented Apr 16, 2019

tear down event handlers on or before beforeDestroy

^^^^
Was thinking about what Devon said and I wonder if this could be the case actually :)

window.removeEventListener('focus', this.containFocus, true); is called only when navigation closed but I think that this event listener might survive a while longer than needed when a navigation item clicked. I'm not sure if I can understand how the navigation works in detail yet, but after a brief look, it seems that on nav item click, navigation stays open and then just redirects you to another app/plugin using a regular anchor element so the navigation component is destroyed, is that right? If yes, I guess that this problem might occur more or less randomly based on timing and tabs switch circumstances - although all event listeners should be destroyed after redirect to a new page, I guess that following might happen - focus event is called before the redirect finished but when the side navigation component is being destroyed..?? 🤯 ..Do you think something like that could happen?

@MisRob
Copy link
Member

MisRob commented Apr 16, 2019

Hmm but when I'm thinking about this "destroyed" part, destroy hooks won't be probably even called when redirecting to a new page in this way.

@indirectlylit
Copy link
Contributor Author

Parent component waits for all children to be mounted before it mounts itself so if I can understand that right, $nextTick inside mounted might be unnecessary?

That's what I would have assumed too, but the docs say that mounted does not guarantee that all child components have also been mounted, and recommends using $nextTick

@indirectlylit
Copy link
Contributor Author

on nav item click, navigation stays open and then just redirects you to another app/plugin using a regular anchor element

Yes, most nav items should just be basic <a> links to other pages. There is at least one action that stays inside the SPA - showing the privacy info modal. The two instances @jonboiser showed above are cases where you stay within the SPA, too.

so the navigation component is destroyed, is that right?

I don't think it's destroyed in the Vue component lifecycle sense. My understanding is that on a browser navigation event, JS execution simply "stops" and I wouldn't expect our code to handle any additional events or try to execute anything else.

@MisRob
Copy link
Member

MisRob commented Apr 16, 2019

 I don't think it's destroyed in the Vue component lifecycle sense

Yes, you're right, I realized later that we're not dealing with navigation within one app in this case.

That's what I would have assumed too, but the docs say that mounted does not guarantee that all child components have also been mounted, and recommends using $nextTick

Good to know! I read that all children are mounted before the parent in the past but totally missed that it could happen that it might not finish in time. Thanks for the reference.

@rtibbles rtibbles self-assigned this Aug 17, 2021
@rtibbles rtibbles linked a pull request Aug 18, 2021 that will close this issue
9 tasks
@rtibbles
Copy link
Member

Fixed in #8317

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Behavior is wrong or broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants