Loading a page in a new tab enables the back button

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: dao, Assigned: ttaubert)

Tracking

({regression})

Trunk
Firefox 16
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 wontfix, firefox14 wontfix, firefox15 verified, firefox-esr10 unaffected)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Keywords: uiwanted
(Reporter)

Comment 2

6 years ago
(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.
(Reporter)

Comment 3

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

Comment 4

6 years ago
I also think that about: newtab should be processed as blank tab
(Reporter)

Comment 5

6 years ago
Created attachment 595746 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #595746 - Flags: review?(gavin.sharp)
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)
Attachment #595746 - Flags: review?(gavin.sharp) → review-
(Reporter)

Updated

6 years ago
Assignee: dao → nobody
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
(Reporter)

Comment 9

6 years ago
See comment 3.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Depends on: 729063
Duplicate of this bug: 751107

Comment 11

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

Comment 12

5 years ago
(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.
Blocks: 769108
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).
Assignee: nobody → ttaubert
Created attachment 638535 [details] [diff] [review]
do not add about:newtab to the session history
Attachment #595746 - Attachment is obsolete: true
Attachment #638535 - Flags: review?(gavin.sharp)
Keywords: uiwanted
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?
Attachment #638535 - Flags: review?(gavin.sharp)
Attachment #638535 - Flags: review?(bzbarsky)
Attachment #638535 - Flags: feedback+
Created attachment 638676 [details] [diff] [review]
test to make sure about:newtab is not added to the session history
Attachment #638676 - Flags: review?(gavin.sharp)
Attachment #638676 - Flags: review?(gavin.sharp) → review+
status-firefox-esr10: --- → unaffected
status-firefox13: --- → wontfix
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
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.
Attachment #638535 - Flags: review?(bzbarsky) → review-
(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.
Created attachment 640543 [details] [diff] [review]
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.
Attachment #638535 - Attachment is obsolete: true
Attachment #640543 - Flags: review?(gavin.sharp)
Attachment #640543 - Flags: review?(bzbarsky)
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.)
Attachment #640543 - Flags: review?(gavin.sharp) → feedback+
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.
Attachment #640543 - Flags: review?(bzbarsky) → review+
(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
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
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.
Attachment #640543 - Flags: approval-mozilla-beta?
Attachment #640543 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c9c8ecd605b5
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]

Updated

5 years ago
status-firefox16: affected → ---
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.
Attachment #640543 - Flags: approval-mozilla-beta?
Attachment #640543 - Flags: approval-mozilla-beta-
Attachment #640543 - Flags: approval-mozilla-aurora?
Attachment #640543 - Flags: approval-mozilla-aurora+

Updated

5 years ago
status-firefox14: affected → wontfix
https://hg.mozilla.org/releases/mozilla-aurora/rev/16379f3e5b13
status-firefox15: affected → fixed

Updated

5 years ago
Blocks: 776167

Comment 29

5 years ago
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
status-firefox15: fixed → verified
Duplicate of this bug: 777782

Updated

4 years ago
Blocks: 860084

Comment 31

4 years ago
> 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
Depends on: 1077738

Updated

3 years ago
Depends on: 1084471
You need to log in before you can comment on or make changes to this bug.