724239 - Loading a page in a new tab enables the back button

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

STR:
- open a new tab
- load a page

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

That's not a regression but rather behavior worth talking about. If you look at Chrome, they do the same as we currently do. So we should figure out which behavior we think is best.

I actually think that it provides benefit to users that use the back button to go back to the new tab page and visit a different site from there again.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> I actually think that it provides benefit to users that use the back button
> to go back to the new tab page and visit a different site from there again.

I don't really think this makes much sense. Accel+t and the new tab button are the obvious ways to get that page. Being able to go back is especially unexpected if you consider that the loaded page may be opened from the location bar, a bookmark, etc., so the new tab page may not exist as a step at all in the users mind. This is exactly what happened to me and made me believe I accidentally replaced some page rather than opening a new tab.

Comment 3

[Dão Gottwald [:dao]]

And of course this also happens with about:newtab's grid disabled.

Comment 4

[Tiger.711]

I also think that about: newtab should be processed as blank tab

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Can we instead just change nsDocShell::ShouldAddToSessionHistory to return false for about:newtab, in addition to about:blank?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(or maybe have it check an app-specific pref or something)

Comment 8

[Alex Limi (:limi) — Firefox UX Team]

Back button should work to get back to the starting point.

A very obvious case is if you click the wrong thumbnail, and end up on the wrong site. You shouldn't have to close the tab, open a new tab, and click again. Back should always work when there's content there.

There's also more links and content coming to the new tab page, so the current behavior is correct, and should be kept.

Comment 9

[Dão Gottwald [:dao]]

See comment 3.

Comment 11

[Willy_ Foo_Foo]

when using small icons(theme) the back button should be hidden too

Comment 12

[Sam]

(In reply to Alex Limi (:limi) — Firefox UX Team from comment #8)
> Back button should work to get back to the starting point.
> 
> A very obvious case is if you click the wrong thumbnail, and end up on the
> wrong site. You shouldn't have to close the tab, open a new tab, and click
> again. Back should always work when there's content there.
> 
> There's also more links and content coming to the new tab page, so the
> current behavior is correct, and should be kept.

when you enter your own URL you still have NTP in the tab history/back button/ which - by your own logic shouldn't be - as I didn't use NTP's tiles/contents to navigate to my target page.

Right now users have to tweak about:config stettings to (partially) fix this broken/lacking tabflow.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

The "click wrong thumbnail, need to try again" scenario from comment 8 is compelling, but I'm not sure how it weighs against the scenarios described in comment 2.

UX considerations aside, there are security implications to allowing a web site to trigger the loading of a chrome-privileged page, so until we can make about:newtab not be privileged, I think we're going to need to fix this (per comment 6).

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: do not add about:newtab to the session history

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 638535 [details] [diff] [review]
do not add about:newtab to the session history

Looks good to me, but bz should r+. Can you add a test that checks that the back button is appropriately disabled when navigating away?

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: test to make sure about:newtab is not added to the session history

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 638535 [details] [diff] [review]
do not add about:newtab to the session history

I'm not happy with this hardcoding, because apps other than Firefox might use other things than about:newtab.  And even for Firefox this is pref-controllable.

Should this instead be testing for the initial tab URI, whatever that is?  If we don't want it for random values of that pref, then we should add an API for docshell to ask the UI whether the given URI should be added.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #17)
> Should this instead be testing for the initial tab URI, whatever that is? 
> If we don't want it for random values of that pref, then we should add an
> API for docshell to ask the UI whether the given URI should be added.

I don't think we want this to kick in for arbitrary values of that pref, since someone might want a normal website URL to be their new tab URL, and doing that shouldn't affect session history for other loads of that URL.

Adding an API kind of seems like overkill. The odds of someone using about:newtab for something (and additionally also caring about the session history behavior for that URL) seem rather low - can you reconsider just taking the simple hardcoding patch?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> The odds of someone using about:newtab for something

I'm more worried about a different embedder using some other URI for the default page.  about:newtab is part of Firefox, not toolkit or Gecko....

Would it make sense to compare to the value of the pref on the default branch?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

I'm not sure that saying "browser.newtab.url is the default pref you need to set for docshell to be aware of your new tab page" is much better than saying "about:newtab is the URI you must use for docshell to be aware of your new tab page", from an embedder point of view, but I guess it is more flexible. Let's just do that then.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: do not add about:newtab (or any custom newtab url) to the session history

The patch does now read the value of 'browser.newtab.url' and prevents matching pages from getting added to the session history.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

You'll need to use GetDefaultCString per comment 18/comment 19, but otherwise looks good to me.

(I don't know whether we need to worry about the performance implications of calling GetSpec here for large (e.g. data:) URIs.)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

Should failure rv return true instead?

With that and the GetDefaultCString, should be ok.

And yes, this kinda sucks for data: URIs...  I don't think the suck is too terrible, though.  This is not hot code.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #23)
> Should failure rv return true instead?

Done.

> With that and the GetDefaultCString, should be ok.

Done.

Pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/c9c8ecd605b5

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New Tab Page
User impact if declined: security implications, possible privilege escalation
Risk to taking this patch (and alternatives if risky): risk is very low
String or UUID changes made by this patch: None.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c9c8ecd605b5

Comment 27

[Alex Keybl [:akeybl]]

Comment on attachment 640543 [details] [diff] [review]
do not add about:newtab (or any custom newtab url) to the session history

[Triage Comment]
This missed our last planned Beta 14 build, so only approving for Aurora 15.

Including Dan and Al as well in case they want to protest based upon security implications.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/16379f3e5b13

Comment 29

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0
Build identifier: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0

Comment 31

[Sid]

> do not add about:newtab (or any custom newtab url) to the session history

I don't see it on my Nightly with custom newtab url. Here are STR.
1. Set browser.newtab.url = http://google.com/
2. Open new tab
3. Go to some other page
4. Back button is enabled