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

Make IE happy #76

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from
Open

Conversation

invisoft
Copy link

Fix #72, or at least the IE part - not tested on a Mac.

@tomhodgins
Copy link
Contributor

This is not an IE error, or a Safari error, or even likely a browser error!

This bug is likely because somehow the queries parsed by EQCSS are losing their style. This bug has been reported (and I've seen it on Chrome OS X) but seems to happen only on certain websites, not seeming to come from a problem inside EQCSS.js

This means if there's something happening on the page that is causing style information to be dropped from EQCSS.data and it's throwing this error, if we 'fix' that by checking for code to exist before trying to replace it, or like this PR does to ensure that code is always at least an empty string by the time you try to replace it… it shouldn't change the function of EQCSS for sites where it works, but on sites where something strange is happening all you've done is remove the debugging info letting you know your site is having some issues! I don't think ignoring the problem is a good solution. This feels like taping over the 'Check Engine' light in your car - it doesn't fix the problem, it just lets you feel better about driving a broken car.

It would be beneficial to build some test cases so we could narrow down how and when this is happening. On the last site that reported this bug I was able to verify the queries they were using as valid, I was able to verify that initially the EQCSS syntax does get parsed and loaded by the EQCSS.js plugin correctly, and when I test the queries and EQCSS.js in isolation from the rest of the site they function correctly. But on that site (and possibly yours as well) something present on these page is causing the queries that EQCSS.js has already parsed and loaded into EQCSS.data to drop the style it's holding for that query, and you see this error the next time EQCSS tries to apply your styles because you can't run a replace() on an undefined object.

So what happens between the successful EQCSS.js parse, and the moment where EQCSS.apply() tries to replace a (now) undefined style it used to have? Building stripped down test cases where the bug is still present, and linking to existing sites where this problem is occurring would help us figure out what's happening, but I don't like the idea of ignoring it.

Troubleshooting ideas

Verify your Syntax

You can load your queries in a new document that just contains your queries and EQCSS.js, once the page has loaded, open your JS console and type EQCSS.data to view the parsed data. There will be one object for each query it parsed, where you can view the selector, conditions, and the style information the plugin is aware of.

Another way you can verify that your queries are formatted correctly and able to be parsed by EQCSS.js is to run your EQCSS syntax through eqcss-parse (from npm) with a command like this (assuming your CSS file was style.css)

curl http://path/to/style.css | eqcss-parse | prettier

This will download your file, pass it to eqcss-parse to extract the information from the queries and turn it into JavaScript objects, and prettier to format it nicely for human-reading. If you open the resulting output you should be able to visually inspect what the plugin would load. I'm hoping there aren't any missing or empty style on those objects.

Verify EQCSS.js

If you create an empty HTML document and add EQCSS.js, and either the link to your EQCSS syntax or the output from eqcss-parse, including either should load the queries into the plugin. Once the queries are loaded by EQCSS.js you can run this in your command-line to ensure that every object inside EQCSS.data has a value for style, and if it doesn't, the console will log which query is missing its style:

EQCSS.data.forEach(query => { if (!query.style) console.log(query) })

This code means: “For each query in EQCSS.data, if the query has no style, log that query” and any queries that have been parsed but are now missing style will be shown in your JS console

Test the live site

On the sites where you see the errors occurring, try running the same code to see which queries are dropping style:

EQCSS.data.forEach(query => { if (!query.style) console.log(query) })

If you're seeing the errors reported above you should have something showing up here. What might be interesting to note:

  • does it happen consistently every reload? (so far it hasn't)
  • does it always happen to the same queries (so far it hasn't)

So far nothing has turned up as far as looking at EQCSS.js on pages where this happens, but I'd have to have access to all of the JavaScript they were running to see how something might be interacting funny. For now the most useful thing we can to do nail this down is provide the smallest still-offending test case

@tomhodgins
Copy link
Contributor

Also, this seems to be the same problem as the iOS issue here: #71

@invis-dev
Copy link

invis-dev commented Oct 1, 2017

OK, so I did a little more digging. In this case, for the site I'm working on, it is an IE issue. Everything works fine in every other browser I've tried.

I have some inline SVGs with an embedded non-empty style sheet. However, IE thinks they are empty.

EQCSS.load = function() {
    ...
    EQCSS.process(styles[i].innerHTML);

innerHTML is undefined for the SVG style sheets.

In this context I don't think forcing code to at least an empty string is such a bad fix. Maybe it hides other problems, but then again I don't think the script should crash if something unexpected happens.

Maybe there should be a debug version of the script so that other bugs can be more easily traced?

@tomhodgins
Copy link
Contributor

Ah, this could be separate from the other issue! I'd prefer to fix it, if possible, rather than suspend the error letting you know it's broken. Are you able to produce a stripped-down test that reproduces the bug you're seeing?

Some thoughts based on what you've said:

  • Are there @element queries inside the <style> tag inside the <svg>?
  • In some versions of IE the contents of a <style> are accessed via style.styleSheet.cssText rather than style.innerHTML
  • Technically <svg> and its children an an XML document, so in some situations you would require getElementsByTagNameNS() rather than getElementsByTagName() for retrieving any tags inside the <svg>

In order to try to fix this I'll need a test case - until now you haven't even mentioned which versions of IE this is broken in, I'll need to know everything you know about where the bug is showing up and how to recreate it :D

@tomhodgins
Copy link
Contributor

tomhodgins commented Oct 1, 2017

I think I might have a solution - it does appear IE doesn't have a way to access the innerHTML of <svg> elements, there's a polyfill for this, but it appears if you really wanted to do this, it would involve serializing the XML yourself and digging through that tree to find it.

I think in the case of EQCSS, not supporting @element queries inside SVG is okay - Internal SVG was never a goal anyway, so I think the easiest workaround might be to never even attempt to process <style> tags inside <svg>.

Unfortunately I'm not sure the best way to do this. Both HTML and SVG <style> tags will return style as their tagName, and so I'm not sure how to use JavaScript to detect which is an HTMLStyleElement and which is an SVGStyleElement, but one functional test we can do is to check the namespace of the node. SVG will always have its own type, so if we check for SVG namespace maybe we can use that to avoid processing SVGStyleElement tags :D

I just wrote a quick if statement that seems to dodge the problem, does this work for you:

// Process
if (styles[i].namespaceURI !== 'http://www.w3.org/2000/svg') {

  EQCSS.process(styles[i].innerHTML);

}

I believe that should fix it, but I wonder if there's a better way to detect this!

@tomhodgins
Copy link
Contributor

tomhodgins commented Oct 3, 2017

Ah, a better way might be to check the constructor string:

// Process
if (styles[i].toString() !== '[object SVGStyleElement]') {

  EQCSS.process(styles[i].innerHTML);

}

Testing these approaches to avoid processing any SVGStyleElements in all browsers (not just IE)

Edit:

This also works, not sure which is best!

if ('SVGStyleElement' in window && style[i] instanceof SVGStyleElement) {

  EQCSS.process(styles[i].innerHTML);

}

@invis-dev
Copy link

invis-dev commented Oct 8, 2017

My preference would be:

// Skip SVG styles
if (styles[i].namespaceURI == 'http://www.w3.org/2000/svg') continue;

// Test whatever else

var innerHtml;
if (innerHtml = styles[i].innerHTML) {
  EQCSS.process(innerHtml);
} else {
  // Log all sorts of things
}

Again, I don't think EQCSS should bork everything else when it runs into a situation it doesn't understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants