STR: - open a new tab - load a page

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.

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

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

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

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

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

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.

See comment 3.

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

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

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

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

> 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?

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.

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

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

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

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

> 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