search

google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 291 forks source link

Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node. #2280

Closed jamesozzie closed 3 years ago

jamesozzie commented 4 years ago

Bug Description

There have been reports of the following error from different users. Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

It's unknown at what stage this occurs, possible during a PageSpeed Insights test based on the title of the most recent topic. Awaiting additional information for further troubleshooting.

Support Topics (in order or occurrence - newest on top):

Steps to reproduce

Assuming your browser's preferred language and site locale are set to English:

  1. Configure Chrome language settings (chrome://settings/languages) to add another language
    image
  2. For the added language, check the box to "Offer to translate pages in this language"
  3. Set up Site Kit and enable PageSpeed insights as normal
    • Wait for the PageSpeed report to finish loading
  4. Select the "In the Lab" tab in the "Page Speed and Experience" widget on the dashboard
  5. Right-click on the page and select "Translate to {your chosen language}"
  6. Once translated, toggle the device switch on the PageSpeed Insights report (mobile <-> desktop)
  7. This should immediately trigger a fatal client side error and you should now see the error boundary component with the error

This is only one instance which is easily reproducible and was reported by users in the support forums, but others are possible as well.

Additional Context

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Note: 1st party translation via i18n functions is only possible in WP 5.0+ and when translation files have been downloaded. Also, Google APIs may not support all the possible locales that WP does which may lead to some users needing to use external translation services, such as Google Translate.

Implementation Brief

This bug can be solved in slightly different ways depending on where it occurs:

  1. Edit assets/js/modules/pagespeed-insights/components/common/ReportMetric.js

    • Update each conditional render of the category in the JSX wrapping the rendered text in a span tag. Example:
    • { category === CATEGORY_FAST && <span>{ _x( 'Good', 'Performance rating', 'google-site-kit' ) }</span> }

  2. Edit assets/js/modules/analytics/components/common/TrackingExclusionSwitches.js

    • Within the <p> tag in the JSX, change the two conditional render statements to a single statement using inline conditional syntax ( condition ? true : false )

  3. Edit Assets/js/modules/analytics/components/common/UseSnippetSwitch.js

    • Within the <p> tag in the JSX, each __( 'translate me', 'google-site-kit' ) needs to be wrapped with a span tag when rendered

  4. Edit assets/js/modules/tagmanager/components/common/UseSnippetSwitch.js

    • Within the <p> tag in the JSX, change the two conditional render statements to a single statement using inline conditional syntax ( condition ? true : false )

  5. Edit assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidget/TotalUserCount.js

Here are further locations in the code where __( 'translate me', 'google-site-kit' ) needs to be wrapped with a span tag when rendered:

  1. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/components/setup/CompatibilityChecks/CompatibilityErrorNotice.js#L91-L95
  2. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/components/user-input/UserInputSelectOptions.js#L159-L163
  3. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/adsense/components/settings/SettingsView.js#L134-L137
  4. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/analytics/components/common/AccountCreate/index.js#L169-L172
  5. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/adsense/components/setup/SetupAccountApproved.js#L97-L115
  6. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/analytics/components/settings/SettingsView.js#L121-L124
  7. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/analytics/components/settings/SettingsView.js#L106-L111
  8. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/tagmanager/components/settings/SettingsView.js#L69-L72
  9. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/tagmanager/components/settings/SettingsView.js#L81-L84
  10. https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/tagmanager/components/settings/SettingsView.js#L98-L101

Test Coverage

Visual Regression Changes

QA Brief

Changelog entry

abdullah1908 commented 3 years ago

@jamesozzie I am unable to replicate this at my end. Here are the tricks I applied:

Server Specs:

jamesozzie commented 3 years ago

One more report of this notice in the WordPress support forums, Site Kit 1.26.0, unknown at what stage this occurs.

image

mxbclang commented 3 years ago

@abdullah1908 Can I get an update on testing for this one? Thanks!

abdullah1908 commented 3 years ago

@bethanylang in the latest activity I couldn't reproduce this issue.

mxbclang commented 3 years ago

Thanks @abdullah1908. I'm working on writing up a clearer process for L1 > L2 escalation, but in the meantime, please tag me in an issue if you cannot reproduce so I can escalate.

mxbclang commented 3 years ago

@adamsilverstein Passing over to you for L2 review.

felixarntz commented 3 years ago

@tofumatt @aaemnnosttv @eugene-manuilov Any idea what type of problem could cause this error? From looking at the different support threads, it appears this can happen in various components/screens.

aaemnnosttv commented 3 years ago

@felixarntz looks like the most consistent culprit is maybe the displayValue passed to a ReportMetric within LabReportMetrics.

I noticed that many of the reports came from non-English speakers so it may be related to this issue: https://github.com/facebook/react/issues/11538 (although not sure why it would cause a problem in that component/tree specifically more than others)

Edit: see related bug in Chrome https://bugs.chromium.org/p/chromium/issues/detail?id=872770

Edit: @felixarntz if Google Translate is the culprit here (which it seems like it may be) then I think the reason why it may happen in report metrics compared to elsewhere in the app is that we're passing the display value from the API into the component which isn't localized unlike virtually everything else. So this issue is likely specific to places in the app where we do this rather than providing a proper localized value.

aaemnnosttv commented 3 years ago

Update: I was able to consistently reproduce the error here, so this has ACs now and is ready for an IB 👍

jamesozzie commented 3 years ago

One additional report of this error. No additional insights requested.

aaemnnosttv commented 3 years ago

Thanks @Hazlitte I'm glad that the solution seems to be a fairly simple one, however this issue isn't specific to the PageSpeed component (although that does seem to be where it was most commonly reported). Ideally we would solve this plugin-wide although I'm not sure there is a good solution for that.

We should at least do our best to audit where this is currently possible and fix the existing cases as there are more than just this one. This should also help us identify it preemptively in the future.

In poking around, I've also seen this possible to throw here:

Let's take a few minutes and try to find any other cases to fix while we're at it.

Hazlitte commented 3 years ago

@aaemnnosttv I used regex to search for similar code in JSX that might be susceptable to the same bug. I found 99 results in 48 files. I will update the IB to include this approach and am happy to be assigned to execute this task if you are comfortable with the approach.

Hazlitte commented 3 years ago

@aaemnnosttv Following our discussion I successfully recreated this bug in a number of places. I also investgated other instances where the Switch component is being used as well as the date dropdown and the pie chart. There doesn't appear to be a consistent solution as the bug can be caused in slightly different ways. I have modified the IB with the 5 components which I found that need updating and reduced the estimate to a 7.

aaemnnosttv commented 3 years ago

@Hazlitte I think it's important to note the condition which results in the error here. The problem manifests when an element has multiple text node/element children that render conditionally and React panics when it can't reconcile the changes made by Google Translate when trying to change what is conditionally rendered.

I think in all of these cases we have multiple children that chooses only one, much like an if/else or a switch case. The basis of the solution here is to reduce the children in JSX down to 1, or change the conditional children from text nodes to DOM elements (e.g. by wrapping with a span tag).

The IB looks pretty good for the most part, a few points to address:

Update each conditional render of the category in the JSX wrapping each one in a span tag. Example:

  • <span>{ category === CATEGORY_FAST && _x( 'Good', 'Performance rating', 'google-site-kit' ) }</span>

This would fix the error, but it would result in 2 extra empty span tags rendered for the cases other than the category match. So in this case, we simply need to move the span to be part of the conditionally rendered output rather than outside it.

Within the <p> tag in the JSX, change the two conditional render statements to a single statement using inline conditional syntax ( condition ? true : false )

We have generally avoided using ternary in JSX so far as it can result in blocks which are quite detached/far from the logic responsible for rendering. In this case, I think it makes sense to use but only for simple condition ? __( a ) : __( b ). Anything that starts getting into multiple lines per-case should be wrapped instead and use condition && aBlock and ! condition && bBlock. I think what you have here mostly does this anyways, but I just wanted to explain the reasoning how we should think of each going forward.

Edit assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidget/TotalUserCount.js

  • Change the conditional render of elements in the <h3> tag to two inline conditional render syntaxes
  • Also wrap the render of { dimensionValue } in a <span> tag.

For cases like this which are a bit more complex, it would be good to include the lines for context: https://github.com/google/site-kit-wp/blob/4739c5c87ea9e068b0ef0c2beda4fe79e185f9e9/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidget/TotalUserCount.js#L96-L106

Instead of two inline ternary conditionals, we should simply wrap __( 'All Users', 'google-site-kit' ) with a span tag for the cases where it is not wrapped in a Link. Your second point there is good though.

Also, I found another case or two that we should update (see below). Please take another look now that we have a better understanding as to what causes this specifically and see if there are any others you can find as well.

Hazlitte commented 3 years ago

@aaemnnosttv Thanks for your comments. I updated the approach in the IB and added further locations I found in the code where it appears to be susceptible to this bug.

aaemnnosttv commented 3 years ago

@Hazlitte I think this is good to go now, although please fill in the sections for Test Coverage and VRT. I don't think any changes are needed here, but we should always be explicit about this.

Hazlitte commented 3 years ago

Thanks @aaemnnosttv, I have updated the test sections.

aaemnnosttv commented 3 years ago

Thanks @Hazlitte ! In this case, I don't think N/A is appropriate since these changes may result in test failures (for example if one of these components has a snapshot test), but also there should not be any visual changes as a result of these and N/A doesn't really set any expectations about how to handle that. Since both of these are technically possible given the changes we're outlining here, we should be more explicit about what to expect. Again, a failing snapshot test due to added span tags is probably fine to update, but a failing VRT test should not be ignored/approved.

aaemnnosttv commented 3 years ago

IB ✅

wpdarren commented 3 years ago

@aaemnnosttv I have tried a few times and have not recreated this issue on the latest release with the AC's. I would like to at least see the issue in action to make sure that the QAB tests pass. Any ideas why I might not be able to recreate it?

aaemnnosttv commented 3 years ago

@wpdarren I'm not sure exactly why you might be having a hard time reproducing it, but here's a screencast using a site running on 1.30.0 when translating to Russian.

https://user-images.githubusercontent.com/1621608/115512305-81b15080-a28a-11eb-813b-6820a67e901a.mp4

wpdarren commented 3 years ago

@aaemnnosttv still unable to recreate this issue on latest release 1.30.0, when translating Russian.

https://user-images.githubusercontent.com/73545194/115543748-a6f98b00-a299-11eb-8a5c-473f094ff783.mp4

Using Chrome Version 89.0.4389.128 (Official Build) and on MacOS. I have tried on a few different test sites including my personal site and unable to recreate the issue.

Hazlitte commented 3 years ago

@wpdarren It took me a while to figure out how to trigger this bug when I worked on it. We could do a screen share if you like and see if we can get it to occur.

wpdarren commented 3 years ago

QA Update: Fail ❌

@Hazlitte I found an error Unexpected text node type 1 on the Analytics module, in All Traffic widget. As you can see, the message appeared when I clicked on a slice. I did notice after that it also appeared when I clicked on the Locations tab, but when the page was refreshed it did not occur a second time.

image

Edit: I also found the error when clicking through the tabs a few times.

image

Verified:

aaemnnosttv commented 3 years ago

@wpdarren I think the inline error is probably acceptable for now – the main focus of this issue is the full-page unrecoverable error as shown in my screencast above. This may be something we can fix in a subsequent issue, but it may be from the Google charts as well, in which case we may not be able to do anything about it.

If the Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node. errors are fixed and there are no full-page errors in their place (which would be just as bad) then this is a substantial improvement for the end user as the dashboard and functionality are still in tact as far as I can tell.

cc: @felixarntz

aaemnnosttv commented 3 years ago

Taking a closer look at this today, the Unexpected type of text node error is raised in the Google charts library

So far I've observed it originating within our UserCountGraph as well as the UserDimensionsPieChart although inconsistently. Unlike the previous error, this one is non-blocking and even dismissible! Kapture 2021-04-22 at 09 31 35

There is currently one issue on the visualization issues repo that references this error which is said to be addressed in v49 (the version we use now) but it looks like perhaps before the error was breaking the whole chart. See https://github.com/google/google-visualization-issues/issues/2827

Given that the removeChild on Node error has been resolved here which was much more severe I think a follow-up issue is sufficient to address this in the future, although it seems like we may need to wait for this upstream.

Sending this back to QA for a final pass, and I'll create a follow-up issue for the remaining error/notice.

wpdarren commented 3 years ago

QA Update: Pass ✅

Verified:

PedroLuisDionisioFraga commented 1 year ago

I was having this problem when creating a new repository, my navigator has a primary language in pt-BR and web github is in english, i remove automatic translate and resolve the problem.

https://github.com/google/site-kit-wp/assets/122636344/370efffd-6dbb-4982-ac53-b6398aa10329

ShEpic commented 1 year ago

This error appears in the chatbot moemate.io when I try to translate the page from english to russian via the bing translator or the custom excticion of google translate. And this error is fullscreen.