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

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.

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).

Patch 1.5/2 should be applied on top of Patch 1/2 :)

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.

This is with Droid Pro.

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.

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

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

Updated the 9slice for the door hanger, separate the arrow graphic from the 9slice.

This tweaks the previous patch as required.

This tweaks the previous patch as required.

DoorHanger image updated.

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

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.

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.

The patches that had landed were backed while investigating Talos failures. Now that tests are green again, we will need to reland.

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.

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.

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

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.

https://hg.mozilla.org/projects/birch/rev/afe02e3fdc59 I'm marking this bug as fixed, since future work will happen in follow-ups.

20111114041052 http://hg.mozilla.org/projects/birch/rev/859ecdfe0168 Samsung Galaxy SII (Android 2.3.4)