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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b0efec
Update Gutenberg
koke Jan 13, 2020
4a62b9c
Adds missing DOMParser implementation
koke Jan 13, 2020
397397b
Add comment on DOMParser
koke Jan 14, 2020
7627872
Modify XPath methods to fix failing mobile UI tests
mkevins Jan 14, 2020
65f62ad
Merge branch 'update-master-20200113' of github.com:wordpress-mobile/…
mkevins Jan 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gutenberg
Open in desktop
Submodule gutenberg updated 186 files
8 changes: 8 additions & 0 deletions src/jsdom-patches.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,3 +238,11 @@ Object.defineProperties( Node.prototype, {
},
},
} );

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

@cameronvoell cameronvoell Jan 14, 2020 •
edited
Loading

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.

}
}

global.DOMParser = DOMParser;