Last Comment Bug 698598 - Ux Designs for Doorhanger Notifications
: Ux Designs for Doorhanger Notifications
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: Sriram Ramasubramanian [:sriram]
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 699772 (view as bug list)
Depends on:
Blocks: 701305
  Show dependency treegraph
 
Reported: 2011-10-31 14:52 PDT by Erin Lancaster [:elan]
Modified: 2016-07-29 14:20 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Door hanger spec (2.70 MB, image/jpeg)
2011-11-04 08:18 PDT, Patryk Adamczyk [:patryk] UX
no flags Details
Android 4 door hanger 9 slice (1.75 KB, image/png)
2011-11-04 08:19 PDT, Patryk Adamczyk [:patryk] UX
no flags Details
Android 2.2/2.3 door hanger (1.43 KB, image/png)
2011-11-04 08:19 PDT, Patryk Adamczyk [:patryk] UX
no flags Details
Door hanger spec (2.70 MB, image/jpeg)
2011-11-04 08:25 PDT, Patryk Adamczyk [:patryk] UX
no flags Details
Door hanger spec (2.70 MB, image/jpeg)
2011-11-04 08:27 PDT, Patryk Adamczyk [:patryk] UX
no flags Details
Patch (1/2): Plumbing for showing on top + stacking (27.52 KB, patch)
2011-11-04 14:10 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review
Patch (1.5/2): Avoiding flickering while closing a background tab (3.72 KB, patch)
2011-11-04 14:44 PDT, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review
DoorHanger with native theme in Nexus S (40.66 KB, image/png)
2011-11-04 15:34 PDT, Sriram Ramasubramanian [:sriram]
no flags Details
DoorHanger with native theme in Droid Pro (40.66 KB, image/png)
2011-11-04 15:34 PDT, Sriram Ramasubramanian [:sriram]
no flags Details
9Slice with Layout Spec (1.49 MB, application/zip)
2011-11-07 07:33 PST, Patryk Adamczyk [:patryk] UX
no flags Details
Patch (1/2): Plumbing for showing on top + stacking (27.58 KB, patch)
2011-11-07 10:32 PST, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review
Patch (1.5/2): Avoiding flickering while closing a background tab (3.69 KB, patch)
2011-11-07 10:32 PST, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review
DoorHanger with native theme in Nexus S (141.65 KB, image/png)
2011-11-07 11:11 PST, Sriram Ramasubramanian [:sriram]
no flags Details
Updated stacked door hangers (1.22 MB, image/png)
2011-11-08 14:25 PST, Patryk Adamczyk [:patryk] UX
no flags Details
Updated 9slice for Android 2.3 and older devices (3.04 KB, application/zip)
2011-11-09 08:01 PST, Patryk Adamczyk [:patryk] UX
no flags Details
Patch (2/2): Reskinning the UI (15.48 KB, patch)
2011-11-09 14:49 PST, Sriram Ramasubramanian [:sriram]
mark.finkle: review+
Details | Diff | Splinter Review

Description Erin Lancaster [:elan] 2011-10-31 14:52:13 PDT
Meta bug.
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-04 06:41:34 PDT
I am morphing this bug into "UX for doorhanger notifications". Alerts and Dialogs are both using the standard Android UI elements. Doorhangers are not and need attention.

Patryk ans Sriram are working on doorhanger notifications
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-04 06:41:50 PDT
*** Bug 699772 has been marked as a duplicate of this bug. ***
Comment 3 Patryk Adamczyk [:patryk] UX 2011-11-04 08:18:30 PDT
Created attachment 571983 [details]
Door hanger spec
Comment 4 Patryk Adamczyk [:patryk] UX 2011-11-04 08:19:03 PDT
Created attachment 571984 [details]
Android 4 door hanger 9 slice
Comment 5 Patryk Adamczyk [:patryk] UX 2011-11-04 08:19:39 PDT
Created attachment 571985 [details]
Android 2.2/2.3 door hanger
Comment 6 Patryk Adamczyk [:patryk] UX 2011-11-04 08:25:29 PDT
Created attachment 571986 [details]
Door hanger spec
Comment 7 Patryk Adamczyk [:patryk] UX 2011-11-04 08:27:20 PDT
Created attachment 571987 [details]
Door hanger spec
Comment 8 Sriram Ramasubramanian [:sriram] 2011-11-04 14:10:16 PDT
Created attachment 572073 [details] [diff] [review]
Patch (1/2): Plumbing for showing on top + stacking

