Closed
Bug 942325
Opened 11 years ago
Closed 11 years ago
[offline] iframes in packaged apps display offline screen, "Close" button doesn't work
Categories
(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)
Firefox OS Graveyard
Gaia::System::Window Mgmt
Tracking
(blocking-b2g:1.3+, b2g-v1.3 verified)
Tracking | Status | |
---|---|---|
b2g-v1.3 | --- | verified |
People
(Reporter: mikehenrty, Assigned: mikehenrty)
References
Details
(Keywords: regression, Whiteboard: [systemsfe])
Attachments
(3 files)
In the new offline error screen landed in bug 882186, clicking the close button will not do anything.
Comment 1•11 years ago
|
||
Technically a regression because this worked on the old offline error page.
Keywords: regression
Updated•11 years ago
|
Component: Gaia::System → Gaia::System::Window Mgmt
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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)
Comment 6•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
(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•11 years ago
|
||
(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.
Updated•11 years ago
|
blocking-b2g: 1.3? → 1.3+
Assignee | ||
Comment 9•11 years ago
|
||
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
Updated•11 years ago
|
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Comment 10•11 years ago
|
||
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)
Comment 11•11 years ago
|
||
Glad that they are a new UX spec for this. Sounds like the needinfo? for me is not useful anymore. \o/
Flags: needinfo?(21)
Assignee | ||
Comment 12•11 years ago
|
||
WIP patch, still need the final design, but this at least has the functionality we need.
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
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+
Assignee | ||
Comment 16•11 years ago
|
||
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.
Updated•11 years ago
|
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 C1/1.4 S1(20dec)
Comment 17•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 18•11 years ago
|
||
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
Comment 19•11 years ago
|
||
This hasn't been uplifted to 1.3 yet, so the testing in comment 18 didn't verify this bug.
Assignee | ||
Comment 20•11 years ago
|
||
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•11 years ago
|
||
Uplifted fd9a5d36581aa4fcfd4b121cd1cd2187496ce222 to:
v1.3 already had this commit
status-b2g-v1.3:
--- → fixed
Updated•11 years ago
|
Flags: needinfo?(pdolanjski)
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•