Closed Bug 601751 Opened 10 years ago Closed 10 years ago

Redesign the Edit Bookmark Panel to have the new look

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(fennec2.0b4+)

VERIFIED FIXED
Tracking Status
fennec 2.0b4+ ---

People

(Reporter: nhirata, Assigned: mfinkle)

Details

(Whiteboard: [strings])

Attachments

(1 file, 9 obsolete files)

1. go to the control side panel
2. click on the star icon

Actual : see attachment.

Note: 
1. alignment and arrows were discussed in irc:
mfinkle: madhava: centered?
mfinkle: or still positioned relativ to the button
madhava: I would have thought aligned to the button
madhava: with a > pointing to the button
Attached image linux screenshot (obsolete) —
screenshot : courtesy of mfinkle
tracking-fennec: --- → ?
Madhava - can we get a new design?
tracking-fennec: ? → 2.0+
Summary: Please consider redesigning the Favorite Panel to have the chic look → Redesign the Bookmark Panel to have the new look
Assignee: nobody → madhava
tracking-fennec: 2.0+ → 2.0b3+
The bookmark popup already have the look described in #c0, bugmorphing to restyle the "Edit Bookmark Panel" that appears once you've clicked "Edit" in the popup:
 - it looks deprecated with the new style now
 - textboxes background should be gradient
Summary: Redesign the Bookmark Panel to have the new look → Redesign the Edit Bookmark Panel to have the new look
Maybe it should be an arrow panel too?

Madhava, help us!
yes - agreed.  Here were my quick notes from the other week:
http://www.flickr.com/photos/madhava_work/5121132829/
Attachment #480746 - Attachment is obsolete: true
Madhava - I plan to make this dialog look like the Larry panel except the arrow would point to the "star". Is that OK?
yes please.
Assignee: madhava → mark.finkle
Attached patch patch (obsolete) — Splinter Review
This patch converts the bookmark editor dialog into an arrow panel. It also adds some "resize" friendly code to the binding (removing some "resize" code already in the app).

I removed the "star" image mainly for space in portrait.

Will add screenshots
Attachment #488896 - Flags: review?(mbrubeck)
Attached image screenshot portrait (obsolete) —
Attached image screenshot landscape (obsolete) —
Attached image screenshot landscape (obsolete) —
real landscape screenshot
Attachment #488898 - Attachment is obsolete: true
Comment on attachment 488896 [details] [diff] [review]
patch

Code looks good.  Not sure about the transition from the bookmark-popup with its horizontal arrow to the bookmark-container with its vertical arrow.  Maybe we should move bookmark-popup so the arrow stays in the same place.
Attachment #488896 - Flags: review?(mbrubeck) → review+
tracking-fennec: 2.0b3+ → 2.0b4+
Attached patch patch (obsolete) — Splinter Review
This patch moves to a dialog look and feel. The current patch converts the bookmark editor and the sync setup.

Things this patch needs:
* Maybe remove the "star" in the bookmark editor - it looks weird
* Tapping outside the bookmark editor closes it. Tapping outside the syncsetup does not.
  * Bookmark dialog might be fine like this
  * Sync setup should probably have a modal block behind it
Attachment #488896 - Attachment is obsolete: true
Attachment #488897 - Attachment is obsolete: true
Attachment #488900 - Attachment is obsolete: true
Attachment #503750 - Flags: feedback?(21)
Note: The Sync dialog inner body is scrollable. The bookmark dialog is currently not scrollable, but could be.
Attached image bookmark dialog (obsolete) —
Attached image sync dialog - jpake (obsolete) —
Attached image sync dialog - manual (obsolete) —
Is there some reason these don't match the native dialogs shown here:

http://www.flickr.com/photos/madhava_work/5145131220/

Or does this match native dialogs in some other android distribution?
Attached patch patch 2Splinter Review
* Updated from feedback from Madhava.
* These are both dialogs, with modal-block backgrounds and CSS based centering
* Removed the "star" from bookmark dialog
* Used a bigger font for title and smaller font for the descriptions (but not the widgets)

* Removed unneeded code from BookmarkHelper
* Had to add a hacky wait loop for XBL to be ready (we load faster now)

* Fixed an error from bug 609940

We should unify the CSS in a followup bug - rolling the prompt system into it as well.
Attachment #503750 - Attachment is obsolete: true
Attachment #503751 - Attachment is obsolete: true
Attachment #503752 - Attachment is obsolete: true
Attachment #503753 - Attachment is obsolete: true
Attachment #503990 - Flags: review?(mbrubeck)
Attachment #503750 - Flags: feedback?(21)
Comment on attachment 503990 [details] [diff] [review]
patch 2