This patch moves the doorhangers to be anchored to the browser toolbar. More doorhangers from same page are stacked one below another. This popup is scrollable.

Sample testing navigation flow:
1. Open a new tab with google.com
2. Search for "google maps"
3. Navigate to "maps.google.com" from the results
4. Navigate back (back button)
5. Navigate forward (menu > forward)
6. Reload the page
7. Hit on the url bar and change the url to "http://sjeng.org/mozilla/geo.html"
8. Add a new tab (with or without doorhangers)
9. Close a foreground tab with notifications
10. Close a background tab with notifications

Code comments:
1. addDoorHanger() in DoorHangerPopup.java makes sure that, if there is an existing DoorHanger for the tab with same value, it is removed and a new one is replaced there. Google maps had issues while reloading (rather it was trying to send periodic requests for geo-location after a timeout), which caused stacking of more DoorHangers without this _removing_. Also, there were issues with callback which made me return a brand new DoorHanger everytime we get a "DoorHanger:Add" request from Gecko.

2. The "hideAllDoorHangers()" inside "showForTab()" (in DoorHangerPopup.java) makes the DoorHangers to hide and show, if the selectedTab has doorhangers and a background tab is closed. This should be made more intelligent by not hiding the doorhangers for the selected tab. This will be taken as a followup patch.

3. I am using "value" as the key in the tab and it is barely used inside DoorHanger.java. I feel like removing it. In case, we want to have the value in it for semantic reasons, we can have it there.

Follow-up patches:
1. Making hideAllDoorHangers() more intelligent.
2. Reskinning based on the spec.
Comment 9 Sriram Ramasubramanian [:sriram] 2011-11-04 14:44:22 PDT
Created attachment 572085 [details] [diff] [review]
Patch (1.5/2): Avoiding flickering while closing a background tab

This patch avoids the flickering (hide and show) of the doorhangers if a background tab is closed. This is done by making the "hideAllDoorHangers()" not to close if the doorhanger is for the selected-tab.

This also fixes a condition where DoorHanger's can appear on wrong tab by hiding it while creation. (This happens, when a tab with door hangers is open, google maps is open in another tab, and switched to the old tab with door hangers -- google maps tries request periodically, causing the doorhanger to appear in old tab).
Comment 10 Sriram Ramasubramanian [:sriram] 2011-11-04 14:44:46 PDT
Patch 1.5/2 should be applied on top of Patch 1/2 :)
Comment 11 Sriram Ramasubramanian [:sriram] 2011-11-04 15:34:29 PDT
Created attachment 572108 [details]
DoorHanger with native theme in Nexus S

I was trying to see how things will look if we use native styling for DoorHangers like in http://www.flickr.com/photos/patrykdesign/6299624109/in/photostream
This is Nexus S.
Comment 12 Sriram Ramasubramanian [:sriram] 2011-11-04 15:34:58 PDT
Created attachment 572109 [details]
DoorHanger with native theme in Droid Pro

This is with Droid Pro.
Comment 13 Sriram Ramasubramanian [:sriram] 2011-11-04 16:02:33 PDT
Based on these, here are my suggestions:

1. The bottom "button bar" can use the styling from the Native android. This makes sure we use the styling as needed. Like say a crazy theme has white button with a grey bar, we are not at loss.

2. The top "message" portion can be custom with an arrow. Thereby, no matter what sort of UI it is, this will be part of our primary UI.
Comment 14 Sriram Ramasubramanian [:sriram] 2011-11-05 10:50:14 PDT
I copied GCP's test site, and edited two doorhangers (because of popup windows) to appear after a timeout. Please use this to test that condition: http://dl.dropbox.com/u/3017599/Geo/geo.html
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-07 05:56:58 PST
Comment on attachment 572073 [details] [diff] [review]
Patch (1/2): Plumbing for showing on top + stacking


