Last Comment Bug 771915 - Show the origin of the page in the chrome of the app when it is different from webapp's origin
: Show the origin of the page in the chrome of the app when it is different fro...
Status: VERIFIED FIXED
[blocking-webrtandroid1+]
: uiwanted
Product: Firefox for Android
Classification: Client Software
Component: Web Apps (show other bugs)
: Trunk
: ARM Android
: P1 normal (vote)
: Firefox 18
Assigned To: Wesley Johnston (:wesj)
: Aaron Train [:aaronmt]
:
Mentors:
Depends on:
Blocks: Blocking-FFA-WebRT1+
  Show dependency treegraph
 
Reported: 2012-07-08 08:24 PDT by Jason Smith [:jsmith]
Modified: 2012-10-06 00:19 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified


Attachments
Patch (4.81 KB, patch)
2012-07-13 17:42 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Screenshot (198.99 KB, image/png)
2012-07-24 17:35 PDT, Wesley Johnston (:wesj)
no flags Details
Patch (5.35 KB, patch)
2012-08-01 15:36 PDT, Wesley Johnston (:wesj)
mark.finkle: feedback+
Details | Diff | Splinter Review
Screenshot (177.33 KB, image/png)
2012-08-01 15:37 PDT, Wesley Johnston (:wesj)
no flags Details
Mockup (207.74 KB, image/png)
2012-08-10 08:27 PDT, Ian Barlow (:ibarlow)
no flags Details
Patch (10.00 KB, patch)
2012-08-14 13:15 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch (10.00 KB, patch)
2012-08-14 13:49 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2 (10.04 KB, patch)
2012-08-14 13:51 PDT, Wesley Johnston (:wesj)
mark.finkle: review-
Details | Diff | Splinter Review
Patch v3 (10.41 KB, patch)
2012-08-23 11:52 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Screenshot (399.92 KB, image/png)
2012-08-23 11:55 PDT, Wesley Johnston (:wesj)
no flags Details

Description Jason Smith [:jsmith] 2012-07-08 08:24:01 PDT
Flagged by security for desktop in bug 771395, so likely applies to Android as well. If an app goes off the origin of the app origin, it needs to show the current origin of the page in the chrome of the web app. An example use case:

