797272 - [Browser] Close Tab option will not close a crashed page

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Tony Chung [:tchung]]

Attachment: screenshot

The close tab option when there's a content crash doesn't work.  The tab is still open after tapping the option.

Screenshot attached.

Environment:
- daily build, otoro
- Gaia commit: 
- Gecko Commit: 

Logcat: 
10-02 23:24:31.750: E/GeckoConsole(106): [JavaScript Error: "TypeError: this.hideStartScreen is not a function" {file: "app://browser.gaiamobile.org/js/browser.js" line: 214}]

Repro:
1) launch browser
2) visit a webpage that will crash the content (see URL)
3) when the page crashes, click the "Close Tab" button
4) Verify the tab doesnt close.

Expected:
- tab closes when clicked

Actual:
- tab is still open.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

How many tabs did you have open?

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Turns out that if you have multiple tabs, then the close tab will dismiss the error message but does not close the actual tab.  There will still be that many tabs open.  The crash tab will be all white and will show the error message again.

Comment 3

[Ben Francis [:benfrancis]]

Josh/Larissa,

Usually we don't allow users to close the tab if there's only one tab open. Should we not display the "Close Tab" on this error screen in this case? Or should it do something different?

Comment 4

[Ben Francis [:benfrancis]]

Oh, looks like Naoki suggested that we grey out (disable) the button if there's only one tab open. Works for me!

bug 798009

Comment 5

[Larissa Co [:lco]]

