494491 - make edit bookmark more discoverable but still lightweight

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect)

Description

[Madhava Enros [:madhava]]

Attachment: quick path for showing that a bookmark's been created, and offering some options

see attachment.

the first window that comes up should have a similar style to the alerts that come up for manager announcements (download complete, etc.).

Comment 1

[Madhava Enros [:madhava]]

notes for requesting blocking -- this cleans up the bookmarking interaction a lot, and makes it closer to what we'd like to do in desktop firefox as well.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Madhava brought up some issues that I haven't yet addressed:
-remove "delete" and "move" from bookmark popup only (not from edit mode, though that will need to be reworked a bit too)
  - move buttons to the bottom (also not in edit mode)

That requires forking the "edit" widgets between the two, I think, which I didn't get into.

hideBookmarkPopup is a bit hacky - had to refer to BrowserUI directly since it's being called from the event handler directly, and added the try/catch since I don't take care of cancelling the timeout if the dialog is closed manually (so it might be called twice while the listener was added once). Wouldn't mind if that were cleaned up a bit :)

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 392781 [details] [diff] [review]
patch

>+  get canvasBox() {
>+    delete this.canvasBox;
>+    return this.canvasBox = document.getElementById("canvasbox");
>+  },
>+
>+  get bookmarkPopup() {
>+    delete this.bookmarkPopup;
>+    return this.bookmarkPopup = document.getElementById("bookmark-popup");
>+  },

I have been thinking that a common utility for holding DOM references might be useful. I'll file a bug on that.


>+  hideBookmarkPopup : function () {
>+    BrowserUI.bookmarkPopup.hidden = true;
>+    try {
>+      BrowserUI.canvasBox.removeEventListener("mousedown", this.hideBookmarkPopup, false);
>+    } catch (ex) {}
>+  },
>+  
>+  showBookmarkPopup : function (aAutoClose) {
>+    this.bookmarkPopup.hidden = false;
>+
>+    if (aAutoClose)
>+      setTimeout(this.hideBookmarkPopup, 2000);
>+
>+    this.canvasBox.addEventListener("mousedown", this.hideBookmarkPopup, false);
>+  },
>+

I guess it's not a big deal that the timeout can fire after a mousedown. We could clearTimeout if it was a problem. If we have more popups that get closed on a canvsBox mousedown, a more general purpose solution could be created.

Looks good so far. I have no problem with landing this as is and adding more patches.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

uses the new pushPopup thingy, much nicer

Comment 5

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mobile-browser/rev/d5371176339a

Filed bug 510969 for the edit popup followup.

Comment 6

[Stuart Parmenter]

Comment on attachment 394872 [details] [diff] [review]
updated patch

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
>--- a/chrome/content/browser-ui.js
>+++ b/chrome/content/browser-ui.js
>@@ -275,16 +275,29 @@ var BrowserUI = {

>+  
>+  get sidebarW() {
>+    if (!this._sidebarW) {
>+      let sidebar = document.getElementById("browser-controls");
>+      this._sidebarW = sidebar.boxObject.width;
>+    }
>+    return this._sidebarW;
>+  },
>+  

we can have multiple sidebars, and extension authors can add in other sidebars.  Getting the actual sidebar elements is bad.  What you really want here is to get the tile-container's right and the outer window's right and subtract them.


>+  get starButton() {
>+    delete this.starButton;
>+    return this.starButton = document.getElementById("tool-star");
>+  },
> 
>   _initControls : false,
>   sizeControls : function(windowW, windowH) {
>     let tabs = document.getElementById("tabs-container");
>     let controls = document.getElementById("browser-controls");
>     if (!this._initControls) {
>       this._initControls = true;
>     }
>@@ -310,16 +323,26 @@ var BrowserUI = {
>     // bookmark editor
>     let bmkeditor = document.getElementById("bookmark-container");
>     bmkeditor.width = windowW;
> 
>     // bookmark list
>     let bookmarks = document.getElementById("bookmarklist-container");
>     bookmarks.height = windowH;
>     bookmarks.width = windowW;
>+    
>+    // bookmark popup
>+    let starRect = this.starButton.getBoundingClientRect();
>+    let popupMargin = 10;

const?

>+    bookmarkPopup.top = starRect.top + popupMargin;

getBoundingClientRect returns floats -- you need to Math.round them.

>+    bookmarkPopup.left = windowW - this.sidebarW - bookmarkPopupW - popupMargin;

see above about sidebar position being wrong

>+var BookmarkPopup = {
>+  get box() {
>+    delete this.box;
>+    return this.box = document.getElementById("bookmark-popup");
>+  },
>+

huh??

>+  _bookmarkPopupTimeout: -1,
>+
>+  hide : function () {
>+    if (this._bookmarkPopupTimeout != -1) {
>+      clearTimeout(this._bookmarkPopupTimeout);
>+      this._bookmarkPopupTimeout = -1;
>+    }
>+    this.box.hidden = true;
>+  },

why do we need to get the box thing above every time?

>+  
>+  show : function (aAutoClose) {
>+    this.box.hidden = false;
>+
>+    if (aAutoClose) {
>+      this._bookmarkPopupTimeout = setTimeout(function (self) {
>+        self._bookmarkPopupTimeout = -1;
>+        self.hide();
>+      }, 2000, this);
>+    }
>+
>+    // include starButton here, so that click-to-dismiss works as expected
>+    BrowserUI.pushPopup(this, [this.box, BrowserUI.starButton]);
>+  },
>+  

why do we need to get the box thing above every time?

>+  toggle : function (aAutoClose) {
>+    if (this.box.hidden)
>+      this.show(aAutoClose);
>+    else
>+      this.hide();
>+  }
>+}
>+

why do we need to get the box thing above every time?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> we can have multiple sidebars, and extension authors can add in other sidebars.
>  Getting the actual sidebar elements is bad.  What you really want here is to
> get the tile-container's right and the outer window's right and subtract them.

This doesn't work right now, because the initial sizeControls is called before tile-container is properly sized.

> >+    let popupMargin = 10;
> 
> const?
> 
> >+    bookmarkPopup.top = starRect.top + popupMargin;
> 
> getBoundingClientRect returns floats -- you need to Math.round them.

https://hg.mozilla.org/mobile-browser/rev/f5240e6cc47c

> >+  get box() {
> >+    delete this.box;
> >+    return this.box = document.getElementById("bookmark-popup");
> >+  },
> >+
> 
> huh??

sorted this out on IRC (memoizing-getter)

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> This doesn't work right now, because the initial sizeControls is called before
> tile-container is properly sized.

bug 510981

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

verfied with beta3

Comment 10

[Aakash Desai [:aakashd]]

testcases are already in litmus for this bug:

https://litmus.mozilla.org/show_test.cgi?id=9848
https://litmus.mozilla.org/show_test.cgi?id=9789