Open Bug 1533058 Opened 9 months ago Updated 6 months ago

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

Categories

(Testing :: Marionette, defect, P1)

Version 3
defect

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: jgraham, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.

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?

Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P3
Summary: WebDriver new window command should open a window with about:blank, not the new tab page → "WebDriver:NewWindow" should open a window with "about:blank", but not the new tab page

(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).

Flags: needinfo?(gijskruitbosch+bugs)

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

(Sorry, this was out of context.)

Whiteboard: [geckoview]
OS: Unspecified → Android
Whiteboard: [geckoview] → [geckoview:p3]
Blocks: 1536583

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.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Flags: needinfo?(james)
Priority: P3 → P1

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.

Flags: needinfo?(james)

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.

Flags: needinfo?(james)

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

Flags: needinfo?(james)

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

Attachment #9071821 - Attachment description: Bug 1533058 - [marionette] Force opening "about:blank" for new tabs and windows in Firefox. r=#webdriver → Bug 1533058 - [marionette] Force opening "about:blank" for "WebDriver:NewWindow" in Firefox. r=#webdriver
Attachment #9071820 - Attachment description: Bug 1533058 - [marionette] Force opening "about:blank" for new tabs in Fennec. r=#webdriver → Bug 1533058 - [marionette] Force opening "about:blank" for "WebDriver:NewWindow" in Fennec. r=#webdriver

Moving the needinfo request to the revision in Phabricator itself.

Flags: needinfo?(james)
Attachment #9071820 - Attachment description: Bug 1533058 - [marionette] Force opening "about:blank" for "WebDriver:NewWindow" in Fennec. r=#webdriver → Bug 1533058 - [marionette] Force opening "about:blank" for new tabs in Fennec. r=#webdriver
Attachment #9071821 - Attachment description: Bug 1533058 - [marionette] Force opening "about:blank" for "WebDriver:NewWindow" in Firefox. r=#webdriver → Bug 1533058 - [marionette] Force opening "about:blank" for new tabs and windows in Firefox. r=#webdriver
Attachment #9071820 - Attachment description: Bug 1533058 - [marionette] Force opening "about:blank" for new tabs in Fennec. r=#webdriver → Bug 1533058 - [marionette] Force opening "about:blank" for "WebDriver:NewWindow" in Fennec. r=#webdriver
Attachment #9071821 - Attachment description: Bug 1533058 - [marionette] Force opening "about:blank" for new tabs and windows in Firefox. r=#webdriver → Bug 1533058 - [marionette] Force opening "about:blank" for "WebDriver:NewWindow" in Firefox. r=#webdriver

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

Attachment #9071820 - Attachment is obsolete: true

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

OS: Android → Unspecified
Whiteboard: [geckoview:p3]
No longer blocks: 1536583

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?

Flags: needinfo?(james)
Flags: needinfo?(ato)

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.

Flags: needinfo?(james)

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?

Flags: needinfo?(ato)
Flags: needinfo?(james)

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.

Flags: needinfo?(james)

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.

See Also: → 1506782

(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.

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.

You need to log in before you can comment on or make changes to this bug.