>diff --git a/embedding/android/DoorHanger.java b/embedding/android/DoorHanger.java

>+public class DoorHanger extends LinearLayout implements Button.OnClickListener {

>+    public void onClick(View v) {
>+        GeckoEvent e = new GeckoEvent("Doorhanger:Reply", "" + v.getTag());

v.getTag.toString()  ?

>diff --git a/embedding/android/Tab.java b/embedding/android/Tab.java

> public class Tab {

>+    public void addDoorHanger(String value, DoorHanger dh) {
>+        mDoorHangers.put(value, dh);
>+    } 
>+
>+    public void removeDoorHanger(String value) {
>+        mDoorHangers.remove(value);
>+    }
>+
>+    public void removeAllDoorHangers() {
>+        mDoorHangers = new HashMap<String, DoorHanger> ();
>+    }
>+
>+    public DoorHanger getDoorHanger(String value) {
>+        if (mDoorHangers == null)
>+            return null;
>+
>+        if (mDoorHangers.containsKey(value))
>+            return mDoorHangers.get(value);
>+
>+        return null;
>+   } 

Adding these methods to Tab seems messy, but I suppose we can add a TabDoorHanger later to cleanup the Tab interface.

>+    public HashMap<String, DoorHanger> getDoorHangers() {
>+        if (mDoorHangers == null)
>+            return null;
>+
>+        return mDoorHangers;
>+    }

Why bother with the null check? Looks like | return mDoorHangers; | would work

r+ with any tweaks and unbitrot the patch
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-07 06:07:38 PST
Comment on attachment 572085 [details] [diff] [review]
Patch (1.5/2): Avoiding flickering while closing a background tab


>diff --git a/embedding/android/DoorHangerPopup.java b/embedding/android/DoorHangerPopup.java

>     public void hideAllDoorHangers() {
>+        hideAllDoorHangers(null);
>+    }
>+
>+    public void hideAllDoorHangers(Tab exceptForTab) {
>         for (int i=0; i < mContent.getChildCount(); i++) {
>             DoorHanger dh = (DoorHanger) mContent.getChildAt(i);
>-            dh.hidePopup();
>+            if (dh.getTab() != exceptForTab)
>+                dh.hidePopup();
>         }
> 
>-        hide();
>+        if (exceptForTab == null)
>+            hide();
>     }

I don't normally like this kind of method name overriding. It's hard to know if we are hiding doorhangers for the tab or everything else.

I'd be happier if you renamed the new method to "hideAllDoorHangersExcept" (other options welcome)

r+, but make the change and make sure the patch is unbitrotted
Comment 17 Patryk Adamczyk [:patryk] UX 2011-11-07 07:33:38 PST
Created attachment 572468 [details]
9Slice with Layout Spec

Updated the 9slice for the door hanger, separate the arrow graphic from the 9slice.
Comment 18 Sriram Ramasubramanian [:sriram] 2011-11-07 10:32:05 PST
Created attachment 572522 [details] [diff] [review]
Patch (1/2): Plumbing for showing on top + stacking

This tweaks the previous patch as required.
Comment 19 Sriram Ramasubramanian [:sriram] 2011-11-07 10:32:49 PST
Created attachment 572523 [details] [diff] [review]
Patch (1.5/2): Avoiding flickering while closing a background tab

This tweaks the previous patch as required.
Comment 20 Sriram Ramasubramanian [:sriram] 2011-11-07 11:11:35 PST
Created attachment 572538 [details]
DoorHanger with native theme in Nexus S

DoorHanger image updated.
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-07 12:37:32 PST
pushed these patches:
https://hg.mozilla.org/projects/birch/rev/c52dbaf4f26b
https://hg.mozilla.org/projects/birch/rev/a5d887e503b5

We still need additional styling, so leaving open
Comment 22 Patryk Adamczyk [:patryk] UX 2011-11-08 14:25:18 PST
Created attachment 573006 [details]
Updated stacked door hangers
Comment 23 Patryk Adamczyk [:patryk] UX 2011-11-09 08:01:13 PST
Created attachment 573201 [details]
Updated 9slice for Android 2.3 and older devices

The 9 slice should be cut as follows.
From the left. 1 / 1 / 1 px
From the top. 4 / 1 / 11px
The arrow should be placed on top of the 9slice, 3 px deep.
Comment 24 Sriram Ramasubramanian [:sriram] 2011-11-09 14:49:45 PST
Created attachment 573338 [details] [diff] [review]
Patch (2/2): Reskinning the UI

This patches adds the new UI for doorhangers.
I am a bit worried about the images -- especially the shadows as they don't taper in the edges. Probably we might need bigger images to get what the UI spec demands.
Comment 25 Wesley Johnston (:wesj) 2011-11-10 10:30:45 PST
The patches that had landed were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.
Comment 26 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-10 12:28:50 PST
Comment on attachment 573338 [details] [diff] [review]
Patch (2/2): Reskinning the UI


>diff --git a/embedding/android/BrowserToolbar.java b/embedding/android/BrowserToolbar.java

>-    final private ImageButton mFavicon;
>+    final public ImageButton mFavicon;

Lets add a public function called getDoorHangerAnchor(), it returns a View, and have it return the mFavicon. This lets us move the anchor without updating lots of code.
Comment 27 :Margaret Leibovic 2011-11-10 17:25:03 PST
Sriram, I just noticed that this latest patch conflicts with my patch in bug 701305. Is it ready to land once the tree re-opens? If so, I'll just rebase my patch on top of it.
Comment 28 Sriram Ramasubramanian [:sriram] 2011-11-10 18:25:43 PST
This is ready to land. I thought of making that one change before landing. But, the previous patches were backed out in birch to make any change. I'll probably do it as a followup.
Comment 29 Doug Turner (:dougt) 2011-11-10 20:42:51 PST
patched didn't apply clean.


Thu Nov 10 20:42:13 dougt@dougt:~/builds/birch ] $ hg qpush
applying attachment.cgi?id=572522
patching file embedding/android/DoorHanger.java
Hunk #1 FAILED at 14
Hunk #2 FAILED at 31
2 out of 2 hunks FAILED -- saving rejects to file embedding/android/DoorHanger.java.rej
patching file embedding/android/DoorHangerPopup.java
Hunk #1 FAILED at 14
Hunk #2 FAILED at 31
2 out of 2 hunks FAILED -- saving rejects to file embedding/android/DoorHangerPopup.java.rej
patching file embedding/android/GeckoApp.java
Hunk #1 FAILED at 91
Hunk #2 FAILED at 615
Hunk #3 FAILED at 802
Hunk #4 FAILED at 1039
Hunk #5 FAILED at 1486
5 out of 5 hunks FAILED -- saving rejects to file embedding/android/GeckoApp.java.rej
patching file embedding/android/Makefile.in
Hunk #1 FAILED at 137
1 out of 1 hunks FAILED -- saving rejects to file embedding/android/Makefile.in.rej
patching file embedding/android/Tab.java
Hunk #1 FAILED at 48
Hunk #2 FAILED at 90
Hunk #3 FAILED at 207
3 out of 3 hunks FAILED -- saving rejects to file embedding/android/Tab.java.rej
file embedding/android/resources/layout/doorhanger.xml already exists
1 out of 1 hunks FAILED -- saving rejects to file embedding/android/resources/layout/doorhanger.xml.rej
patching file embedding/android/resources/layout/doorhangerpopup.xml
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file embedding/android/resources/layout/doorhangerpopup.xml.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh attachment.cgi?id=572522
Comment 30 :Margaret Leibovic 2011-11-11 09:39:21 PST
I only got one small merge conflict. Doug, maybe you tried to apply the patches that already landed?

Sriram, I'll land this for you so that I can update my patches.
Comment 31 :Margaret Leibovic 2011-11-11 09:45:40 PST
https://hg.mozilla.org/projects/birch/rev/afe02e3fdc59

I'm marking this bug as fixed, since future work will happen in follow-ups.
Comment 32 Aaron Train [:aaronmt] 2011-11-14 06:50:00 PST
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)

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