Ux Designs for Doorhanger Notifications

VERIFIED FIXED

Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: elan, Assigned: sriram)

Tracking

unspecified
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(8 attachments, 8 obsolete attachments)

40.66 KB, image/png
Details
1.49 MB, application/zip
Details
27.58 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
3.69 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
141.65 KB, image/png
Details
1.22 MB, image/png
Details
3.04 KB, application/zip
Details
15.48 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Meta bug.
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
Assignee: madhava → sriram
Summary: Ux Designs for Alerts, Dialogs, and Notifications → Ux Designs for Doorhanger Notifications
Duplicate of this bug: 699772
Created attachment 571983 [details]
Door hanger spec
Created attachment 571984 [details]
Android 4 door hanger 9 slice
Created attachment 571985 [details]
Android 2.2/2.3 door hanger
Created attachment 571986 [details]
Door hanger spec
Attachment #571986 - Attachment is obsolete: true
Created attachment 571987 [details]
Door hanger spec
Attachment #571983 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
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.
Attachment #572073 - Flags: review?(mark.finkle)
(Assignee)

Comment 9

6 years ago
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).
Attachment #572085 - Flags: review?(mark.finkle)
(Assignee)

Comment 10

6 years ago
Patch 1.5/2 should be applied on top of Patch 1/2 :)
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
Created attachment 572109 [details]
DoorHanger with native theme in Droid Pro

This is with Droid Pro.
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
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 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
Attachment #572073 - Flags: review?(mark.finkle) → review+
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
Attachment #572085 - Flags: review?(mark.finkle) → review+
Created attachment 572468 [details]
9Slice with Layout Spec

Updated the 9slice for the door hanger, separate the arrow graphic from the 9slice.
Attachment #571984 - Attachment is obsolete: true
Attachment #571985 - Attachment is obsolete: true
Attachment #571987 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
Created attachment 572522 [details] [diff] [review]
Patch (1/2): Plumbing for showing on top + stacking

This tweaks the previous patch as required.
Attachment #572073 - Attachment is obsolete: true
Attachment #572522 - Flags: review?(mark.finkle)
(Assignee)

Comment 19

6 years ago
Created attachment 572523 [details] [diff] [review]
Patch (1.5/2): Avoiding flickering while closing a background tab

This tweaks the previous patch as required.
Attachment #572085 - Attachment is obsolete: true
Attachment #572523 - Flags: review?(mark.finkle)
(Assignee)

Comment 20

6 years ago
Created attachment 572538 [details]
DoorHanger with native theme in Nexus S

DoorHanger image updated.
Attachment #572108 - Attachment is obsolete: true
Attachment #572522 - Flags: review?(mark.finkle) → review+
Attachment #572523 - Flags: review?(mark.finkle) → review+
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
Created attachment 573006 [details]
Updated stacked door hangers
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.
(Assignee)

Comment 24

6 years ago
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.
Attachment #573338 - Flags: review?(mark.finkle)
The patches that had landed were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.

Updated

6 years ago
Blocks: 701305
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.
Attachment #573338 - Flags: review?(mark.finkle) → review+

Comment 27

6 years ago
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.
(Assignee)

Comment 28

6 years ago
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.
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

6 years ago
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

6 years ago
https://hg.mozilla.org/projects/birch/rev/afe02e3fdc59

I'm marking this bug as fixed, since future work will happen in follow-ups.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.