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

Preview development mode warns on invalid JSON if excluding all logged in users from Analytics #7939

Closed
LuckynaSan opened this issue Jun 11, 2021 · 11 comments
Labels
Group: Integration Integration with other platforms and plugins Group: WordPress Changes related to WordPress or Gutenberg integration Type: Bug Something isn't working

Comments

@LuckynaSan
Copy link

LuckynaSan commented Jun 11, 2021

Bug Description

1.) Story passes AMP validator, but Story's debug mode in the story preview is outputting the below warning when you enable the option to exclude all logged in users from Analytics in Site Kit by Google:

 INVALID_JSON_CDATA at 6:106
 The script tag contains invalid JSON that cannot be parsed

Screen Shot 2021-06-11 at 12 06 36 PM

Screen Shot 2021-06-11 at 12 08 40 PM

2.) Debug mode in the story preview is outputting inconsistent warnings on refresh on the same URL as above

Inconsistent.Debug.Results.mov

Expected Behaviour

1.) The warning should not be present
2.) Debug results should be consistent.

Steps to Reproduce

  1. Site Kit settings > edit Analytics > Exclude from Analytics all logged-in users
  2. Preview story
  3. See debug tab and notice invalid JSON error
  4. Refresh page and notice inconsistent results

Screenshots

Additional Context

related support topic: https://wordpress.org/support/topic/debug-error-issue-json/

  • Plugin Version:
    Web Stories 1.7.2
    Site Kit by Google 1.34.0

  • WordPress Version:

  • Operating System: macOS

  • Browser: Chrome


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

Acceptance Criteria

Implementation Brief

@LuckynaSan LuckynaSan added Type: Bug Something isn't working Pod: WP & Infra labels Jun 11, 2021
@swissspidy
Copy link
Collaborator

This would be a bug in Site Kit and should be reported there.

cc @felixarntz

@aaemnnosttv
Copy link
Contributor

@swissspidy looks like a similar issue was raised in the AMP plugin before which changed the way it handled this:
ampproject/amp-wp#4430

The validator extension also does not flag this as invalid:
image

Is this something different about how Web Stories is validating the markup?

@swissspidy
Copy link
Collaborator

We use the same sanitizers, so there shouldn‘t be a difference. I can check next week though.

cc @westonruter for thoughts just in case

@westonruter
Copy link
Collaborator

I forget what the purpose is for this __gaOptOutExtension element. If it needs to be a JSON script, you could just give it the contents {}.

Otherwise, if it just has to be some other tag that is allowed in the head, why not meta? You could add this instead:

<meta id="__gaOptOutExtension">

@aaemnnosttv
Copy link
Contributor

I forget what the purpose is for this __gaOptOutExtension element.

@westonruter it is for opting out of all Google Analytics measurement in AMP.

if it just has to be some other tag that is allowed in the head, why not meta? You could add this instead:

<meta id="__gaOptOutExtension">

This was what we originally planned on doing, but this fails HTML validation without additional attributes.

If it needs to be a JSON script, you could just give it the contents {}.

Yes, I found this passes the validation as well, but as far as I know the contents of the element aren't used at all, it simply relies on its presence.

The AMP validator does not flag the current empty element as invalid though and we also include AMP validation in our tests so that Site Kit does not generate invalid AMP markup. Is there another form of validation that Web Stories applies?

@westonruter
Copy link
Collaborator

This was what we originally planned on doing, but this fails HTML validation without additional attributes.

Then you could just add empty name and content attributes? Like so:

<meta id="__gaOptOutExtension" name="sitekit-ga-opt-out" content="">

@westonruter
Copy link
Collaborator

The AMP validator does not flag the current empty element as invalid though and we also include AMP validation in our tests so that Site Kit does not generate invalid AMP markup. Is there another form of validation that Web Stories applies?

A JSON+LD script with empty text content is not a validation error, but rather a validation warning.

The Web Stories debugger doesn't seem to be differentiating between errors and warnings. But the AMP validator does:

image

image

@swissspidy
Copy link
Collaborator

Bit unfortunate that the Web Stories debugger doesn't mark it as a warning as it should. Definitely confusing for users.

@aaemnnosttv Are any of the suggested workarounds reasonable to add to Site Kit? e.g. adding {} contents.

In the meantime we could add some workaround in the Web Stories plugin for that.

@aaemnnosttv
Copy link
Contributor

@swissspidy we've already updated the element to use a meta tag instead – sorry, I meant to follow up here 😅

There's still some debug output that looks similar to before but the errors are different and don't seem to be related to Site Kit which might have something to do with my environment or the fact that it's mostly an empty story 😄

image

@swissspidy
Copy link
Collaborator

@aaemnnosttv Awesome, thanks. Those other errors you see are unrelated and caused by:

  • Previewing a draft (drafts don't have a rel=canonical in WP)
  • Not adding a poster image
  • Not adding a publisher logo

@swissspidy
Copy link
Collaborator

Closing this issue now as google/site-kit-wp#3593 was merged and this will be fixed in Site Kit 1.36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Integration Integration with other platforms and plugins Group: WordPress Changes related to WordPress or Gutenberg integration Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants