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

Update Gutenberg to master #1771

Merged
merged 5 commits into from
Jan 14, 2020
Merged

Update Gutenberg to master #1771

merged 5 commits into from
Jan 14, 2020

Conversation

koke
Copy link
Member

@koke koke commented Jan 13, 2020

WordPress/gutenberg#18132 introduced a new way of doing accessibility labels, and relies on DOMParser, which doesn't exist in our jsdom implementation.

This PR adds a simple DOMParser implementation based on the API of our jsdom version.

To test:

  • The app should run without crashing
  • CI tests should pass

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@koke koke added the Web breakage Caused by incompatible changes on Gutenberg web label Jan 13, 2020
@koke koke requested review from geriux and mkevins January 13, 2020 09:25
Copy link
Contributor

@geriux geriux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working great 🎉 thanks for this fix! 🙌


class DOMParser {
parseFromString( string ) {
return jsdom.html( string );
Copy link
Contributor

@cameronvoell cameronvoell Jan 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing gb-mobile branch @koke

For this line, I'm comparing our parseFromString inner code: https://github.com/iamcco/jsdom-jscore-rn/blob/a562f3d57c27c13e5bfc8cf82d496e69a3ba2800/lib/jsdom.js#L50-L67 to Gutenberg's jsdom parseFromString inner code: https://github.com/jsdom/jsdom/blob/d240291edbe7d4180a5152993ced7950c834dc57/lib/jsdom/living/domparsing/DOMParser-impl.js#L12 and they look quite different (hope I'm comparing correct code :-P). For example, Gutenberg jsdom disables scripting presumably for security concerns. Perhaps we can add a comment that this simple implementation is being added to accommodate this change: WordPress/gutenberg#18132, and is not a full-fledged implementation of DOMParser parseFromString?

Also, it seems the parseFromString is only used in a newly added stripHTML method, which received some constructive criticism, and advice for an alternative implementation of stripping HTML from the RichText in this comment: WordPress/gutenberg#18132 (comment). If we add a comment to your fix here, we might be better prepared to remove this code if someone removes the stripHTML and parseFromString method from Gutenberg code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, I added a comment. I don't see that our implementation of jsdom allows to disable scripting, but it also doesn't seem like a big risk, since we are not rendering into the DOM, and only using this for accessibility labels.

@mkevins
Copy link
Contributor

mkevins commented Jan 14, 2020

Thanks for implementing this @koke! As discussed, the label changes in the same PR (WordPress/gutenberg#18132) are causing the mobile UI tests to fail.

Many tests are failing since the remove block button was not found by the driver (so all tests in a series would fail after that first failure).

I was able to get the tests passing again by using a different XPath method for the remove block button label: [contains(@content-desc, "...")] instead of [@content-desc="..."]. This is sufficient to have all tests passing on Android, however, I'm uncertain whether the previous specificity was intended (e.g. the tests no longer assert that Android button labels include instructions to "Double tap"). Additionally, some other labels needed a period removed at the end to get an element match.

I have pushed these changes to this PR. If it turns out the specificity of using strict equality is necessary, feel free to revert and/or use an alternative method to get the labels matching again.

Note: there are many places in the UI tests that seem ripe for refactoring, however, I kept the diff here to a minimum, with the primary focus of unblocking other PRs.

cc: @Tug

Copy link
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Tug Tug merged commit fc03918 into develop Jan 14, 2020
@SergioEstevao SergioEstevao deleted the update-master-20200113 branch January 14, 2020 11:34
@geriux geriux mentioned this pull request Jan 15, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Web breakage Caused by incompatible changes on Gutenberg web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants