Yoast / yoast-components

Accessible React components by Yoast
GNU General Public License v3.0
21 stars 6 forks source link

Make getFeed work in IE11 and Edge #800

Closed afercia closed 5 years ago

afercia commented 5 years ago

Summary

This PR can be summarized in the following changelog entry:

Relevant technical choices:

Test instructions

Updated instructions:

Test in the plugin

Switch the plugin to the 11935-get-feed-ie11-edge branch (see https://github.com/Yoast/wordpress-seo/pull/11968) and yarn link this yoast-component PR to the plugin:

Note: the Courses page layout is broken in IE11 because the current CSS grid implementation doesn't work in this browser. Will be addressed separately.

Screenshots from IE11:

screenshot 160

screenshot 161

(as said, the broken layout will be addressed in https://github.com/Yoast/wordpress-seo/issues/11953 )

Fixes https://github.com/Yoast/wordpress-seo/issues/11935 Fixes https://github.com/Yoast/wordpress-seo/issues/11073

andizer commented 5 years ago

CR Done 👍

afercia commented 5 years ago

Seems there are additional issues with Edge, where each feed item get parsed but many of the item elements are undefined (which can also explain why the cars 🚗 cards look "empty" in Edge, see https://github.com/Yoast/wordpress-seo/issues/11953#issuecomment-450953569):

screenshot 171

At the moment, I have no idea what's going on.

afercia commented 5 years ago

Moving back to development as the Edge issue needs more investigation, and some help 🙂

afercia commented 5 years ago

Re: the Edge problem: after an insane amount of time spent investigating on this, turns out in Edge getElementsByTagName doesn't recognize namespaces, at least in some cases. It was introduced here to check for the existence of an element: https://github.com/Yoast/yoast-components/commit/aa87a129c6f0bfc2aff9c4a542f1693636bc11e6

What's happening is:

afercia commented 5 years ago

Additionally, in the WordPress dashboard widget the last link has no href as seems getFeedMeta() is not able to parse /rss/channel/link (not even /rss/channel/title and /rss/channel/description):

screenshot 2019-01-04 at 10 27 36

[Edit]: turns out the cause for this is simple: getElementsByTagName() can't recognize an element with a passed tagname of /rss/channel/link ...

afercia commented 5 years ago

Last commit re-implements the check for element existence but this time using XPath count().

However, worth noting a few things:

This works if the element doesn't exist or the XPath expression is malformed in the part that represents the element for example: getXPathText( "child::content:XYZencoded", parsed, snapshot, nsResolver );

It doesn't work if the malformed part is the namespace part: it will throw a DOMException, for example: getXPathText( "child::XYZcontent:encoded", parsed, snapshot, nsResolver );

Regardless, I'm not sure why we want to check for the elements existence in the first place. We're passing these elements to React (unless we're using this in other places too): if the element doesn't exist this method was already returning undefined (note: I've changed it to null) so React won't render anything.

Worth also noting some of the components that use this, have required properties: if the element is missing, React will throw a warning. /Cc @herregroen

afercia commented 5 years ago

Changed console.log to console.error for both the Dashboard widget and Courses getFeed examples.

Worth noting these examples just re-use the code that's used in wordpress-seo, where console.log is still used. Discussed a bit with @IreneStr and maybe it would be better to display some useful message to users when the feeds can't be parsed. This should be addressed separately though.

IreneStr commented 5 years ago

2nd CR 👍

Dieterrr commented 5 years ago

Acceptance done 👍 Everything works as advertised :) Styling indeed still broken in IE, in Edge is fine.

@afercia Thanks for the very clear PR and instructions!