Conversation
geriux
left a comment
There was a problem hiding this comment.
Working great 🎉 thanks for this fix! 🙌
|
|
||
| class DOMParser { | ||
| parseFromString( string ) { | ||
| return jsdom.html( string ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…gutenberg-mobile into update-master-20200113
|
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: 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 |
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
DOMParserimplementation based on the API of ourjsdomversion.To test:
PR submission checklist:
RELEASE-NOTES.txtif necessary.