(In reply to Ben Francis [:benfrancis] from comment #3)
> Josh/Larissa,
> 
> Usually we don't allow users to close the tab if there's only one tab open.
> Should we not display the "Close Tab" on this error screen in this case? Or
> should it do something different?

This seems to be a different issue from the one Naoki reported (that the tab doesn't actually close). But I do have an opinion on what should happen if there's only one tab and the app crashes. 

Stepping back for a second, I don't know why we don't allow the user to close the tab if there's only one tab. Fennec does this, and I think it's a bit weird, especially if I don't want others to see the one site I was on. If the user closes the single tab, can we instead assume that he wants to close the webpage but not the browser? Thus, closing that last tab will take him back to the Start page. If this is possible, I can write a bug for it.

Taking that assumption into consideration, I think that if we have a crashed tab, and there's only one tab, "closing" that tab would redirect the user to the Start Page.

What do you guys think?

Comment 6

[Ben Francis [:benfrancis]]

(In reply to Larissa Co from comment #5)
> This seems to be a different issue from the one Naoki reported (that the tab
> doesn't actually close).

Yes, I think he's saying it doesn't work *ever*, which we need to fix.
 
> Stepping back for a second, I don't know why we don't allow the user to
> close the tab if there's only one tab. Fennec does this, and I think it's a
> bit weird, especially if I don't want others to see the one site I was on.

I suspect the reason that we do it this way is *because* Fennec does it this way. I assume when Dale implemented it he copied the behaviour of Fennec.

> If the user closes the single tab, can we instead assume that he wants to
> close the webpage but not the browser?

Apps don't generally close themselves in B2G, in fact I'm not even sure if an app *can* close itself, but I can try and find out.

> Thus, closing that last tab will take
> him back to the Start page. If this is possible, I can write a bug for it.

If that's what you think it should do then feel free to file a bug and try to nominate it for blocking-basecamp.

Would you agree that disabling the close button if there's only one tab open would be a good fallback if that doesn't happen?

Comment 7

[Dale Harvey (:daleharvey)]

No browser lets you close the last tab, I think it introduces some fairly confusing the UI, the location bar is tied to the current tab, using it to create new tabs seems to break the mental model (imho)

Comment 8

[Larissa Co [:lco]]

(In reply to Dale Harvey (:dale) from comment #7)
> No browser lets you close the last tab, I think it introduces some fairly
> confusing the UI, the location bar is tied to the current tab, using it to
> create new tabs seems to break the mental model (imho)

For the crashed tab + one tab only case, I don't think this is as big an issue because the crash message hides the URL bar (at least that's what I see so far). Thus, "Close Tab" can successfully take the user to About:home without confusing the user.

For the general case of closing the single tab, I"m not sure I understand what you mean by "using the location bar to create new tabs". You would close it from the Tab list, which means you don't see the location bar.

Maybe I should wireframe this out and see if this makes sense to us.

Comment 9

[Larissa Co [:lco]]

(In reply to Larissa Co from comment #8)
> (In reply to Dale Harvey (:dale) from comment #7)
> > No browser lets you close the last tab, I think it introduces some fairly
> > confusing the UI, the location bar is tied to the current tab, using it to
> > create new tabs seems to break the mental model (imho)
> 
> For the crashed tab + one tab only case, I don't think this is as big an
> issue because the crash message hides the URL bar (at least that's what I
> see so far). Thus, "Close Tab" can successfully take the user to About:home
> without confusing the user.

Eh, I guess I was wrong about this case seeing the screenshot. This might be the case you're concerned about confusing the user. I think I'll wireframe this out.

> 
> For the general case of closing the single tab, I"m not sure I understand
> what you mean by "using the location bar to create new tabs". You would
> close it from the Tab list, which means you don't see the location bar.
> 
> Maybe I should wireframe this out and see if this makes sense to us.

Comment 10

[Larissa Co [:lco]]

Here are a couple of animated prototypes for what how I was thinking of doing this:
 
http://people.mozilla.com/~lco/FX_B2G/Prototypes/closing-last-tab.mov
http://people.mozilla.com/~lco/FX_B2G/Prototypes/crashed-last-tab.mov

Let me know if you have trouble viewing them. .mov was the best I could do :)

Comment 11

[Josh Carpenter [:jcarpenter]]

Those look good, Larissa. I like.

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Actually this bug causes more than just the crash tabs not to crash, it also sometimes causes non crash tabs not to crash if there's a crash tab in their midst.  Should I write a separate bug for that?

Comment 13

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

hrm.  I might have to :

10-17 17:12:44.212: E/GeckoConsole(106): [JavaScript Error: "TypeError: this.tabs[id] is undefined" {file: "app://browser.gaiamobile.org/js/browser.js" line: 1161}]
10-17 17:12:44.882: E/GeckoConsole(106): [JavaScript Error: "TypeError: this.tabs[id] is undefined" {file: "app://browser.gaiamobile.org/js/browser.js" line: 1161}]

looks like I got into a state where the tab isn't found anymore.

Comment 14

[Ben Francis [:benfrancis]]

I'm not quite sure what you mean Naoki, but yes please file a separate bug or at least steps to reproduce those errors

Comment 15

[Ben Francis [:benfrancis]]

OK, having looked at this again I'd like to propose that to fix this particular bug, we make the button close the tab and just disable the close tab button if there's only one tab open. This is the lowest risk approach to fixing this bug.

Feel free to file a second bug (as you suggested) to change the current behaviour and allow closing the last tab, but I think that's a separate issue.

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I'll file a separate bug for the comment 12.

What I was alluding to is that the other way to close a tab is to slide the tab out in the tab view... I'm not sure if that's accounted for a method to close the tab.  It's not just the close button that could be used to close the crashed tab.

Comment 17

[Larissa Co [:lco]]

(In reply to Ben Francis [:benfrancis] from comment #15)
> OK, having looked at this again I'd like to propose that to fix this
> particular bug, we make the button close the tab and just disable the close
> tab button if there's only one tab open. This is the lowest risk approach to
> fixing this bug.

Doing this might get the user stuck in an infinite loop of refreshing and then seeing a crashed page, unless a refresh works 100% of the time. If it does work 100% of the time, then I'm ok with this behavior for v1. If it doesn't, can we replace the "close tab" button with "close browser" when there is only one tab? So that there's at least some way of "resetting" the browser.


> 
> Feel free to file a second bug (as you suggested) to change the current
> behaviour and allow closing the last tab, but I think that's a separate
> issue.

That's fine, I'll file a bug for that, and it can wait till v2 if needed. I did get some good reception from the Android team about the ability to close the last tab, so we're likely to try and implement it there too.

Comment 18

[Ben Francis [:benfrancis]]

(In reply to Larissa Co from comment #17)
> Doing this might get the user stuck in an infinite loop of refreshing and
> then seeing a crashed page, unless a refresh works 100% of the time. If it
> does work 100% of the time, then I'm ok with this behavior for v1. If it
> doesn't, can we replace the "close tab" button with "close browser" when
> there is only one tab? So that there's at least some way of "resetting" the
> browser.

The infinite loop you describe seems like the correct behaviour if the tab is repeatedly crashing. But the user isn't stuck, they can enter a new URL into the URL bar or create a new tab.

I don't know if it's possible for an app to close itself, no other app in Gaia does that and I thought we were to avoid the idea that users should manually close apps in general.

Thanks for filing a separate bug for the other part.

Comment 19

[Larissa Co [:lco]]

> The infinite loop you describe seems like the correct behaviour if the tab
> is repeatedly crashing. But the user isn't stuck, they can enter a new URL
> into the URL bar or create a new tab.

Ok, that's good enough for me right now then. Just to be clear, I think we should remove the "close tab" button, not have it disabled in this case.


> I don't know if it's possible for an app to close itself, no other app in
> Gaia does that and I thought we were to avoid the idea that users should
> manually close apps in general.
> 
> Thanks for filing a separate bug for the other part.

Filed! bug 803223

Comment 20

[Ben Francis [:benfrancis]]

(In reply to Larissa Co from comment #19)
> Just to be clear, I think we
> should remove the "close tab" button, not have it disabled in this case.

Heh, you would now that I've already submitted a patch :P Let me update it...

Comment 21

[Larissa Co [:lco]]

(In reply to Ben Francis [:benfrancis] from comment #20)
> (In reply to Larissa Co from comment #19)
> > Just to be clear, I think we
> > should remove the "close tab" button, not have it disabled in this case.
> 
> Heh, you would now that I've already submitted a patch :P Let me update it...

Sorry! Stop being so fast :)

Comment 22

[Ben Francis [:benfrancis]]

Done.

Comment 23

[Darren]

Verified fixed on Unagi build 201212310070201.