-
-
Notifications
You must be signed in to change notification settings - Fork 144
Enable mobile emulation #527
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
base: main
Are you sure you want to change the base?
Conversation
This provides an LSP interface to the codebase, allowing for easier navigation, searching, etc. in editors that support LSP.
This will make debugging rspec examples easier.
Note that when mobile is true, *both* mobile emulation and touch support are enabled. The spec checks whether mobile support has been enabled by checking for touch support with JavaScript.
This way we can use Cuprite to register a driver that in turn enables mobile emulation in the browser.
This is a bit hacky; ideally, a caller wouldn't both specify a non-mobile size *and* mobile: true.
Add details of resize and mobile behavior to the README. Note that the behaviour on height or width of 0 was pre-existing; I just documented it and added a spec.
f5aa29f
to
6753f84
Compare
lib/ferrum/page.rb
Outdated
maxTouchPoints: 1 | ||
) | ||
|
||
command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uncertain about this. Ideally the caller would always specify a mobile height, width, and scale factor. But in principle that doesn't always happen, so I decided to override them here.
Perhaps we should raise an exception if a height and width other than 0
are passed along with mobile: true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why setting it hard in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't, but it's also surprising - if we didn't hard-code the resolution here, users would have to know:
- That they need to set the resolution at the same time as enabling mobile, and
- What the right resolution is for mobile devices.
Hence my suggestion that perhaps I should make this raise an exception if the user also passes height or width? That way it should be really clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about it, the more I like the exception idea - I'll add one in. LMK what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have made it raise an exception now, so the behaviour is:
context "when a width is passed" do
it "raises an error" do
expect do
page.resize(width: 100, height: 0, mobile: true)
end.to raise_error("specifying `mobile: true` causes automatic resizing to mobile dimensions")
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing with @DanielHeath, will not implement hard-coded resizing in Ferrum, but instead provide common mobile devices in Cuprite to choose from.
} | ||
end | ||
|
||
def is_mobile? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really test whether mobile emulation is enabled, but I couldn't find a good way of doing that directly via JavaScript evaluation. So we use touch emulation as a proxy.
Gemfile
Outdated
@@ -4,6 +4,7 @@ source "https://rubygems.org" | |||
|
|||
gem "byebug", "~> 11.0", platforms: %i[mri mingw x64_mingw] | |||
gem "chunky_png", "~> 1.3" | |||
gem "debug", "~> 1.10", require: false, platform: :mri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove unnecessary changes not related to Enable mobile emulation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure - would you like me to raise it as a separate PR? I found both deubgger and LSP support handy while developing this PR.
@@ -194,7 +194,7 @@ Ferrum::Browser.new(options) | |||
* `:proxy` (Hash) - Specify proxy settings, [read more](https://github.com/rubycdp/ferrum#proxy) | |||
* `:save_path` (String) - Path to save attachments with [Content-Disposition](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition) header. | |||
* `:env` (Hash) - Environment variables you'd like to pass through to the process | |||
|
|||
* `:mobile` (Boolean) - Specify whether to enable mobile emulation and touch UI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need flag for browser instantiation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intent was to expose this via driver registration, as per Selenium.
I have a draft PR open for Cuprite that uses this; see e.g.:
Capybara.register_driver(:cuprite_mobile) do |app|
options = { mobile: true }
options.merge!(inspector: true) if ENV["INSPECTOR"]
options.merge!(logger: StringIO.new) if ENV["CI"]
options.merge!(headless: false) if ENV["HEADLESS"] == "false"
Capybara::Cuprite::Driver.new(app, options)
end
lib/ferrum/page.rb
Outdated
maxTouchPoints: 1 | ||
) | ||
|
||
command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why setting it hard in the first place?
dffdd4e
to
06ca9b4
Compare
Moving this back to draft, for now, while I clean up following a move of the mobile specific stuff into Cuprite. |
06ca9b4
to
f6a5577
Compare
This should really live at the Cuprite level, emulating specific devices.
f6a5577
to
bd578fb
Compare
Okay I think this is good for review again :) Note the new, improved, and not-hard-coded API. In Cuprite you'd do:
|
There is a request on Cuprite to support mobile emulation: rubycdp/cuprite#216
A prerequisite for this is for Ferrum to support
mobile: true
inPage#resize
. This PR adds that support.Note that passing
mobile: true
also sets the screen resolution to that of an iPhone 14; see this comment. Perhaps there's a nicer way of doing this?Merging this should let us close #94.