Site Selector: fix bug with highlighted site refs

[lamosty]

Today, I noticed a bug with SiteSelector: if you have two sites which end in numbers (like lamostytransfer1280.blog and lamostytransfer1281.blog) and you type in 80 or 81 into site selector, then select the site and then open site selector and remove the numbers, it will break Calypso: http://cld.wthms.co/bHCnSh.

@rachelmcr reported that: "I get the error when I clear any search that’s 4 characters or less (regardless of whether it’s letters or numbers)" and @bperson "I get a similar React error: “Unable to find node on an unmounted component.” if I clear the text from the switch site filter input ( by clicking on the right cross )".

It's not 100% deterministic and sometimes doesn't happen but I could reproduce it pretty reliably.

The error is about using ReactDom.findDOMNode on an already unmounted React component. When using keyboard or mouse navigation to scroll through sites in SiteSelector, we were assigning the this.refs.highlightedSite with the currently highlighted site. It's necessary to have a ref to this component because when we navigate the list with keyboard (arrow down or up), we want the items to move (with scrollIntoView). This was introduced in #5808 by @marekhrabe.

We could fix it by wrapping ReactDom.findDOMNode in a try...catch. However, I think we should not use findDOMNode on unmounted components and also using string refs through this.refs is deprecated. Therefore, I decided to fix it by adding a fn setHighlightedSiteRef which will assign the currently selected site component into the this.highlightedSiteRef class property. If the currently selected component is being unmounted , we assign null so we prevent the error.

Testing

Play with the SiteSelector: try to repro the original bug, try using keyboard navigation (it should move the list when navigating to a site that's not currently visible).

[matticbot]

Test live: https://calypso.live/?branch=site-selector/fix-highlighted-site-refs

[rachelmcr]

This looks good based on my manual testing. Keyboard navigation works as expected and I can no longer reproduce the issue using the same steps I tried before.

[gwwar]

Have we tried calling this.isHighlighted() a second time so we can avoid generating functions in render?

[lamosty]

Yeah, I spent some time thinking about how to avoid generating functions in render. However, as you can see, this.isHighlighted accepts siteId. So, whether a site component is highlighted or not depends on which siteId it is. We would have to create the same number of functions as there are sites in order to not have to supply siteId.

Or am I missing something?

[gwwar]

Ah ok there are two usages, so we'd need one function for all sites, and another for site. I believe site should be available as a prop? Still might be worth giving it a try.

[lamosty]

Well, site is not available as a prop because this component (SiteSelector) renders all the sites. A site is supplied to the renderSite function from renderSites.

[gwwar]

ah got it, the rest of the code looked fine btw, so if manual tests are good, feel free to 🚢