Closed
Bug 698598
Opened 13 years ago
Closed 13 years ago
Ux Designs for Doorhanger Notifications
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox11 fixed, fennec11+)
VERIFIED
FIXED
People
(Reporter: elan, Assigned: sriram)
References
Details
Attachments
(8 files, 8 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
Meta bug.
Comment 1•13 years ago
|
||
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
Comment 3•13 years ago
|
||
Comment 4•13 years ago
|
||
Comment 5•13 years ago
|
||
Comment 6•13 years ago
|
||
Updated•13 years ago
|
Attachment #571986 -
Attachment is obsolete: true
Comment 7•13 years ago
|
||
Attachment #571983 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Patch 1.5/2 should be applied on top of Patch 1/2 :)
Assignee | ||
Comment 11•13 years ago
|
||
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•13 years ago
|
||
This is with Droid Pro.
Assignee | ||
Comment 13•13 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•13 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 15•13 years ago
|
||
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 16•13 years ago
|
||
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+
Comment 17•13 years ago
|
||
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•13 years ago
|
||
This tweaks the previous patch as required.
Attachment #572073 -
Attachment is obsolete: true
Attachment #572522 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 19•13 years ago
|
||
This tweaks the previous patch as required.
Attachment #572085 -
Attachment is obsolete: true
Attachment #572523 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 20•13 years ago
|
||
DoorHanger image updated.
Attachment #572108 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #572522 -
Flags: review?(mark.finkle) → review+
Updated•13 years ago
|
Attachment #572523 -
Flags: review?(mark.finkle) → review+
Comment 21•13 years ago
|
||
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•13 years ago
|
||
Comment 23•13 years ago
|
||
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•13 years ago
|
||
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)
Comment 25•13 years ago
|
||
The patches that had landed were backed while investigating Talos failures. Now that tests are green again, we will need to reland.
Comment 26•13 years ago
|
||
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•13 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•13 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.
Comment 29•13 years ago
|
||
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•13 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•13 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
Closed: 13 years ago
Resolution: --- → FIXED
Comment 32•13 years ago
|
||
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
Updated•13 years ago
|
tracking-fennec: --- → 11+
Updated•13 years ago
|
status-firefox11:
--- → fixed
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•