make edit bookmark more discoverable but still lightweight

VERIFIED FIXED

Status

Fennec Graveyard
Bookmarks
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: madhava, Assigned: Gavin)

Tracking

(Blocks: 1 bug)

Bug Flags:
wanted-fennec1.0 +
in-litmus +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 379259 [details]
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.).
(Reporter)

Updated

9 years ago
Blocks: 477628
(Reporter)

Updated

9 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → ---
Flags: wanted-fennec1.0+
(Reporter)

Comment 1

9 years ago
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.
tracking-fennec: --- → ?

Updated

9 years ago
tracking-fennec: ? → 1.0+
Assignee: nobody → gavin.sharp
Created attachment 392781 [details] [diff] [review]
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 :)
Attachment #392781 - Flags: review?(mark.finkle)
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.
Attachment #392781 - Flags: review?(mark.finkle) → review+
Created attachment 394872 [details] [diff] [review]
updated patch

uses the new pushPopup thingy, much nicer
Attachment #394872 - Flags: review?(mark.finkle)
Attachment #392781 - Attachment is obsolete: true
Attachment #394872 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mobile-browser/rev/d5371176339a

Filed bug 510969 for the edit popup followup.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Attachment #394872 - Flags: review+ → review-

Comment 6

9 years ago
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?
(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)
(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
Flags: in-litmus?
verfied with beta3
Status: RESOLVED → VERIFIED
Component: General → Bookmarks
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
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.