Closed Bug 1374768 Opened 7 years ago Closed 7 years ago

Location bar not focused immediately with remote/content about:newtab page

Categories

(Firefox :: Tabbed Browser, enhancement)

Product:
Firefox
Component:
Tabbed Browser
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(1 file)

Bug 1374768 - Location bar not focused immediately with remote/content about:newtab page.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
The change from bug 1367596 caused browser_bug455852.js and browser_bug520538.js tests to fail with activity stream enabled. See https://github.com/mozilla/activity-stream/issues/2698
The failing test when activity stream is enabled:
TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug455852.js | location bar is focused for the new tab - Got [object XULElement], expected [object HTMLInputElement] 

Where the thing being focused is the xul:browser
Comment on attachment 8879686 [details]
Bug 1374768 - Location bar not focused immediately with remote/content about:newtab page.

https://reviewboard.mozilla.org/r/151030/#review156262

::: browser/base/content/tabbrowser.xml:4180
(Diff revision 1)
>                  // 2. The tab has never presented, and has not finished loading
> -                //    a non-blank page.
> +                //    a non-about: page.
>                  //
> -                // For (2), "finished loading a non-blank page" is determined by
> +                // For (2), "finished loading a non-about: page" is determined by
>                  // looking at the loaded URI and the busy state on the tab element.
> +                // We assume about: pages are local and do not show progress.

We don't show progress only for about: URIs pointing to local resources: http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/browser/base/content/tabbrowser.xml#573
Nod. mconley and I discussed that, and we originally wanted to try to refactor shouldShowProgress and spinner usage to share the about/jar+file logic, but that requires the request instead of just a URI. So I ended up just adding a comment about assuming local about:.

Is there a good way to get at URI + originalURI from the requestedBrowser? I seem to recall some casting to/from the channel?
(In reply to Ed Lee :Mardak from comment #4)
> Nod. mconley and I discussed that, and we originally wanted to try to
> refactor shouldShowProgress and spinner usage to share the about/jar+file
> logic, but that requires the request instead of just a URI. So I ended up
> just adding a comment about assuming local about:.

Right, but the comment doesn't make the code correct, it just reiterates the wrong assumption. What would make more sense is if the comment acknowledged that the code is wrong and explained why that's acceptable here.

> Is there a good way to get at URI + originalURI from the requestedBrowser? I
> seem to recall some casting to/from the channel?

Yeah, I think there might be a way, but I don't know it off-hand.
So, given an about: URI, we can resolve what the underlying URI will be:

NetUtil.newChannel({uri: NetUtil.newURI("about:newtab"), loadUsingSystemPrincipal: true}).URI.schemeIs("jar")

So we should be able to pass in the URI and resolvedURI to a shared helper function for local-about detection, and that would be okay?
(In reply to Ed Lee :Mardak from comment #6)
> So we should be able to pass in the URI and resolvedURI to a shared helper
> function for local-about detection, and that would be okay?

That seems like a fine workaround to me, assuming we also check for the file:// case.
Although be advised that (I believe) NetUtil usage is being discouraged since most of its functions can be performed directly via Services.io.
Comment on attachment 8879686 [details]
Bug 1374768 - Location bar not focused immediately with remote/content about:newtab page.

https://reviewboard.mozilla.org/r/151030/#review159344

Thanks Mardak! See below.

::: browser/base/content/tabbrowser.xml:4183
(Diff revision 1)
> -                // For (2), "finished loading a non-blank page" is determined by
> +                // For (2), "finished loading a non-about: page" is determined by
>                  // looking at the loaded URI and the busy state on the tab element.
> +                // We assume about: pages are local and do not show progress.
>                  let hasSufficientlyLoaded =
>                    !this.requestedTab.hasAttribute("busy") &&
> -                  requestedBrowser.currentURI.spec != "about:blank";
> +                  !requestedBrowser.currentURI.schemeIs("about");

Let's use the technique you described above to check for local resource loads, and make sure we check for the file: scheme as well.
Attachment #8879686 - Flags: review?(mconley) → review-
Comment on attachment 8879686 [details]
Bug 1374768 - Location bar not focused immediately with remote/content about:newtab page.

https://reviewboard.mozilla.org/r/151030/#review159344

> Let's use the technique you described above to check for local resource loads, and make sure we check for the file: scheme as well.

Moved logic to `tabbrowser._isLocalAboutURI` and added unit tests for that method.
Comment on attachment 8879686 [details]
Bug 1374768 - Location bar not focused immediately with remote/content about:newtab page.

https://reviewboard.mozilla.org/r/151030/#review159650

::: browser/base/content/tabbrowser.xml:541
(Diff revisions 1 - 2)
> +            null, // loadingNode
> +            Services.scriptSecurityManager.getSystemPrincipal(), // loadingPrincipal
> +            null, // triggeringPrincipal
> +            Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, // securityFlags
> +            Ci.nsIContentPolicy.TYPE_OTHER // contentPolicyType
> +          ).URI;

This is basically inlining and constant-propagating the logic from `NetUtil.newChannel` https://dxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#249-363
I just realized that about:credits nowadays does a full-fledged redirect to https://www.mozilla.org/credits/ rather than masking that URL. So I guess we might not need to check for remote about: URIs anymore?
Comment on attachment 8879686 [details]
Bug 1374768 - Location bar not focused immediately with remote/content about:newtab page.

https://reviewboard.mozilla.org/r/151030/#review160058

This looks good to me!

In the interests of getting Activity Stream preffed on, let's land this. I'll point out, however, that it's possible that bug 1378203 will render this change obsolete down the road (but I still need to get review on it, run it through automation, etc).

Thanks for the patch!

::: browser/base/content/test/tabs/browser_isLocalAboutURI.js:5
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +

Can you please add a quick description of what this test is testing?
Attachment #8879686 - Flags: review?(mconley) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8c55a7d1e55c
Location bar not focused immediately with remote/content about:newtab page. r=mconley
https://hg.mozilla.org/mozilla-central/rev/8c55a7d1e55c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(In reply to Dão Gottwald [::dao] from comment #13)
> I just realized that about:credits nowadays does a full-fledged redirect to
> https://www.mozilla.org/credits/ rather than masking that URL. So I guess we
> might not need to check for remote about: URIs anymore?
Flags: needinfo?(mconley)
Flags: needinfo?(edilee)
I just tested with modifying browser/components/about/AboutRedirector.cpp to point pages to a remote page, and the location bar does seem to behave the same way as the about:credits entry in docshell/base/nsAboutRedirector.cpp.

The request/channel's .originalURI is still the about: URI and .URI is the remote page. So the new _isLocalAboutURI is indeed getting called with about:credits when loading.

For the spinner, turns out _isLocalAboutURI isn't called for about:credits because it's not a remote page anyway. But if you directly load https://www.mozilla.org/credits/, _isLocalAboutURI is indeed called and returns false.
Flags: needinfo?(edilee)
(In reply to Dão Gottwald [::dao] from comment #18)
> (In reply to Dão Gottwald [::dao] from comment #13)
> > I just realized that about:credits nowadays does a full-fledged redirect to
> > https://www.mozilla.org/credits/ rather than masking that URL. So I guess we
> > might not need to check for remote about: URIs anymore?

That sounds probable. I just did a fly-over on our redirector modules, and I can't see any other places where we redirect to remote content.
Flags: needinfo?(mconley)
Depends on: 1384168
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: