895826 - Resolve lack of YT back button

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

As an app developer I would like to opt-in to system navigation of session history so that I don't have to modify my existing web content to work without a back button.


A lot of existing web content assumes the presence of a browser back button for navigation. Even AJAX-heavy web apps are encouraged to use the HTML5 history API and use URIs to represent state in order to not break the back button.

In Firefox OS we have been encouraging app developers to design their apps without relying on a back button to reduce the amount of browser chrome required for installed apps. A lot of app developers are reluctant to make this change to their web apps just for Firefox OS so we end up breaking the web.

The user experience of high profile properties like Facebook (bug 856567) and YouTube (bug 885227) on Firefox OS is currently very poor as a result.

There has been a proposal for over a year to add a property to the app manifest in order to let app developers opt-in to system navigation and show the wrapper we already have for everything.me content if this property is present.

I believe Jonas is in support of this change and Donovan has pointed out that it's literally a one line patch to enable this feature in the Gaia system app:

--- a/apps/system/js/window_manager.js
+++ b/apps/system/js/window_manager.js
@@ -949,7 +949,7 @@ var WindowManager = (function() {
     // frames are began unloaded.
     iframe.dataset.unloaded = true;

-    if (!manifestURL) {
+    if (!manifestURL || manifest.showNavigationUI) {
       frame.setAttribute('data-wrapper', 'true');
       return frame;

I'd like to submit this feature request for the system team to consider. Content partners have a strong need for this in 1.0.1, we'd love to see it in 1.1 but as we're feature frozen it may be more realistic to target it for 1.2? It seems to be extremely high value, low effort and low risk.

We should make the apps team aware of this as it would add a property to the app manifest format, but it probably doesn't require any platform changes.

In the longer term UX could look at making this type of navigation a more seamless part of the interaction model of the system UI.

Comment 1

[Ben Francis [:benfrancis]]

NEEDINFO Peter for prioritisation in backlog

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

Hmm...this sounds reasonable despite inventing slots in manifest.
Something in my brain is a general software backward button, not only using at in-app navigation, but also be able to back to the previous opened app/page, which may be tracked in window manager.

Comment 3

[Ben Francis [:benfrancis]]

(In reply to Alive Kuo [:alive] from comment #2)
> Hmm...this sounds reasonable despite inventing slots in manifest.
> Something in my brain is a general software backward button, not only using
> at in-app navigation, but also be able to back to the previous opened
> app/page, which may be tracked in window manager.

This is explicitly *not* a global system back button which switches between apps and confuses "back" with "up" or "switch window", it's just an opt-in session history navigation UI for web content which uses session history and was designed to be used with a back button.

Comment 4

[Donovan Preston [:fzzzy]]

We really need this in 1.1 if we are going to have a chance of including YouTube.

Please bikeshed on the name of the manifest property, I just picked the first thing that came to mind.

Comment 5

[Jason Smith [:jsmith]]

Is this blocking YouTube certification or preventing leo from allowing YouTube in our build if this isn't fixed? If answers to one of those questions is yes, then this should be a blocker.

Comment 6

[Donovan Preston [:fzzzy]]

Yes, this is blocking YT certification. They are unwilling to add back nav to their mobile site and are expecting that we will provide this functionality.

Comment 7

[Chris Lee [:clee]]

Given the needs from some of our key app partners, this should be prioritized high and if possible target 1.1.  

Jonas, can you help assess the risk here given we're way past adding new things to 1.1?  Thanks.

Nom'ing for leo.

Comment 8

[Alex Keybl [:akeybl]]

This isn't only a matter of risk, we (and our partners) are way way past the point of taking new feature change in any way. One of our partners is only cherry picking changes at this point.

This is basically a cut and dry minus. 1.2 is around the corner. I'll leave it nom'd so that people have the chance to reply before we bump this to 1.2.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Alex Keybl [:akeybl] from comment #8)
> This isn't only a matter of risk, we (and our partners) are way way past the
> point of taking new feature change in any way. One of our partners is only
> cherry picking changes at this point.
> 
> This is basically a cut and dry minus. 1.2 is around the corner. I'll leave
> it nom'd so that people have the chance to reply before we bump this to 1.2.

See https://bugzilla.mozilla.org/show_bug.cgi?id=895826#c6. We won't pass certification with YouTube without this.

Comment 10

[Donovan Preston [:fzzzy]]

It looks like manifest keys use underscores rather than camel case, so the name should probably be show_navigation_ui, unless someone has a better idea.

Comment 11

[Donovan Preston [:fzzzy]]

Attachment: Show the navigation wrapper if show_navigation_ui is true in the manifest

Attaching the patch with show_navigation_ui as the key.

Comment 12

[Donovan Preston [:fzzzy]]

Somehow the ? flag got cleared.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Since Donovan is writing the patch, I think he'll be better at assessing risk

Comment 14

[Donovan Preston [:fzzzy]]

I believe the risk to be very small.

It is a one line patch that does not change existing behavior unless the new key is provided in the manifest of an app.

If the key is present, it triggers the same path used by everything.me apps, which is presumably well tested.

Without this functionality in 1.1, I believe YouTube will not even be able to submit their app to the marketplace until 1.2, much less get preloaded.

Comment 15

[[:fabrice] Fabrice Desré]

(In reply to Donovan Preston from comment #14)
> I believe the risk to be very small.
> 
> It is a one line patch that does not change existing behavior unless the new
> key is provided in the manifest of an app.
> 
> If the key is present, it triggers the same path used by everything.me apps,
> which is presumably well tested.

If that means that apps end up using the same security principal as the system app, we can't do that. But hopefully you're just reusing the wrapper UI and not the iframe setup for the app itself.

Comment 16

[Jason Smith [:jsmith]]

Note - Getting leo+ isn't enough to get this into leo's build. You need a VB+ from them for the patch to get picked up.

Wayne - Can you followup here?

If leo ends up being unwilling to take this, then we'll need to use the everything.me YouTube app as a fallback.

Comment 17

[Jason Smith [:jsmith]]

Specifically meant to say in comment 16 - Wayne - Can you talk to Leo directly about this bug and see if they are okay taking the patch or not?

Comment 18

[Donovan Preston [:fzzzy]]

(In reply to Fabrice Desré [:fabrice] from comment #15)
> If that means that apps end up using the same security principal as the
> system app, we can't do that. But hopefully you're just reusing the wrapper
> UI and not the iframe setup for the app itself.

Ok. Thanks for the info. Sounds like I should look into this.

Comment 19

[Wayne Chang [:wchang]]

(In reply to Jason Smith [:jsmith] from comment #17)
> Specifically meant to say in comment 16 - Wayne - Can you talk to Leo
> directly about this bug and see if they are okay taking the patch or not?

Leo is assessing this now and will likely to take this patch.

Hi Youngjun, please whiteboard this bug accordingly when you have patched this in leo.

Comment 20

[Chris Lee [:clee]]

If we can get a leo+ on this, that'd be great.  This will make the priority clear for the team working on this.  Thanks.

Comment 21

[Donovan Preston [:fzzzy]]

(In reply to Donovan Preston from comment #18)
> (In reply to Fabrice Desré [:fabrice] from comment #15)
> > If that means that apps end up using the same security principal as the
> > system app, we can't do that. But hopefully you're just reusing the wrapper
> > UI and not the iframe setup for the app itself.
> 
> Ok. Thanks for the info. Sounds like I should look into this.

I don't think we can land this patch until understanding the implications of this comment.

Does this mean that apps launched from everything.me have the same security principal as the system app?

Comment 22

[Donovan Preston [:fzzzy]]

Unfortunately I am going to be traveling for a workweek this week and won't be able to look into it more than I did yesterday.

Comment 23

[Alex Keybl [:akeybl]]

(In reply to Chris Lee [:clee] from comment #20)
> If we can get a leo+ on this, that'd be great.  This will make the priority
> clear for the team working on this.  Thanks.

I can only leo+ fixing this bug in a low risk fashion, not this implementation specifically. Retitling to cover that.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Donovan Preston from comment #21)
> Does this mean that apps launched from everything.me have the same security
> principal as the system app?

Each application gets *two* principals, or rather principal-groups (each group contains different principals for different origins still). One principal-group for the application content, and one principal-group for "browser content". The "browser content" principals are used for content rendered by <iframe mozbrowser> iframes opened by that application.

So all everything.me applications use the same pricipal group. Which means that they live in the same cookie jar and can open each other using <iframe>s. It also means that we can't uninstall everything.me apps really since we don't know which of the cookies and other local data was created by a particular everything.me app.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Put another way, we need to create an <iframe mozbrowser mozapp="..."> here. If this patch makes us use a "simple" <iframe mozbrowser> (which is what we use for e.me apps), then that won't work.

But all we need is to ensure that we create a <iframe mozbrowser mozapp="..."> iframe. The two will have the same API so they should work pretty equivalently.

Comment 26

[[:fabrice] Fabrice Desré]

Attachment: patch

This patch adds the navigation wrapper UI while keeping the usual setup for apps. You can test by installing the app at https://people.mozilla.com/~fdesre/wrapper/install.html

We need YT to add the "show_navigation_ui": true property to their manifest.

Comment 27

[vkrishnamoorthy]

(In reply to Fabrice Desré [:fabrice] from comment #26)
> 
> We need YT to add the "show_navigation_ui": true property to their manifest.

I've sent out an email to YT to add this to their manifest

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 779394 [details] [diff] [review]
patch

Not the that I really care, but I was told we will never ever do this unless our fundamental definition on "open web app" changes.

Also, please do s/show_navigation_ui/show_navigation/.

Comment 29

[Youngjun Kim]

Leo will check the patch and if there is no issue, we will merge it.

Comment 30

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

(In reply to Jonas Sicking (:sicking) from comment #25)
> Put another way, we need to create an <iframe mozbrowser mozapp="..."> here.
> If this patch makes us use a "simple" <iframe mozbrowser> (which is what we
> use for e.me apps), then that won't work.

The navigation UI (we call it wrapper) is completely <iframe mozbrowser>/<iframe mozapp> agnostic. So there is no risk involve with security at all in this bug.


The goal is just to turn on the wrapper for some apps that needs it. 

Also in a foreseeable future the wrapper may contains a bit more things than just back/forward buttons so I would rather call the field 'toolbar' to match with what is described in https://developer.mozilla.org/en-US/docs/Web/API/window.open#Toolbar_and_chrome_features 

Bookmarks from the homescreen are opened via window.open call and I can see us trying to activate /deactivate the 'toolbar' name in the feature field for bookmark that needs / does not need a back button.

Then apps / bookmarks would be closer and share the same capability about having the system toolbar.

Comment 31

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

Now because this is a new property in the manifest it would be good to tell that to Mounir/Marco as well.

Does it sounds good for you guys?

Comment 32

[Marcos Caceres [:marcosc]]

In W3C parlance, this is really a "view_mode" of type "maximized" - which "Describes a Web application that is occupying the entirety of the screen area but with chrome." [1]  

So, instead of "show_navigation_ui", I would recommend adding "view_modes" member that is an array of string, of which the first is the most preferred view mode. 

For example: 

{
…
"view_modes": ["maximized"]
…
}

For a list of other view modes an app can be in see [1].

We are adding something similar to the W3C version of the manifest format [2]. 


[1] http://www.w3.org/TR/view-mode/#view-modes
[2] https://github.com/w3c/manifest/issues/22

Comment 33

[[:fabrice] Fabrice Desré]

I'm not convinced that we want a view mode here. Youtube is not asking to run maximized : this is done by the window manager because this is the only way we run apps on device. On bigger screens, we could have eg a tiling window manager, but YT would still need a navigation toolbar.

Comment 34

[Marcos Caceres [:marcosc]]

(In reply to Fabrice Desré [:fabrice] from comment #33)
> I'm not convinced that we want a view mode here. Youtube is not asking to
> run maximized : this is done by the window manager because this is the only
> way we run apps on device. On bigger screens, we could have eg a tiling
> window manager, but YT would still need a navigation toolbar.

Sure, but then it would just be "windowed": "Describes a Web application running in a windowed manner, which is to say with chrome and without occupying the entire screen area."

Then the app can just state:
{
…
"view_modes": ["maximized", "windowed"]
…
}

And the UA can select what is best for the available viewable area or window manager. Or is that totally off base?

Comment 35

[Marcos Caceres [:marcosc]]

Actually, you would declare that backwards:

{
…
"view_modes": ["windowed", "maximized"]
…
}

Meaning "if you support windowed" then use it (tablet), otherwise, if you support "maximized" use it (mobile), otherwise, I'll take whatever you got.

Comment 36

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

Comment on attachment 779394 [details] [diff] [review]
patch

I think doing this without setting originURL / searchURL on the iframe dataset will force a bookmark button on the bar wich is not wanted for apps I believe. 
Can you check that there is no bookmark button?

Comment 37

[Jason Smith [:jsmith]]

Let's be careful here. We need to ensure that the solution we have here is an incredibly low risk solution. Otherwise, leo won't take the patch for 1.1. I'm feeling a little bit nervous about the complexity being suggested here with view-modes for 1.1, as that could warrant a denial to take the patch into 1.1. That might mean that we need to temporary support for a different manifest property, however, like what's suggested in this patch.

Comment 38

[[:fabrice] Fabrice Desré]

(In reply to Marcos Caceres [:marcosc] from comment #35)
> Actually, you would declare that backwards:
> 
> {
> …
> "view_modes": ["windowed", "maximized"]
> …
> }
> 
> Meaning "if you support windowed" then use it (tablet), otherwise, if you
> support "maximized" use it (mobile), otherwise, I'll take whatever you got.

I still think that view modes are not the semantics we want to express here, but I won't fight on that.

Comment 39

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Fabrice Desré [:fabrice] from comment #38)
> (In reply to Marcos Caceres [:marcosc] from comment #35)
> 
> I still think that view modes are not the semantics we want to express here,
> but I won't fight on that.

I am not here to fight but "windowed" and "maximized" doesn't fit the case. We could also put a "transparent floating toolbar" on top of maximized window. IMO.

Comment 40

[Marcos Caceres [:marcosc]]

(In reply to Jason Smith [:jsmith] from comment #37)
> Let's be careful here. We need to ensure that the solution we have here is
> an incredibly low risk solution. Otherwise, leo won't take the patch for
> 1.1. I'm feeling a little bit nervous about the complexity being suggested
> here with view-modes for 1.1, as that could warrant a denial to take the
> patch into 1.1. That might mean that we need to temporary support for a
> different manifest property, however, like what's suggested in this patch.

Sure, that's a business decision someone here needs to make. The view modes thing doesn't really affect much, as it's just as much a trigger as `show_navigation` (as we only really support two view modes - fullscreen and maximized - and we don't need to add support for any additional ones for now). 

The differentiation between the two proposals is that if you go down the `show_navigation` path you start handing over fine-grained control of the UA's GUI to developers. I personally don't think this is a good thing because control of the UA's UI has generally been a contract between the browser maker and the user (except in the case of window.open() ... I don't want to get into comparisons here - but just keep that in mind). 

FWIW, I'm ok with whatever option you guys choose.

Comment 41

[Marcos Caceres [:marcosc]]

(In reply to Alive Kuo [:alive][Paris work week 7/22-26] from comment #39)
> (In reply to Fabrice Desré [:fabrice] from comment #38)
> > (In reply to Marcos Caceres [:marcosc] from comment #35)
> > 
> > I still think that view modes are not the semantics we want to express here,
> > but I won't fight on that.
> 
> I am not here to fight but "windowed" and "maximized" doesn't fit the case.
> We could also put a "transparent floating toolbar" on top of maximized
> window. IMO.

No, that would defeat the purpose: The only point of view modes is to abstract the GUI components away (and let the UA and the user negotiate what GUI they want). What you are proposing goes in the opposite direction (this is not a bad thing, btw - I'm just trying to be clear as to why view modes seems kinda limited ... it's supposed to be a fairly minimal declarative mechanism).

Comment 42

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Now using view_modes. If either 'windowed' or 'maximized' is asked for, we add the wrapper (I think that since we don't have really a windowed mode, we still need to provide some chrome in this case).

Vivien, I checked that the toolbar has only back, forward and reload - but no "bookmark" control.

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think that there are at least three "modes" here.

For windowed UIs it's pretty clear that the UA can provide UI with or without navigation. On desktop we can provide a window which looks just like normal app windows which has UI for resizing (including maximizing/minimizing/fullscreening), dragging and closing the window, or windows which also has navigation controls.

Same thing on mobile where we can either stick navigation controls at the stop of the screen or not.

For fullscreen UIs it's a bit more tricky. Even in browsers we generally don't provide navigation UI in fullscreen mode. So maybe it makes sense not to do that here either, at least for now.


However I'm still not convinced that it makes sense to use view modes here. We need to be able to provide guarantees to the app that we are providing navigation UI. If an application requests that we display UI, and we don't, then the application will most likely be broken from the users point of view.

So tying it in with view modes, which seems more like hints rather than guarantees, doesn't seem right.

So I think I would prefer to keep the two separate. And for now, if an app requests fullscreen but with navigation UI, that we ignore the fullscreen part. Maybe some time in the future we'll figure out how to render navigation UI even in fullscreen, at which point we can support that combination too.

Comment 44

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Jonas agreed on irc to use enable_navigation_ui as the manifest property name.

Comment 45

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

(In reply to Fabrice Desré [:fabrice] from comment #44)
> Created attachment 780726 [details] [diff] [review]
> patch v3
> 
> Jonas agreed on irc to use enable_navigation_ui as the manifest property
> name.

I'm really sorry about bikesheding here for the name here but I feel like the concern I tried to raised in #c30 has not been answered.

Can we have a more generic name than navigation_ui? The UX concepts for the future does already have more controls than just 'back'/'forward'.  I don't remember what I have seen exactly but there were at least 5 controls and this is just mockups.

To me 'navigation' == 'back' and 'forward' buttons. A more generic name would avoid adding too much granularity / confusion in the manifest imho. 

If 'navigationUI' sounds like a good name to contains things like 'save as' or 'view source' for examples then that sounds good to me and that's only because my english fails. If not please pick an other name.

So Jonas I will ask a needinfo? on you and you will be the one responsible for the naming of this stuff for the future. If you state clearly here that 'show_navigation_ui' makes sense even if there are more buttons than just back/forward versus something like 'show_chrome_ui' for example the patch has my r+.  

I know bikeshedding on name is not your cup of tea btw :) Have fun.

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we explicitly want just back/forward/reload here. Not things like "save as" and a url bar. That's why I suggested using "navigation" in the name rather than "toolbar".

Comment 47

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

(In reply to Jonas Sicking (:sicking) from comment #46)
> I think we explicitly want just back/forward/reload here. Not things like
> "save as" and a url bar. That's why I suggested using "navigation" in the
> name rather than "toolbar".

Argh. I would like to unify the experience here vs browsing regular web content and avoid such granularity :(

I'm fine with the code though. So let's go ahead.

Comment 48

[Mounir Lamouri (:mounir)]

I do not think this is really related to view_modes. It might be a problem with want to solve only/mostly for windowed applications but we can definitely imagine a windowed application without a back button. For example, a game, windowed or fullscreen will unlikely want a back button.

The issue here seems more related to what kind of chrome UI we should add. The default could vary depending on the view_mode type but I am not sure if we want that to happen. But for sure, we might want to have other UI element to show up in other contexts. For example, maybe some hosted web applications will require an URL bar or a refresh button. Some windowed applications might want to not have a title bar or no maximise button. It seems that a saner short them solution would be to add a "chrome" property that is an object taking a set of properties. For example:
"chrome": {
  "back_button": true,
  "url_bar": false,
  "refresh": false
}

That way, we don't have to assume that a windowed application will always want to have a back button (which is what the current proposal does AFAICT).

Comment 49

[Marcos Caceres [:marcosc]]

(In reply to Mounir Lamouri (:mounir) from comment #48)
> "chrome": {
>   "back_button": true,
>   "url_bar": false,
>   "refresh": false
> }
> 
> That way, we don't have to assume that a windowed application will always
> want to have a back button (which is what the current proposal does AFAICT).

If we are going to go down this route, I agree that the above is a much nicer way to go.

Comment 50

[vkrishnamoorthy]

This maybe more a documentation issue and I bring it up for completeness.

For platforms that already have back navigation in place (such as android devices etc..), I am assuming we ignore this property.

Comment 51

[[:fabrice] Fabrice Desré]

Pushed on master: https://github.com/mozilla-b2g/gaia/commit/06f5557075c578525eddbc90b9687f8b18b6f545

Comment 52

[Leo]

(In reply to Fabrice Desré [:fabrice] from comment #51)
> Pushed on master:
> https://github.com/mozilla-b2g/gaia/commit/
> 06f5557075c578525eddbc90b9687f8b18b6f545

Would you uplift to v1-train also? Thanks!

Comment 53

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Mounir Lamouri (:mounir) from comment #48)
> "chrome": {
>   "back_button": true,
>   "url_bar": false,
>   "refresh": false
> }

I like this! Though I would change "back_button" to "navigation" and have it include both back and forward buttons.

Comment 54

[[:fabrice] Fabrice Desré]

:( if we're serious about using "chrome" : {...}, let's push a followup asap. Jonas?

Comment 55

[Mounir Lamouri (:mounir)]

(In reply to Fabrice Desré [:fabrice] from comment #54)
> :( if we're serious about using "chrome" : {...}, let's push a followup
> asap. Jonas?

Please :)

Comment 56

[[:fabrice] Fabrice Desré]

Followup to use "chrome" : {"navigation": true} instead:
https://github.com/mozilla-b2g/gaia/commit/adc94dd96cc417f2c3961b0176c2f2b2d96b9abe

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes!

Comment 58

[Mounir Lamouri (:mounir)]

Thank you Fabrice :)

Comment 59

[Jason Smith [:jsmith]]

With the patches landed on this bug, can we mark this resolved fixed? Or is there work left to do here?

Comment 60

[Donovan Preston [:fzzzy]]

(In reply to Jason Smith [:jsmith] from comment #59)
> With the patches landed on this bug, can we mark this resolved fixed? Or is
> there work left to do here?

The patch just needs uplifting into the 1.1 branch, then we can close this.

Comment 61

[Jason Smith [:jsmith]]

Closing as fixed since this landed on master. Flagging affected on b2g18 to indicate this needs to be uplifted.

Comment 62

[vkrishnamoorthy]

now that we have this feature, it will be good to document and make it visible to app developers, bug 901003 addresses that.

Comment 63

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

Uplifted adc94dd96cc417f2c3961b0176c2f2b2d96b9abe to:
v1-train: c6e2637ab69606009ae927bc8a0b716c10ee4c23

Comment 64

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: c6e2637ab69606009ae927bc8a0b716c10ee4c23
v1.1.0hd: 1d544b94949213979377a52a696a59fa4e767db7

Comment 65

[Jason Smith [:jsmith]]

Looks good with a nit-pick followup in bug 903165.

Comment 66

[Mark Giffin [:markg]]

The chrome manifest property is documented here in MDN:

https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#chrome