fotinakis
commented
8 years ago First, thanks for this! And for adding tests and all the things, this is great. Does it work when rendered in Percy? :)
Two quick thoughts (just at very quick glance, I will do a more thorough review this week):
- The ideal world is that this should be default, people shouldn't have to enable it — so that means part of
SprocketsLoader(and, if you feel up to it, shared inNativeLoaderas well—but I'm fine with not having support for it there as that is a less common path). - Can you add a test for an iframe that is not on the same domain? Selenium often throws terrible errors for single-origin policy violations.
- Optionally, it should be safe/fast and potentially not require JavaScript. I just discovered a native Selenium method to switch context to the iframe — perhaps you can test out
within_frameand see if it does what we want? Just need to find a way to get the source while within the frame context, and if Selenium has a native way to do that then it probably will let us dramatically simplify this code and probably not require JavaScript at all — http://stackoverflow.com/a/11910060/128597
chrismaximin
Note:
SprocketsLoader.SprocketsLoaderbecause it's not directly using sprockets, but it does depend on it to load the iframe content assets; also because it uses javascript to load the iframe contents, which can be potentially slow.Percy::Capybara.use_loader(Percy::Capybara::Loaders::SprocketsWithIframesLoader)SprocketsLoader, say the word.