1. Launch mozilla marketplace <-- nothing in the chrome yet
2. Click login <-- pop-up would have chrome with BID as the origin
Comment 1 Aaron Train [:aaronmt] 2012-07-08 19:25:40 PDT
Vlad/Wes, thoughts?
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-09 09:02:34 PDT
It's all possible to do, but this needs UX input to figure out how to do it.
Comment 3 Wesley Johnston (:wesj) 2012-07-09 10:46:46 PDT
Idea? Show the Android titlebar in these cases, and use setTitle() (which will work here) to make it show the origin?
Comment 4 Jason Smith [:jsmith] 2012-07-09 16:18:11 PDT
(In reply to Wesley Johnston (:wesj) from comment #3)
> Idea? Show the Android titlebar in these cases, and use setTitle() (which
> will work here) to make it show the origin?

Works for me.
Comment 5 Wesley Johnston (:wesj) 2012-07-13 17:42:55 PDT
Created attachment 642152 [details] [diff] [review]
Patch

This shows a simple titlebar at the top of the screen when you're on a third party and hides it when you're on the trusted app's origin. Tested with Facebook and Twitter links on Jauntly.

Its a bit ugly right now, but I can live with that.
Comment 6 Wesley Johnston (:wesj) 2012-07-24 17:35:46 PDT
Created attachment 645603 [details]
Screenshot

This is a screenshot of this (with the url moved to the bottom of the screen). Probably needs a shadow or a line dividing it from content and a little padding. Also, its shifting content up (i.e. we need to relayout).
Comment 7 Wesley Johnston (:wesj) 2012-08-01 15:36:50 PDT
Created attachment 648112 [details] [diff] [review]
Patch

This seems to work well. Very unobtrusive. Some amazing art by me. Shows the url at the bottom of the screen. Sadly, its spoofable if the app wants to pretend to be off-origin when its not. I'll post a screenshot.
Comment 8 Wesley Johnston (:wesj) 2012-08-01 15:37:24 PDT
Created attachment 648114 [details]
Screenshot
Comment 9 Wesley Johnston (:wesj) 2012-08-01 15:39:01 PDT
That's a bit hard to read. Looks better on my device, but its still light colored. Easy way to test is to use ed.agadak.net/app, install the ed agadak app which has links to google/youtube/etc.
Comment 10 Jason Smith [:jsmith] 2012-08-01 15:40:13 PDT
(In reply to Wesley Johnston (:wesj) from comment #7)
> Created attachment 648112 [details] [diff] [review]
> Patch
> 
> This seems to work well. Very unobtrusive. Some amazing art by me. Shows the
> url at the bottom of the screen. Sadly, its spoofable if the app wants to
> pretend to be off-origin when its not. I'll post a screenshot.

Can you clarify how an app could do this? Could this be a security concern?
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-02 20:52:27 PDT
Comment on attachment 648112 [details] [diff] [review]
Patch

Overall this approach is fine, but I want Ian to take a look at the screenshot and offer some tweaks.

For example:
* Top or bottom of the screen? Bottom is more like a statusbar, but Android has no such concept.
* Should we mimic the normal Android title bar a bit more?
Comment 12 Wesley Johnston (:wesj) 2012-08-02 20:55:14 PDT
Sounds good. I can put up a build as well as some instructions too. I found the appearance of the titlebar at the top distracting. It would appear and shove content down. Maybe a nice transition would help (but that would be a layout transition which Android kinda hates). I'd rather do that later though.
Comment 13 Jason Smith [:jsmith] 2012-08-03 14:08:13 PDT
Btw, looking at the screenshot, are you showing the full URL or just the origin?
Comment 14 Wesley Johnston (:wesj) 2012-08-03 14:09:05 PDT
full url
Comment 15 Jason Smith [:jsmith] 2012-08-03 14:15:11 PDT
(In reply to Wesley Johnston (:wesj) from comment #14)
> full url

Oh. For meeting the original security requirement, I believe you only need to show the origin (developer.mozilla.org if you were on developer.mozilla.org/en-US/Apps), not the full URL. Not sure of the tradeoffs of only showing the origin vs. the full URL.
Comment 16 Wesley Johnston (:wesj) 2012-08-08 09:49:05 PDT
I talked to ian on irc. He said he'd rather the url was at the top. I'll make that adjustment and file a follow up to animate it? He said he can do a mockup, but I'm fine moving that somewhere else for follow up prettifying.
Comment 17 Ian Barlow (:ibarlow) 2012-08-10 08:27:00 PDT
Created attachment 650891 [details]
Mockup

Attached is a mockup describing where we'd like to take the design of this bar. Basically, it should look like a micro version of the title bar. 

I have broken the design into two parts here. The first part takes what Wes has already done and adds some styling. The second part adds a few elements that might also be useful down the road, like a place for the Globe / Lock icons, and potentially a progress indicator while the page is loading. 

One possible approach might be to do the first part now for 16, and the second for 17.
Comment 18 Ian Barlow (:ibarlow) 2012-08-10 08:29:23 PDT
(In reply to Ian Barlow (:ibarlow) from comment #17)
> Basically, it should look like a micro version of the title bar. 
> 

Sorry, I meant to say a micro version of the *Firefox* title bar there.
Comment 19 Myk Melez [:myk] [@mykmelez] 2012-08-10 14:06:43 PDT
(In reply to Ian Barlow (:ibarlow) from comment #17)
> I have broken the design into two parts here. The first part takes what Wes
> has already done and adds some styling. The second part adds a few elements
> that might also be useful down the road, like a place for the Globe / Lock
> icons, and potentially a progress indicator while the page is loading. 

Over in bug 772680, we decided not to show a throbber when an app is doing network activity.  The rationale is not described in the bug, but the summary is that doing so should be the responsibility of the application, not the runtime, because such runtime behavior is conventional (cf. the Java and Adobe Air runtimes), and because the semantics of network activity are complex in modern web apps, and the runtime does not have enough context to present progress indicators on their apps' behalves.

The latter is especially true when the indication is of the loading of a page, since modern webapps are often constructed with lightweight pages that load content dynamically after the static page and its statically-declared resources have loaded.

So I don't think we should be showing a progress indicator for these pages.  Nor show other UI that is designed to emulate browsing affordances.  The goal here is for the normally-invisible runtime to intrude on the user's experience of apps to the minimum extent necessary to protect users from the consequences of malicious or insecure apps.
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-11 11:29:06 PDT
(In reply to Myk Melez [:myk] [@mykmelez] from comment #19)

> So I don't think we should be showing a progress indicator for these pages. 
> Nor show other UI that is designed to emulate browsing affordances.  The
> goal here is for the normally-invisible runtime to intrude on the user's
> experience of apps to the minimum extent necessary to protect users from the
> consequences of malicious or insecure apps.

That's OK. We can move forward on the basic styling and perhaps the globe
Comment 21 Ian Barlow (:ibarlow) 2012-08-13 06:40:09 PDT
(In reply to Mark Finkle (:mfinkle) from comment #20)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #19)
> 
> > So I don't think we should be showing a progress indicator for these pages. 
> > Nor show other UI that is designed to emulate browsing affordances.  The
> > goal here is for the normally-invisible runtime to intrude on the user's
> > experience of apps to the minimum extent necessary to protect users from the
> > consequences of malicious or insecure apps.
> 
> That's OK. We can move forward on the basic styling and perhaps the globe

Yup, sounds good.
Comment 22 Wesley Johnston (:wesj) 2012-08-14 13:15:13 PDT
Created attachment 651861 [details] [diff] [review]
Patch

This move this to the top and styles it using an Android gradient.
Comment 23 Wesley Johnston (:wesj) 2012-08-14 13:49:30 PDT
Created attachment 651870 [details] [diff] [review]
Patch

Patch v2 - This changes this to just show the protocol + host
Comment 24 Wesley Johnston (:wesj) 2012-08-14 13:51:15 PDT
Created attachment 651872 [details] [diff] [review]
Patch v2

Grrr. forgot to qref.
Comment 25 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-14 17:42:56 PDT
Comment on attachment 651872 [details] [diff] [review]
Patch v2

* Is the webapp_titlebar_bg.xml Ian-approved? :)
* You add webapp_titlebar_bg.xml into the Makefile twice, but only have one actual file (I think). Is that right?
* Do we need to import AnimationUtils? I don't see it explicitly used.
* SharedPreferences prefs does not seem to be used
* Getting the "app" using the index created from a string seems weak. Do we have any other ways to look it up?
Comment 26 Wesley Johnston (:wesj) 2012-08-23 11:52:36 PDT
Created attachment 654718 [details] [diff] [review]
Patch v3

Updated patch. I added a "getIndex" function that's defined for WebApp# in the fragment. Good idea. I'll attach a screenshot too and ping ibarlow for approval.
Comment 27 Wesley Johnston (:wesj) 2012-08-23 11:55:18 PDT
Created attachment 654719 [details]
Screenshot
Comment 28 Wesley Johnston (:wesj) 2012-08-23 13:10:41 PDT
irc approval from ian with a change in the text color to 666666
Comment 29 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-23 13:25:37 PDT
Comment on attachment 654718 [details] [diff] [review]
Patch v3

>diff --git a/mobile/android/base/WebAppsFragment.java.frag b/mobile/android/base/WebAppsFragment.java.frag

>+    protected int getIndex() { return @APPNUM@; }

Nice

>diff --git a/mobile/android/base/resources/drawable/webapp_titlebar_bg.xml b/mobile/android/base/resources/drawable/webapp_titlebar_bg.xml

>\ No newline at end of file

nit: add a newline
Comment 31 Ed Morley [:emorley] 2012-08-28 06:13:00 PDT
https://hg.mozilla.org/mozilla-central/rev/8264b7d01a5a
Comment 32 Wesley Johnston (:wesj) 2012-08-28 09:15:35 PDT
Comment on attachment 654718 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Webapps
User impact if declined: Security concerns with sites that take people to an unexpected off-origin url
Testing completed (on m-c etc.): Landed on mc 8/27
Risk to taking this patch (and alternatives if risky): Low risk. Webapps only
String or UUID changes made by this patch: None.
Comment 33 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-29 15:16:10 PDT
Comment on attachment 654718 [details] [diff] [review]
Patch v3

Low risk, webapps only (targeted for 17) approving.
Comment 34 Wesley Johnston (:wesj) 2012-10-05 18:15:53 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3a804ecaa08

Note You need to log in before you can comment on or make changes to this bug.