Closed
Bug 494491
Opened 16 years ago
Closed 15 years ago
make edit bookmark more discoverable but still lightweight
Categories
(Firefox for Android Graveyard :: Bookmarks, defect)
Firefox for Android Graveyard
Bookmarks
Tracking
(fennec1.0+)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
fennec | 1.0+ | --- |
People
(Reporter: madhava, Assigned: Gavin)
References
Details
Attachments
(2 files, 1 obsolete file)
334.08 KB,
image/jpeg
|
Details | |
9.83 KB,
patch
|
pavlov
:
review-
|
Details | Diff | Splinter Review |
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•16 years ago
|
tracking-fennec: --- → ?
Updated•16 years ago
|
tracking-fennec: ? → ---
Flags: wanted-fennec1.0+
Reporter | ||
Comment 1•15 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•15 years ago
|
tracking-fennec: ? → 1.0+
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → gavin.sharp
Assignee | ||
Comment 2•15 years ago
|
||
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 3•15 years ago
|
||
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+
Assignee | ||
Comment 4•15 years ago
|
||
uses the new pushPopup thingy, much nicer
Attachment #394872 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•15 years ago
|
Attachment #392781 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #394872 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 5•15 years ago
|
||
https://hg.mozilla.org/mobile-browser/rev/d5371176339a
Filed bug 510969 for the edit popup followup.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Attachment #394872 -
Flags: review+ → review-
Comment 6•15 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?
Assignee | ||
Comment 7•15 years ago
|
||
(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)
Assignee | ||
Comment 8•15 years ago
|
||
(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
Updated•15 years ago
|
Flags: in-litmus?
Updated•15 years ago
|
Component: General → Bookmarks
Comment 10•15 years ago
|
||
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.
Description
•