1533058 - "WebDriver:NewWindow" should open a window with "about:blank", but not the new tab page

Status: ASSIGNED

(Testing :: Marionette, defect, P1)

Description

[James Graham [:jgraham]]

https://w3c.github.io/webdriver/#new-window is pretty specific that the new window should be about:blank and instead opening with the new tab page is obsrvably different from this.

Comment 1

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

So we disable the browser.newtabpage.enabled preference for profiles as generated with Marionette or geckodriver, and which doesn't seem to be set when wptrunner creates the profile.

It means that we should hard-code opening the window as about:blank, and not relying on the newtabpage preference value.

Problem is that we are using OpenBrowserWindow(), and it doesn't seem to support specifying a URL to load.

Gijs, do you know of a way to force that method to open a new browser window displaying about:blank?

Comment 2

[:Gijs (he/him)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #1)

So we disable the browser.newtabpage.enabled preference for profiles as generated with Marionette or geckodriver, and which doesn't seem to be set when wptrunner creates the profile.

This seems fixable. Why can't we just set the pref for the wptrunner profile?

Gijs, do you know of a way to force that method to open a new browser window displaying about:blank?

Calling the relevant internal APIs by hand, which I don't want us to do because we should be reducing the number of ways we have of opening windows, and making their APIs sane, not adding more consumers of the internal window watcher APIs or similar (which will make any refactoring even harder than it already is).

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Opening a new window opens your homepage, not the new tab page, although they can be set to be the same thing. I think you'd want to override the homepage pref like this https://hg.mozilla.org/try/rev/fe589126355655da96dc13cf47ec23095b2d191b

Comment 4

[Andreas Tolfsen 「:ato」]

(Sorry, this was out of context.)

Comment 5

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

So I also noticed this as problem on Fennec while investigating bug 1536583. Reason is that Fennec doesn't obey browser.newtabpage.enabled and as such displays this page after opening a new tab. I can actually pretty easily fix that by passing about:blank into the browserApp.addTab() method.

For Firefox this is more complicated. As mentioned above whether OpenBrowserWindow() nor BrowserOpenTab() allow me to pass in a URL. Which means that tests for a different harness (like web-platform-tests) will fail, when browser.newtabpage.enabled = false isn't set.

(In reply to :Gijs (he/him) from comment #2)

So we disable the browser.newtabpage.enabled preference for profiles as generated with Marionette or geckodriver, and which doesn't seem to be set when wptrunner creates the profile.

This seems fixable. Why can't we just set the pref for the wptrunner profile?

James doesn't really want that. So maybe he can explain.

Calling the relevant internal APIs by hand, which I don't want us to do because we should be reducing the number of ways we have of opening windows, and making their APIs sane, not adding more consumers of the internal window watcher APIs or similar (which will make any refactoring even harder than it already is).

Having another look over those methods I can understand that. The target URL is kinda nested deep in sub methods. Also all the extra features eg. private browsing is not something I'm particularly interested in replicating, and as such say would need to be updated all the time when something changes in the browser too.

While I see it isn't elegant to rely on the preference while the WebDriver spec says to hard-code about:blank, each browser vendor has to find the optimal way to get that working. And hard-coding seems to be nearly impossible right now for an ideal solution.

Comment 6

[James Graham [:jgraham]]

This seems fixable. Why can't we just set the pref for the wptrunner profile?
James doesn't really want that. So maybe he can explain.

Only because the WebDriver spec requires that we get about:blank in new windows for any marionette session, so fixing this at the wptrunner level is wrong.

I don't mind if we rely on a pref, but marionette needs to ensure it's correctly set.

Comment 7

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Please note that web-platform-tests don't use the preferences as set by marionette in geckoinstance.py, nor the recommended preferences when marionette.prefs.recommended is set to True. Instead the profile is created on its own by collecting the various preferences under testing/profiles. Note that Marionette doesn't use those yet.

I assume that you don't want that the recommended preferences are getting set by Marionette. See the list of prefs here:

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/components/marionette.js#62

As such I can only see that we add browser.newtabpage.enabled = false to testing/profiles/web-platform/user.js.

Comment 8

[James Graham [:jgraham]]

Well we could also use the prefs service to ensure the pref is set before trying to open the new window.

Comment 9

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Oh! That is actually a perfect solution. I totally missed that. So yes, that sounds like the proper solution here.

Comment 10

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0054681c542c7f21e571aafb674f4e82f536b96

James, would you mind trying https://hg.mozilla.org/try/rev/a9f2bf3bdb792b0c4b3e6d1bf7ea7b6544aa0c91 if that fixes the problem for you? Thanks!

Comment 11

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Attachment: Bug 1533058 - [marionette] Force opening "about:blank" for "WebDriver:NewWindow" in Fennec. r=#webdriver

Comment 12

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Attachment: Bug 1533058 - [marionette] Force opening "about:blank" for "WebDriver:NewWindow" in Firefox. r=#webdriver

Depends on D34831

Comment 13

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Moving the needinfo request to the revision in Phabricator itself.

Comment 14

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Note, the patch for Fennec will be moved out to bug 1506782.

Comment 15

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

This covers desktop only now. So moving the whiteboard entry also to bug 1506782.

Comment 16

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

Seeing how we make progress on bug 1506782 I wonder if the proposed solution for Firefox is even correct. It would also be affected by preference name changes.

What if we simply wait for the initial page load (whatever that loads) in a new tab and window, and then do another navigation to "about:blank". Especially for mobile applications based on GeckoView this seems to be the only available option.

James and Andreas, what do you think?

Comment 17

[James Graham [:jgraham]]

Preference name changes should presumably be handled by the person renaming preferences grepping the source and replacing all occurences of the pref name. I suppose there's a problem that we want stronger compatibility guarantees than the frontend code, but I wonder how often we expect prefs to change. By contrast having multiple loads in the window seems like a constant performance hit and adds additional complexity that other code may need to account for.

Comment 18

[Andreas Tolfsen 「:ato」]

There is a lot of context here that I may not have amassed, but if
WPT is explicitly disabling the recommended preferences, it needs
to bear the responsibility of that by setting the necessary preferences
in the profile it prepares.

Is there a particular reason why WPT does not want the recommended
preferences?

We do have the distinction between recommended and required
preferences in Marionette, but the latter is typically reserved
only for the preferences needed for the Marionette server to operate
correctly, and not for enforcing WebDriver conformance.

I also agree that a second navigation to about:blank is undesirable.
Am I missing the point, or could WPT not just set
browser.newtabpage.enabled or adopt marionette.prefs.recommended?

Comment 19

[James Graham [:jgraham]]

Of course we can set that pref, but it feels like the WebDriver implementation should follow the spec by opening about:blank for new tabs even if you've set some other site as the homepage for wahtever reason.

Comment 20

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

So what about a compromise here. Given that it is a feature highly needed for getting the New Window command used in WPT we set the preference for now in the profile. Then we can file a new bug to get a sane implementation without using this and probably other necessary preferences. As we know it's still more complicated for Fennec or mobile in general. See bug 1506782.

Comment 21

[Andreas Tolfsen 「:ato」]

(In reply to James Graham [:jgraham] from comment #19)

Of course we can set that pref, but it feels like the WebDriver
implementation should follow the spec by opening about:blank for
new tabs even if you've set some other site as the homepage for
wahtever reason.

I see, so the distinction is that if there’s a user pref overriding
the recommended new tab page, that URL is opened instead on new
tabs.

The recommended preferences in Marionette deliberately does not
overrule user prefs, except for marionette.port which is required
for the server to function correctly. This behaviour means the users
can—at their own risk—override prefs necessary for WebDriver
conformance. This behaviour is transparent, because whatever pref
the user writes to the profile ends up taking effect. There’s no
misunderstanding why a set pref ends up not being respected.

If we were to change the recommended preferences to in essence act
as required, or over overriding preferences, we lose that transparency.
Suddenly you need the prerequisite knowledge to first disable
marionette.prefs.recommended in order for prefs set in user.js
to take effect. I think this is unfortunate because it encodes
behaviour that is not obvious from just reading user.js.

For the record I think the hardcoded option to enforcing about:blank
by setting the pref and resetting to its original value before the
opening a tab is worse in this regard, because then it’s not obvious
from either reading prefs.js or about:config what rules are in
effect.

I’m just trying to explain the reasoning behind the current model.
I’m open to arguments to the contrary!

All that said, I couldn’t find the instance where WPT explicitly
turns off marionette.prefs.recommended (the default is true),
which means it must some place set browser.newtabpage.enabled to
true for it to see this behaviour, right?

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #20)

As we know it's still more complicated for Fennec or mobile in
general. See bug 1506782.

Judging by the look of it, using an intent argument will cause the
first run pane to disappear:
https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/firstrun/OnboardingHelper.java#89

I guess there’s no real concept of a new tab page in GeckoView? At
least getting rid of an onboarding pane seems sensible.

Comment 22

[Henrik Skupin (:whimboo) [⌚️UTC+2]]

I talked to Gijs yesterday, and he is fine for having an additional argument for openBrowserWindow() and openBrowserTab() which allows consumers to override the initial URL. As he mentioned too for the latter method it should already be possible. So I will have to check that the next days, and will provide a patch for Firefox which adds it to openBrowserWindow().

Please lets not continue to discuss the topic about mobile on this bug but use bug 1506782 for that.