-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Launch immersive experiences directly #1171
Conversation
This PR can be tested from the command line with: (deprecated) |
15751a8
to
9f956d4
Compare
I have updated the PR so that it uses the XPath of the parent element, which is a more flexible solution. |
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 think I told @felipeerias about this in private, but I'll add it here for everybody interested to know. The idea of using an extension is nice, but the problem is that the Chromium backend won't have extensions support initially as it's a lot of work. So provided that we don't have a solution at the moment I'd prioritize one that does not involve extensions.
That said I agree that having an extension that inspects the DOM is likely the best we could do, so I indeed have mixed feelings.
09a9140
to
8aecce9
Compare
This implementation might be the best that we can do, but it still has several issues. First of all, the command to launch Wolvic in immersive mode needs to know the exact element in the page which will open the WebXR experience. If the page changes, it will be necessary to update that command. Secondly, the approach of using a Web extension to trigger specific elements in the page will not be possible on Chromium, because we don't support extensions there just yet. Third, in some cases our extension is not able to reach the immersive button because the WebXR experience is inside of an iframe from a different origin, and security rules prevent us from accessing a cross-origin object from the embedding page. It might be possible to work around this issue by passing messages between the content scripts running on the page and on the iframe. This last problem is particularly painful because it affects HeyVR, a large repository of WebXR games: the pages are in Finally, some pages might have specific quirks that we have to consider. For example, Moon Rider requires two clicks to open the immersive experience, and Magical Reflections requires the user to accept cookies before anything else. With all that in mind, the implementation in this PR does work reasonably well. Check out the following video: ScreenRecording_2024.07.10-16.57.13.mp4Command to launch A-Painter: (deprecated) Command to launch Moon Rider: (deprecated) |
8aecce9
to
701c442
Compare
if (targetElement) { | ||
logDebug('Target element found, calling click()'); | ||
targetElement.click(); | ||
targetElement.click(); |
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 two?
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.
There was a case where one click()
didn't work. I think that I will remove this and test again different experiences, to check if it is still needed.
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.
So, we still have the two clicks... does it mean it's still required?, I still don't get why
@@ -128,6 +128,12 @@ public class VRBrowserActivity extends PlatformActivity implements WidgetManager | |||
public static final String EXTRA_KIOSK = "kiosk"; | |||
private static final long BATTERY_UPDATE_INTERVAL = 60 * 1_000_000_000L; // 60 seconds | |||
|
|||
boolean mOpenInImmersive = false; |
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 think this should be private
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.
Yes, of course.
app/src/common/shared/com/igalia/wolvic/ui/widgets/Windows.java
Outdated
Show resolved
Hide resolved
app/src/common/shared/com/igalia/wolvic/ui/widgets/Windows.java
Outdated
Show resolved
Hide resolved
7aad147
to
f0f25fb
Compare
@svillar I have updated the PR following your comments. I also had to allow autoplay when we start in this mode to remove an error with Moon Rider, although for some reason it is still not working properly (it was working fine before). |
Access Mars: (deprecated) (Note that depending on your setup you might have to add |
f0f25fb
to
a2b14df
Compare
public static final String EXTRA_OPEN_IN_IMMERSIVE = "open_in_immersive"; | ||
// Element where a click would be simulated to launch the WebXR experience. | ||
public static final String EXTRA_OPEN_IN_IMMERSIVE_PARENT_XPATH = "open_in_immersive_parent_xpath"; | ||
public static final String EXTRA_OPEN_IN_IMMERSIVE_ELEMENT_XPATH = "open_in_immersive_element_xpath"; |
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.
private?
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 other Intent parameters in that file are public.
I think it's a good idea for documentation purposes, as a way to make clear that these strings are supposed to be used outside of this class.
if (targetElement) { | ||
logDebug('Target element found, calling click()'); | ||
targetElement.click(); | ||
targetElement.click(); |
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.
So, we still have the two clicks... does it mean it's still required?, I still don't get why
|
||
// Limit the number of times that we can try to launch the experience, to avoid an infinite loop. | ||
var retryCounter = 0; | ||
const RETRY_LIMIT = 20; |
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 know this is kind of arbitrary but a maximum 20" retry seems to much to me. Maybe good for poor connections though...? In that case the user would have some other problems. My point is that users won't get any idea of what's going on for a lot of time. The feeling after 4-5" is that nothing works, nobody will wait for 20" for an app to launch.
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.
Yes, but that's a problem in general with this feature. Some experiences just take a long time to load from the network.
In this initial implementation, we show the browser window before launching the immersive experience. Eventually my plan is to go directly from the loading screen to the immersive experience.
app/src/main/res/raw/fxr_config.yaml
Outdated
apz.one_touch_pinch.enabled: false | ||
apz.one_touch_pinch.enabled: false, | ||
# allows scripts to open WebXR immersive sessions | ||
dom.vr.require-gesture: false |
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 feel a bit uneasy having this. Would it be possible not to add this here and manually add it to the runtime settings builder in EngineProvider.kt
somehow? We don't need to spend much time on that, but if it's possible to add it just whenever is needed it'd be much better.
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.
One possibility could be to do it in SessionUtils.prepareConfigurationPath()
, which copies the configuration file from the assets to the device's filesystem. Basically, the idea would be to append an extra line to the file if needed:
final String REQUIRE_GESTURE_CONFIG = "dom.vr.require-gesture: false";
WidgetManagerDelegate widgetManager = (WidgetManagerDelegate) aContext;
if (widgetManager.isOpenInImmersive()) {
outputStream.write(REQUIRE_GESTURE_CONFIG.getBytes());
}
Another approach could be to have two files in the assets, and copy one or the other depending on the current configuration. Maybe that would be better?
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 think it's enough with the first approach you mentioned. Adding a line to the stream should make it
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've been working on this but it is a bit more complex than I expected because prepareConfigurationPath()
is called very early, when we have not parsed the incoming Intent yet and so we don't know that we will be in the automatically immersive mode.
Specifically, as a result of VRBrowserActivity.onCreate()
calling
((VRBrowserApplication) getApplication()).onActivityCreate(this)
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 finally found a way to change the configuration prefs only when needed for launching in immersive mode.
The problem is that we only write the prefs file the first time that we create the browser engine, which happens several times at launch time before we have parsed the incoming Intent and we can know in which mode we are launching.
The solution is that WidgetManagerDelegate.isOpenInImmersive()
internally will check the current Intent, to cover the case where it is being called too early.
We then pass VRBrowserActivity
as the context to the objects that may initialize the engine, so eventually SessionUtils.prepareConfigurationPath()
is able to decide whether specific prefs should be added to the configuration file.
For launching Wolvic directly in immersive mode, these prefs are:
dom.vr.require-gesture: false
media.autoplay.default: 0
a2b14df
to
77692ff
Compare
77692ff
to
9533dfe
Compare
1d41b8e
to
48b2399
Compare
48b2399
to
e97f239
Compare
I have thoroughly updated this PR. Unfortunately, it is hard to find experiences which will work well with it:
These are a couple of experiences which work consistently. A-Painter:
Access Mars:
(remove the |
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.
Looking good, we're superclose to land this. I have some comments
app/src/common/shared/com/igalia/wolvic/browser/engine/SessionUtils.java
Show resolved
Hide resolved
e97f239
to
f963360
Compare
I had to modify Now that These are the new comands to test this feature:
|
app/src/common/shared/com/igalia/wolvic/VersionCheckActivity.java
Outdated
Show resolved
Hide resolved
f963360
to
3a8e714
Compare
This PR implements support for opening immersive experiences directly as soon as the application is launched. We do this by adding a built-in extension which will activate a particular element in the page. We need to set the preference dom.vr.require-gesture to false so this script can launch immersive WebXR experiences. Media autoplay is also allowed in this mode. The information required to identify the element to activate is passed as parameters to the Intent: - launch_immersive - launch_immersive_parent_xpath - launch_immersive_element_xpath - a target URL to open One additional concern is that we should only change the configuration prefs when that is needed for launching Wolvic directly in immersive mode. The problem is that we only write that prefs file the first time that we create the browser engine, which happens several times at launch time **before** we have parsed the incoming Intent and we can know in which mode we are launching. The solution is that WidgetManagerDelegate.isOpenInImmersive() internally will check the current Intent, to cover the case where it is being called too early. We then pass VRBrowserActivity as the context to the objects that may initialize the engine, so eventually SessionUtils.prepareConfigurationPath() is able to decide whether specific prefs should be added to the configuration file. For launching Wolvic directly in immersive mode, these prefs are: dom.vr.require-gesture: false media.autoplay.default: 0 These preferences need to be applied on startup. Therefore, when a running Wolvic receives a new Intent to launch in immersive we will finish the current activity and launch a new one.
3a8e714
to
dbd2a73
Compare
The change which assigned the intent filters to I have rebased this PR and it is once again ready for review. |
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.
OK let's land this
This PR implements support for opening immersive experiences directly as soon as the application is launched.
We do this by adding a built-in extension which will activate a particular element in the page.
We need to set the preference
dom.vr.require-gesture
to false so this script can launch immersive WebXR experiences without user input. Media autoplay also need to be allowed in this mode.The information required to identify the element to activate is passed as parameters to the Intent:
launch_immersive
launch_immersive_parent_xpath
launch_immersive_element_xpath
One additional concern is that we should only change the configuration prefs when that is needed for launching Wolvic directly in immersive mode.
The problem is that we only write that prefs file the first time that we create the browser engine, which happens several times at launch time before we have parsed the incoming Intent and we can know in which mode we are launching.
The solution is that
WidgetManagerDelegate.isOpenInImmersive()
internally will check the current Intent, to cover the case where it is being called too early.We then pass
VRBrowserActivity
as the context to the objects that may initialize the engine, so eventuallySessionUtils.prepareConfigurationPath()
is able to decide whether specific prefs should be added to the configuration file.For launching Wolvic directly in immersive mode, these prefs are: