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

Dispose of CSSOM as a dependency #7

Closed
cburgmer opened this issue Oct 3, 2015 · 15 comments
Closed

Dispose of CSSOM as a dependency #7

cburgmer opened this issue Oct 3, 2015 · 15 comments

Comments

@cburgmer
Copy link
Owner

cburgmer commented Oct 3, 2015

Use browser's own cssom once the fix for https://code.google.com/p/chromium/issues/detail?id=161644 has been rolled out in Chrome. This will fix the underlying issue for #5.

@cburgmer
Copy link
Owner Author

@barillax
Copy link
Contributor

barillax commented Mar 4, 2016

I'm running into a related problem, where NV/CSSOM fails to parse a minified stylesheet. I'd settle for the broken font-face behavior of native CSSOM over this failure mode :(

I suspect I'll need to fork inlineresources to make a build without CSSOM included, unless there's a clever way to disable it at runtime to force it to use the browser's CSSOM. Thoughts?

@cburgmer
Copy link
Owner Author

cburgmer commented Mar 5, 2016

Thanks for the feedback. It looks like a balancing act, either support font-face for Chrome or offer more reliability in general. Just not both.

With some activity with the newly reported bug for Chrome and the short release cycles, I'm positive that we can solve this issue here soonish.

In the meantime, there's no strict requirement on CSSOM (the JS implementation), if you don't provide it, the workaround is just not going to be triggered. I believe it depends on your packaging/build setup how difficult it is going to be to setup.

I'm on a bus right now, so excuse the brevity of the answer :)

@barillax
Copy link
Contributor

barillax commented Mar 5, 2016

No worries, I wouldn't expect you to disable CSSOM until the browser support was more stable. I'll play around to get a custom build for our project in the meantime.

@cburgmer
Copy link
Owner Author

cburgmer commented Mar 6, 2016

Hey, after looking at the current code, I have to partially retract my initial statement. I believe building inlineresources without cssom will fail, as this is currently not handled at the import level. If you are willing to investigate, the following pointers might help:

@barillax
Copy link
Contributor

barillax commented Mar 7, 2016

Thanks for the tips! Will give it a go today.

@barillax
Copy link
Contributor

barillax commented Mar 7, 2016

Got it working. I'm not as familiar with the browserify way of doing things, so it's possible my approach isn't very idiomatic. Here's the changeset: barillax@20a1135

@cburgmer
Copy link
Owner Author

cburgmer commented Mar 8, 2016

That looks good to me. I'm happy to take in the changes and release another version. Good with you?

@barillax
Copy link
Contributor

barillax commented Mar 8, 2016

OK, I'll create a PR momentarily!

@barillax
Copy link
Contributor

barillax commented Mar 8, 2016

Done: #9

@cburgmer
Copy link
Owner Author

There's yet another bug in Chrome (https://bugs.chromium.org/p/chromium/issues/detail?id=660663) and a work around committed in 6ab44b8.

cburgmer added a commit that referenced this issue Jun 4, 2017
cburgmer added a commit that referenced this issue Jun 4, 2017
@kapouer
Copy link

kapouer commented Nov 20, 2017

This can be closed since version 0.4.0, right ?

@cburgmer
Copy link
Owner Author

This is still pending a release if I'm not mistaken.

cburgmer added a commit that referenced this issue Mar 18, 2018
cburgmer added a commit that referenced this issue Mar 18, 2018
@cburgmer
Copy link
Owner Author

Sorry for the noise. There was one failing test, which depended on the baseURI of the test runner. Took a while to figure out that the issue is indeed fixed in Chrome, just a corner case with an "illegal" relative URL was to blame.

@cburgmer
Copy link
Owner Author

Released

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

No branches or pull requests

3 participants