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

RESOLVED FIXED in 1.3 C1/1.4 S1(20dec)

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mikehenrty, Assigned: mikehenrty)

Tracking

({regression})

unspecified
1.3 C1/1.4 S1(20dec)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, b2g-v1.3 verified)

Details

(Whiteboard: [systemsfe])

Attachments

(3 attachments)

In the new offline error screen landed in bug 882186, clicking the close button will not do anything.
Blocks: 930630
blocking-b2g: --- → 1.3?
Technically a regression because this worked on the old offline error page.
Keywords: regression
Component: Gaia::System → Gaia::System::Window Mgmt
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?
Flags: needinfo?(fabrice)
Flags: needinfo?(21)
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.
Flags: needinfo?(pdolanjski)
Flags: needinfo?(fdjabri)
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.
Flags: needinfo?(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.
Flags: needinfo?(mhenretty)
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.
Flags: needinfo?(fabrice)
(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.
(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.
blocking-b2g: 1.3? → 1.3+
Changing the name here, since this applies to any packaged app with a remote iframe and not just marketplace.
Summary: [Marketplace] When offline, "Close" button does not actually close the app. → [offline] iframes in packaged apps display offline screen, "Close" button doesn't work
Target Milestone: --- → 1.3 Sprint 6 - 12/6
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.
Flags: needinfo?(fdjabri) → needinfo?(padamczyk)
Glad that they are a new UX spec for this. Sounds like the needinfo? for me is not useful anymore. \o/
Flags: needinfo?(21)
WIP patch, still need the final design, but this at least has the functionality we need.
Posted file 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.
Flags: needinfo?(padamczyk)
Comment on attachment 8341868 [details] [review]
PR, remove buttons for iframes

Update style of framed net_error page based on spec. Added integration test.
Attachment #8341868 - Flags: review?(21)
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.
Attachment #8341868 - Flags: review?(21) → review+
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.
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 C1/1.4 S1(20dec)
https://github.com/mozilla-b2g/gaia/pull/14325
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
This hasn't been uplifted to 1.3 yet, so the testing in comment 18 didn't verify this bug.
Status: VERIFIED → RESOLVED
Closed: 6 years ago6 years ago
Keywords: verifyme
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.
Uplifted fd9a5d36581aa4fcfd4b121cd1cd2187496ce222 to:
v1.3 already had this commit
Flags: needinfo?(pdolanjski)
Blocks: 882186
No longer depends on: 882186
No longer blocks: 930630
Verified on 1.3 1/26/2014 build.
You need to log in before you can comment on or make changes to this bug.