Migrate from PhantomJS #8
Conversation
| driver.set_window_size(384, total_height) | ||
| screenshot = driver.get_screenshot_as_png() | ||
|
|
||
| # body = driver.find_element_by_tag_name('body') |
There was a problem hiding this comment.
Don't need to keep this around, but it might be more stable than resizing the viewport? It does break if the height is 0 though, which is kind of undefined right now.
There was a problem hiding this comment.
Let's leave it commented there, I can imagine that it could come in handy if the window-resizing method has any issues.
There was a problem hiding this comment.
Just one thought I had - could CSS rules on the html element affect this? I'm guessing that body.parentNode is html... like margin/padding/border on the html element?
There was a problem hiding this comment.
Good question! Part of me wonders if we should apply a global CSS reset here to counter against partial/incomplete HTML layouts (say, just <p>hello world</p>) getting padding/margin added, but I can only imagine that going poorly when someone wants a specific layout.
The whole HTML rendering is probably fairly fragile, and it's definitely undefined right now. I'm sure passing something simple in like <meta name="viewport" content="width=device-width, initial-scale=2"> breaks something we care about here.
I was gonna add a bunch of tests that generate PNGs and compare against fixtures, but I'm wondering how fragile that'll be, i.e. different OS fonts will cause test failures. As long as they pass in Docker/on Circle, that's all that really matters I guess? But it'd be good to use it to define a basic spec of "enter this HTML, get this output", and keep it consistent regardless of our implementations here.
There was a problem hiding this comment.
I was wondering if just a HTML -> image height test would do the trick, although that might fall foul of the fragility problems you raise above re. fonts etc.
HTML passed to the print key API is in fact templated, so <p>Hello, world!</p> would be placed within a HTML document with base styles etc. This is where the Timestamp/from header comes from.
Regardless, we're talking about very edge-casey stuff here, so don't let it hold up your progress!
There was a problem hiding this comment.
Good point about the template! I'd forgotten about that.
I added #13 as a starting point to catch any of the main regressions here. The image height test is probably enough to 99% of issues, but having the image snapshots gives us a way to inspect/visualise the differences so I think has a fair bit of value.
So let's get #13 in, then get this PR green against those tests. Then let's merge it in and see how it goes.
| self.assertEquals(image.size[1], 100) | ||
|
|
||
| def _test_rle(self): | ||
| def test_probably_beyond_viewport_height(self): |
There was a problem hiding this comment.
Trying to make sure that headless browsers can actually capture a full screenshot, and don't get limited to resizing to fit a virtual screen size (say, 1024x768)
notjosh
force-pushed
the
feature/no-more-phantomjs
branch
from
c6d103d to
aef831a
Compare
|
I've rebased after #7, so this should be reviewable/mergeable now. |
joerick
left a comment
joerick
left a comment
There was a problem hiding this comment.
This looks good to me! I suppose the proof is in the printin', so feel free to merge when you're happy and we can give it a go. Easy enough to revert if there are troubles.
| driver.set_window_size(384, total_height) | ||
| screenshot = driver.get_screenshot_as_png() | ||
|
|
||
| # body = driver.find_element_by_tag_name('body') |
There was a problem hiding this comment.
Let's leave it commented there, I can imagine that it could come in handy if the window-resizing method has any issues.
| driver.set_window_size(384, total_height) | ||
| screenshot = driver.get_screenshot_as_png() | ||
|
|
||
| # body = driver.find_element_by_tag_name('body') |
There was a problem hiding this comment.
Just one thought I had - could CSS rules on the html element affect this? I'm guessing that body.parentNode is html... like margin/padding/border on the html element?
2 participants
⚠️ Depends on #7. Merge that first, and I'll rebase this. ⚠️ Hi!
PhantomJS was running a painfully old build of WebKit, and made flexbox rendering a complete bummer. This PR replaces PhantomJS with Chrome (via chromedriver).
A few notes:
DejaVu Sansto match the existing PhantomJS fontapp.jsonfor Heroku - the.buildpackssystem isn't really used any more, but I suspect this project might still use it? In any case, this PR assumes a base buildpack ofheroku/python, but it will need a couple of manual steps (I can't find a way to automate it any other way?):heroku buildpacks:add heroku/google-chromeheroku buildpacks:add heroku/chromedriver"", so the behaviour is fairly undefined there, but we still get a mostly consistent block of whitespaceI booted this on a test instance (https://sirius-sandbox.herokuapp.com/), and it seems to be generating images just fine in the browser. And it now understands (but can't render - no fonts) emojis, instead of just breaking completely. Tiptoeing forward to 2019!