942325 - [offline] iframes in packaged apps display offline screen, "Close" button doesn't work

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

In the new offline error screen landed in bug 882186, clicking the close button will not do anything.

Comment 1

[Jason Smith [:jsmith]]

Technically a regression because this worked on the old offline error page.

Comment 2

[Michael Henretty [:mikehenrty][:mhenretty]]

The problem is that the marketplace is a packaged app that opens an iframe with the src pointing to the marketplace url, and nsGlobalWindow prevents window.close() from being called on certain iframes (http://hg.mozilla.org/mozilla-central/file/9b9796428162/dom/base/nsGlobalWindow.cpp#l7952). We can work around this by making an exception for about:neterror pages again, but there are bigger problems here:

If a packaged app opens an iframe to a remote url that is offline, should we display this offline error screen inside the iframe? My immediate thought is no, for a couple reasons. We don't know the size/shape of this iframe, and this screen might look weird in the middle of a packaged app. Also the app developer probably wouldn't want to give this iframe the ability to close itself based on the about:neterror page (could have side effects with their app's logic if they have references to this frame lying around).

My thinking is that we only want to display this offline screen for top level mozbrowser frames. Conversely, we could display a more simplified offline screen for inner iframes (ie. not have the close button). Or, we could just add an exception to nsGlobalWindow to allow iframes to close themselves if they are in an about:neterror state, but again I don't like this way.

Fabrice or Vivien, can you provide some advice on what we should do here?

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

I think we are going to need some input from UX on this one. Peter, Francis, please see above and weigh in on what we should do with offline iframes in packaged apps.

Comment 4

[Jason Smith [:jsmith]]

I think there might be a second bug here that needs to be filed - there shouldn't even be a generic offline error showing up in marketplace, as marketplace has it's own customized offline error page. I know this offline error page marketplace had worked fine on 1.01, 1.1, & 1.2, which leads me to believe that this is another regression here.

Mike - What do you think? I'll file a bug if you are in agreement.

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

The initial spec for the offline error bug 882186 has the marketplace also using this new offline error screen (see page 7 https://bug882186.bugzilla.mozilla.org/attachment.cgi?id=794281). This screen is meant to show up here.

Comment 6

[[:fabrice] Fabrice Desré]

I agree we should not even try to close sub-frames. Displaying a nice message looks right though. I defer to UX for the details.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Michael Henretty [:mhenretty] from comment #5)
> The initial spec for the offline error bug 882186 has the marketplace also
> using this new offline error screen (see page 7
> https://bug882186.bugzilla.mozilla.org/attachment.cgi?id=794281). This
> screen is meant to show up here.

The spec is incorrect here then. It does not make sense for the system to always override the offline experience if an app decides it wants to have it's own offline error page. This is a firm content management requirement. I'm opening a bug for this & sending it over to UX.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Michael Henretty [:mhenretty] from comment #5)
> > The initial spec for the offline error bug 882186 has the marketplace also
> > using this new offline error screen (see page 7
> > https://bug882186.bugzilla.mozilla.org/attachment.cgi?id=794281). This
> > screen is meant to show up here.
> 
> The spec is incorrect here then. It does not make sense for the system to
> always override the offline experience if an app decides it wants to have
> it's own offline error page. This is a firm content management requirement.
> I'm opening a bug for this & sending it over to UX.

Hmm...apparently this actually works as expected in marketplace.

I also can't reproduce this bug on today's trunk build.

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

Changing the name here, since this applies to any packaged app with a remote iframe and not just marketplace.

Comment 10

[Francis Djabri [:djabber]]

Attachment: offline 1.2 MVP v0.4.pdf

Updated the spec to handle this use case. The original Offline Error Connectivity Action menu outlined in the spec was not intended to be shown in an iFrame within an app, but instead it should be shown when launching applications that do not run offline, or for top level page loads within an app. The Connectivity action menu would not be suitable to show within an iFrame because it's designed to be a full-screen overlay. An error message within an iFrame should have a different visual style, and I agree with Michael that it shouldn't contain buttons, especially as we don't know what size the iFrame will be. 

I would strongly encourage apps to cover error handling within an iFrame themselves, as only they know the size of the iFrame, and the app can design the error handling to fit in with the general flow and branding of the app. However, if we need to provide a fall-back solution, it is outlined in the updated spec. 

Flagging Patryk to advise on the visual style of the iFrame connectivity error.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Glad that they are a new UX spec for this. Sounds like the needinfo? for me is not useful anymore. \o/

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: PR, remove buttons for iframes

WIP patch, still need the final design, but this at least has the functionality we need.

Comment 13

[Amy Lee [:amylee] UX]

Attachment: iFrame_assets.zip

Hi,

Attached is the design spec for the iFrame unable to connect screen. Also attached is the reload icon for use @1 and @1.5 scale.

Comment 14

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8341868 [details] [review]
PR, remove buttons for iframes

Update style of framed net_error page based on spec. Added integration test.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8341868 [details] [review]
PR, remove buttons for iframes

I don't like too much the fact that each functions are beast with 2 code paths. I would have rather prefer to have one function per case but it may be too much of an overhead so I don't feel strongly about it.

Comment 16

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8341868 [details] [review]
PR, remove buttons for iframes

Updated PR based on Vivien's comments. Will merge once we have a green travis.

Comment 17

[Gregor Wagner [:gwagner]]

https://github.com/mozilla-b2g/gaia/pull/14325

Comment 18

[Salem Elkabule]

This bug No longer reproduces on buri 1.3 Moz Ril

Environmental Variables
Device: buri 1.3 Moz Ril
Build ID: 20140102004001
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/61f553e5db49
Gaia: 01e9da49be2cc4bc134eeefc434740d572ec2246
Platform Version: 28.0a2

Comment 19

[Jason Smith [:jsmith]]

This hasn't been uplifted to 1.3 yet, so the testing in comment 18 didn't verify this bug.

Comment 20

[Michael Henretty [:mikehenrty][:mhenretty]]

Note: to verify this, we need to make sure we follow page 10 of the updated spec Francis uploaded. Mainly, the close button no longer appears, and tapping anywhere in the iframe causes the iframe to refresh.

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted fd9a5d36581aa4fcfd4b121cd1cd2187496ce222 to:
v1.3 already had this commit

Comment 22

[Jason Smith [:jsmith]]

Verified on 1.3 1/26/2014 build.