Madhava requested on IRC that the new-style sync setup dialog should not use the Personas background.  We could fix this in the Personas add-on, but Fabrice points out it would require a wait for the add-on review, and would change the behavior for 4.0b3 users.  Instead, we could fix this in this patch by renaming the #syncsetup-jpake and #syncsetup-manual elements (maybe to #syncsetup-simple and #syncsetup-fallback).  Your call.
Attachment #503990 - Flags: review?(mbrubeck) → review+
Comment on attachment 503990 [details] [diff] [review]
patch 2

>diff --git a/chrome/content/BookmarkHelper.js b/chrome/content/BookmarkHelper.js

>+    // Wait for XBL to initialize the widget
>+    setTimeout(function(self) {
>+      let ready = false;
>+      while (!ready) {
>+        try {
>+          self._editor.startEditing();
>+          ready = true;
>+        } catch(e) {}
>+      }
>+    }, 0, this);
>   },

Small nit: We could remove the comment and do a cleaner code

let function = waitForXBLInit(self) {
...
}
setTimeout(waitForXBLInit, 0, this);

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>@@ -1913,17 +1913,16 @@ IdentityHandler.prototype = {
>     // dismiss any dialog which hide the identity popup
>     BrowserUI.activePanel = null;
>     while (BrowserUI.activeDialog)
>       BrowserUI.activeDialog.close();
> 
>     this._identityPopup.hidden = false;
>     this._identityPopup.top = BrowserUI.toolbarH - this._identityPopup.offset;
>     this._identityPopup.anchorTo(this._identityBox);
>-    this._identityPopup.focus();

Just curious, why do you remove this call?

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+    <vbox id="bookmark-container" hidden="true" class="panel-dark window-width window-height">
>+      <vbox id="bookmark-dialog" class="panel-dark">
>+        <hbox id="bookmark-form-title">
>+          <description>&editBookmarkDialog.title;</description>
>+        </hbox>
>+        <separator id="bookmark-form-line"/>
>+        <scrollbox id="bookmark-form" align="start">
>+        </scrollbox>

Nit: scrollbox could be a one liner

>diff --git a/chrome/content/sync.js b/chrome/content/sync.js
>   openManual: function openManual() {
>     this.abortEasySetup();
> 
>     // Reset the scroll since the previous page might have been scrolled
>-    let scrollbox = document.getElementById("syncsetup-scrollbox").boxObject.QueryInterface(Ci.nsIScrollBoxObject);
>-    scrollbox.scrollTo(0, 0);
>+    let scrollboxes = document.getElementsByClassName("syncsetup-scrollbox");
>+    for (let i = 0; i < scrollboxes.length; i++) {
>+      let sbo = scrollboxes[i].boxObject.QueryInterface(Ci.nsIScrollBoxObject);
>+      try {
>+        sbo.scrollTo(0, 0);
>+      } catch(e) {}
>+    }
> 
> 
>   close: function close() {
>-    let scrollbox = document.getElementById("syncsetup-scrollbox").boxObject.QueryInterface(Ci.nsIScrollBoxObject);
>-    scrollbox.scrollTo(0, 0);
>+    // Reset the scroll since the previous page might have been scrolled
>+    let scrollboxes = document.getElementsByClassName("syncsetup-scrollbox");
>+    for (let i = 0; i < scrollboxes.length; i++) {
>+      let sbo = scrollboxes[i].boxObject.QueryInterface(Ci.nsIScrollBoxObject);
>+      try {
>+        sbo.scrollTo(0, 0);
>+      } catch(e) {}
>+    }

Nit: This is basically the same code, so it could worth a helper

Except that I like the new popup style so much!
(In reply to comment #21)
> Comment on attachment 503990 [details] [diff] [review]
> patch 2
> 
> Madhava requested on IRC that the new-style sync setup dialog should not use
> the Personas background.  We could fix this in the Personas add-on, but Fabrice
> points out it would require a wait for the add-on review, and would change the
> behavior for 4.0b3 users.  Instead, we could fix this in this patch by renaming
> the #syncsetup-jpake and #syncsetup-manual elements (maybe to #syncsetup-simple
> and #syncsetup-fallback).  Your call.

I made the ID changes and Fabrice will remove support for the old IDs from Personas too.

I also made the changes Vivien suggested too.

>>-    this._identityPopup.focus();

>Just curious, why do you remove this call?

Viven - I removed this call because we no longer seemed to need it and I don't like playing with focus unless we need too. If we find we need to change the focus, we can add it back.

I stumbled across this while comparing the BookmarkHelper code
Whiteboard: [strings]
pushed:
http://hg.mozilla.org/mobile-browser/rev/26ae3ffbf330
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b10pre) Gecko/20100118 Namoroka/4.0b10pre Fennec/4.0b4pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.