Closed Bug 1219794 Opened 9 years ago Closed 9 years ago

Show the edit bookmark overlay and indicate that a bookmark has been made when clicking the star

Categories

(Firefox :: Bookmarks & History, defect, P2)

Product:
Firefox
Component:
Bookmarks & History
Platform:
All
Unspecified
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: verdi, Assigned: jaws)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [Onboarding])

Attachments

(2 files, 4 obsolete files)

bookmark-overlay.png
33.58 KB, image/png
Details
Patch for check-in
30.15 KB, patch
Details | Diff | Splinter Review
Attached image bm-editpanel-newstring.png (obsolete) —
To reduce confusion and surface the ability to file and organize bookmarks, we should show the edit bookmark overlay on the first click of the star instead of the second. In addition we should change the headline string from "Edit This Bookmark" to "Page Bookmarked!"
I think we need a global overhaul of the bookmarking experience that is currently not on par, this change would just throw away advantages of one click bookmarking, with a small gain overall.
Blocks: 1219810
(In reply to Marco Bonardo [::mak] from comment #1) > I think we need a global overhaul of the bookmarking experience that is > currently not on par, this change would just throw away advantages of one > click bookmarking, with a small gain overall. I agree that we do need an overhaul but we shouldn't let that keep us for making things better in the short term. This still let's people make a bookmark with 1 click because they aren't required to interact with the overlay. And, in tests, the overlay seems to be helpful for letting people know what has just happened as a result of clicking the star. If the advantages you're talking about is interaction between bookmarks and the awesome bar (my favorite Firefox feature) then from what I've seen while observing users, is that there isn't anything about the bookmarking process that lets someone know that they can find their bookmark again by typing in the address field. When we eventually do an overhaul it would be great if we can come up with a way to make the features of the awesomebar more discoverable.
(In reply to Verdi [:verdi] from comment #2) > the overlay seems to be helpful for letting people > know what has just happened as a result of clicking the star. The star animation had this scope. The problem is that the bookmarks menu doesn't show the just added bookmark. > If the advantages you're talking about is interaction between bookmarks and > the awesome bar (my favorite Firefox feature) then from what I've seen while > observing users, is that there isn't anything about the bookmarking process > that lets someone know that they can find their bookmark again by typing in > the address field. When we eventually do an overhaul it would be great if we > can come up with a way to make the features of the awesomebar more > discoverable. We had various design sessions on bookmarks, but until we start a Quality Effort on them, I fear nothing will happen, cause we are under-resourced at the moment (basically nobody is assigned to officially work on Places, I'm just keeping the bugs in order as the de-facto owner).
I just played with the Pocket panel a bit: it does this nice thing where it disappears automatically after a few seconds (unless the mouse is moved over the panel), which would be a good behavior for bookmarks as well when we show panel on first click. For some reason the Pocket panel doesn't animate out though (which it should totally do).
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #4) > I just played with the Pocket panel a bit: it does this nice thing where it > disappears automatically after a few seconds (unless the mouse is moved over > the panel), which would be a good behavior for bookmarks as well when we > show panel on first click. I agree. Let's do this: * User clicks the star to bookmark a page. * Star animation fires and we animate in the bookmark overlay (using the current animation) at the same time. * Bookmark overlay says "Page Bookmarked!" instead of "Edit This Bookmark" (see attachment 8680673 [details]). * Close the bookmark overlay 3 seconds after it's shown unless the user clicks into Name, Folder or Tags section. ** If the user clicks Remove Bookmark, Done or Cancel, immediately close the overlay. > For some reason the Pocket panel doesn't animate > out though (which it should totally do). It looks like all of our panels, including Pocket, have a very quick fade out. I think we should just continue to use that for now. Thinking of how the Pocket panel works, we could consider changing the "Page Bookmarked!" string to "Bookmark Removed" when the user deletes the bookmark and then we'd have to keep the panel open for a second or two so they have time to see that. On the other hand, the star gets filled in when you create a bookmark and then unfilled when you delete and that's probably sufficient in this case.
Priority: -- → P3
Whiteboard: [Onboarding]
Priority: P3 → P1
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8711221 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8711221 [details] MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak https://reviewboard.mozilla.org/r/32023/#review28847 This isn't quite right, but more generally, please get the next review off ::mak . I don't know this code very well, and though I could study it in order to try to review this, it seems pretty plausible he would still be a better reviewer. :-) ::: toolkit/content/widgets/popup.xml:470 (Diff revision 1) > + let prefName = "browser.bookmarks.closePanelQuickForTesting"; r- for this. If we need to do this, let's put a `<field>` in the binding and set that based on the pref when we open the thing, but let's not use a browser and popup-specific pref in a toolkit popup binding.
Comment on attachment 8711221 [details] MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32023/diff/1-2/
Attachment #8711221 - Attachment description: MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?gijs → MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak
Attachment #8711221 - Flags: review?(mak77)
There's one thing that is unclear yet, the Cancel behavior. For example see the difference between: 1. double clicking the star: * Cancel just closes the panel, as well as OK. * We have a Remove button to remove the bookmark 2. CTRL+D * There's no Remove button * Cancel is actually canceling the bookmarking operation and removing the just created bookmark. * Can use ESC to cancel bookmarking and remove the bookmark. If we always show the panel and a Remove button, having both Cancel and OK sounds redundant. What should we keep? A. Remove both Ok and Cancel. The upside is that this is more coherent with other panels. The downside is that it will be more annoying to cancel unwanted bookmarking actions. For example often I wrongly click CTRL+D instead of CTRL+F, currently I can just ESC or Cancel, in the new world I should click on Remove Bookmark (we should keep supporting ESC though). B. Rename Cancel to "Remove Bookmark" and remove the button in the header. Makes remove bookmark less visible, but the user has 2 clear path of actions (ok or remove) I'm asking specifically because the mock-up and the current patch are ignoring this problem, and I don't think it's ignorable, since Cancel sometimes removes the bookmark, sometimes not. Plus I don't think that making Cancel always remove the bookmark is correct, since it's easy for a user to click on it just to dismiss the panel and he would be surprised to see the bookmark go away.
Flags: needinfo?(mverdi)
Jared, there are some edge-cases that should be tested if we change the panel contents: 1. double click on the star. should not flicker and work like a single click 2. slow double click on the star (so it's not a double-click event), should not flicker as well 2. CTRL+D 3. contextual menu star button 4. bookmark menu "bookmark this link" Title should be Page Bookmarked! or Edit bookmark depending on whether the page was bookmarked already. It should be possible to cancel the bookmarking operation with ESC only if the page was not bookmarked already. I think first we need an answer to comment 10 and then it's possible to check we are not breaking those cases.
Attachment #8711221 - Flags: review?(mak77)
Comment on attachment 8711221 [details] MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak https://reviewboard.mozilla.org/r/32023/#review29347 ::: browser/base/content/browser-places.js:112 (Diff revision 2) > + this.panel.removeAttribute("fade"); shouldn't this happen before the defaultPrevented check? ::: browser/base/content/browser-places.js:205 (Diff revision 2) > + fastFade = Services.prefs.getBoolPref("closePanelQuickForTesting"); I don't think this works at all since you are reading a different pref from what the test is setting? ::: browser/base/content/browser-places.js (Diff revision 2) > - this._element("editBookmarkPanelRemoveButton").hidden = this._batching; Doing this depends on comment 10, more specifically currently we hide the remove bookmark button when Cancel or ESC are already doing that, to avoid confusing the user. ::: browser/base/content/browser-places.js:1672 (Diff revision 2) > - PlacesCommandHook.bookmarkCurrentPage(isBookmarked); > + PlacesCommandHook.bookmarkCurrentPage(true); Looks like a few lines above the "closemenu" attribute does not really depend on whether the url is bookmarked, but on whether we will show the edit panel. Since we always want to open it now, sounds like we can remove the if/else from there and always remove the "closemenu" attribute. ::: toolkit/content/widgets/popup.xml:472 (Diff revision 2) > + this.hidePopup(true); I'm not sure it's a good idea making a "fade" attribute control whether the popup hides or not... Maybe we should preventDefault in popuphiding instead?
Attached image bookmark-overlay.png —
(In reply to Marco Bonardo [::mak] from comment #10) > There's one thing that is unclear yet, the Cancel behavior. > > For example see the difference between: > > 1. double clicking the star: > * Cancel just closes the panel, as well as OK. > * We have a Remove button to remove the bookmark > > 2. CTRL+D > * There's no Remove button > * Cancel is actually canceling the bookmarking operation and removing the > just created bookmark. > * Can use ESC to cancel bookmarking and remove the bookmark. > > If we always show the panel and a Remove button, having both Cancel and OK > sounds redundant. What should we keep? > > A. Remove both Ok and Cancel. The upside is that this is more coherent with > other panels. The downside is that it will be more annoying to cancel > unwanted bookmarking actions. For example often I wrongly click CTRL+D > instead of CTRL+F, currently I can just ESC or Cancel, in the new world I > should click on Remove Bookmark (we should keep supporting ESC though). > > B. Rename Cancel to "Remove Bookmark" and remove the button in the header. > Makes remove bookmark less visible, but the user has 2 clear path of actions > (ok or remove) > > I'm asking specifically because the mock-up and the current patch are > ignoring this problem, and I don't think it's ignorable, since Cancel > sometimes removes the bookmark, sometimes not. Plus I don't think that > making Cancel always remove the bookmark is correct, since it's easy for a > user to click on it just to dismiss the panel and he would be surprised to > see the bookmark go away. All very good points. If this isn't increasing the scope of this bug too much, I think we could replace the cancel button with the remove bookmark button (B above) and unite the interactions for clicking the star and using Ctrl+D. I've attached a new mockup that does that. The interactions would now be: * User clicks the star or uses Ctrl+D and the panel opens and remains for 3 seconds unless they interact with it * Panel says "Page Bookmarked!" instead of "Edit This Bookmark" * Clicking Done saves edits and/or dismisses the panel * Clicking Remove Bookmark, removes the bookmark * Hitting Esc always dismisses the panel and never removes the bookmark * Hitting Enter performs the default action, i.e. Done The difference here with what you suggested is how we handle Esc. Normally I don't think anyone expects Esc to do an undo - that would be Ctrl+Z. Now we'd always show the same overlay and Esc would always work the same way. And the user still has a way to use the keyboard to remove the bookmark - Alt+R as indicated on the button.
Attachment #8680673 - Attachment is obsolete: true
Flags: needinfo?(mverdi)
(In reply to Verdi [:verdi] from comment #13) > * User clicks the star or uses Ctrl+D and the panel opens and remains for 3 > seconds unless they interact with it > * Panel says "Page Bookmarked!" instead of "Edit This Bookmark" Provided the page is not already bookmarked, I assume. I agree it makes sense to unify the experience and have a common interaction (ok, remove bookmark).
(In reply to Marco Bonardo [::mak] from comment #14) > > * Panel says "Page Bookmarked!" instead of "Edit This Bookmark" > > Provided the page is not already bookmarked, I assume. > Yes that's correct. If the page is already bookmarked it should continue to say "Edit this Bookmark."
Attached patch Patch v2 (obsolete) — — Splinter Review
Attachment #8711221 - Attachment is obsolete: true
Attachment #8713460 - Flags: review?(mak77)
Comment on attachment 8713460 [details] [diff] [review] Patch v2 Review of attachment 8713460 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_bookmark_popup.js @@ +97,5 @@ > + yield test_bookmarks_popup({ > + isNewBookmark: true, > + popupShowFn() { > + EventUtils.synthesizeMouse(bookmarkStar, 10, 10, window); > + let now = Date.now(); This line can be deleted.
Comment on attachment 8713460 [details] [diff] [review] Patch v2 Review of attachment 8713460 [details] [diff] [review]: ----------------------------------------------------------------- I didn't test this yet, I plan to do that at the next iteration. The global approach seems fine, but there's some details that may cause regressions. One thing we didn't consider yet, is the transaction manager, users can undo/redo bookmarks actions in the Library, the panel is trying to be nice to them by batching operations in a single undo/redo step, and some of the changes in this patch may break that. ::: browser/base/content/browser-places.js @@ +22,5 @@ > + let prefName = "browser.bookmarks.closePanelQuickForTesting"; > + prefVal = Services.prefs.getBoolPref(prefName); > + } catch (ex) {} > + delete this._closePanelQuickForTesting; > + return this._closePanelQuickForTesting = prefVal; By caching this, you make so it can't be changed at runtime. That means once a browser-chrome test access this, all of the tests in the same window will use the first read value. I don't think it's going to work. Honestly at this point I don't see why we couldn't just set a private _closePanelQuickForTesting property from the test itself, you can access the StarUI object from there... It would be faster and it wouldn't really break anything, even if something else touches it, it's a simple bool. The alternative doesn't sound great, you'd need a pref observer and all the supporting code. @@ +115,5 @@ > .transact().catch(Cu.reportError); > break; > } > } > this._actionOnHide = ""; I think you should change _actionOnHide for the "cancel" action to not undo the transaction? Ideally now that only remove when the remove button is pressed, this code could be likely simplified by removing _actionOnHide completely and moving the remove code to be executed when the remove button is ativated... but could also happen in a later cleanup. @@ +146,5 @@ > break; > + case "popupshown": > + // auto-close if new and not interacted with > + if (this._isNewBookmark) { > + let delay = 4000; do we have UX signoff on this value? is it coherent with other popups we have? I honestly think it's a little too long (I'd pick 3000 or 3500), but it's just my opinion and I'm far from being a ux expert :) @@ +207,5 @@ > > _doShowEditBookmarkPanel: Task.async(function* (aNode, aAnchorElement, aPosition) { > if (this.panel.state != "closed") > return; > + clearTimeout(this._autoCloseTimer); why do we need this? are we not already clearing on popuphidden? is this just overzealous? @@ +354,1 @@ > if (itemId == -1) { could reuse the temp var in the if condition @@ -345,5 @@ > - if (aShowEditUI) { > - // If we bookmark the page here (i.e. page was not "starred" already) > - // but open right into the "edit" state, start batching here, so > - // "Cancel" in that state removes the bookmark. > - StarUI.beginBatch(); Why did you remove both calls to beginBatch()? The reasons for it to exist are 2: 1. The cancel button in the old world has to revert all the changes. In the new world this doesn't happen, so it would be fine to remove it but... 2. you should be able to UNDO a bookmark addition as a single operation from the Library undo menuitem (or ctrl+z). In that case, without a batch, you should undo every single operation (change title, change parent, ..., add bookmark). Maybe we could change the condition from aShowEditUI to aShowEditUI && isNewBookmark? When editing it may be fine to have multiple undo steps. Or we should just leave it as-is and retain both add and edit as a single undo step. It depends on the reasons this was removed first. @@ -420,5 @@ > - if (aShowEditUI) { > - // If we bookmark the page here (i.e. page was not "starred" already) > - // but open right into the "edit" state, start batching here, so > - // "Cancel" in that state removes the bookmark. > - StarUI.beginBatch(); ditto ::: browser/base/content/browser.xul @@ -180,5 @@ > <vbox> > <label id="editBookmarkPanelTitle"/> > <description id="editBookmarkPanelDescription"/> > - <hbox> > - <button id="editBookmarkPanelRemoveButton" browser_bug432599.js may need some tweaks now that we don't hide/unhide the remove bookmarks button @@ -205,2 @@ > class="editBookmarkPanelBottomButton" > - label="&editBookmark.cancel.label;" sounds like this label can be removed from browser.dtd ::: browser/base/content/test/general/browser_bookmark_popup.js @@ +17,5 @@ > +function* test_bookmarks_popup({isNewBookmark, popupShowFn, popupEditFn, > + shouldAutoClose, popupHideFn, isBookmarkRemoved}) { > + try { > + if (!isNewBookmark) { > + PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId, Please use the new async bookmarking API (we should in all new code, unless it's a block of browser code that has not yet been updated, like browser-places): yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, url: "about:home", title: "Home page" }); @@ +34,5 @@ > + > + if (popupEditFn) { > + yield popupEditFn(); > + } > + let bookmarks = PlacesUtils.getBookmarksForURI(gBrowser.currentURI); if you just want to check if it's bookmarked (most recent bookmark): let bm = yield PlacesUtils.bookmarks.fetch({ url: gBrowser.currentURI }); if you want to get all bookmarks for an url: let bms = []; yield PlacesUtils.bookmarks.fetch({ url: gBrowser.currentURI }, bm => bms.push(bm)); @@ +35,5 @@ > + if (popupEditFn) { > + yield popupEditFn(); > + } > + let bookmarks = PlacesUtils.getBookmarksForURI(gBrowser.currentURI); > + is(bookmarks.length, 1, "Only one bookmark should be created"); "should exist" since we don't always create a bookmark here. @@ +46,5 @@ > + "title should match isEditingBookmark state"); > + > + if (!shouldAutoClose) { > + yield new Promise(resolve => { > + setTimeout(resolve, 800); sounds like a lot of time to wait for each test.. can we reduce it further? for now there are 4 tests using this, that amounts to more than 3 seconds doing nothing, more tests could be added in future. I think we should try to not wait more than 500ms. @@ +55,5 @@ > + yield popupHideFn(); > + } > + yield promisePopupHidden(bookmarkPanel); > + is(bookmarkStar.hasAttribute("starred"), !isBookmarkRemoved, > + "Page is still starred after closing"); remove "still" since depends on the test @@ +56,5 @@ > + } > + yield promisePopupHidden(bookmarkPanel); > + is(bookmarkStar.hasAttribute("starred"), !isBookmarkRemoved, > + "Page is still starred after closing"); > + gBrowser.removeTab(tab); add the tab before the try and move this to finally? @@ +58,5 @@ > + is(bookmarkStar.hasAttribute("starred"), !isBookmarkRemoved, > + "Page is still starred after closing"); > + gBrowser.removeTab(tab); > + } catch (ex) { > + ok(false, ex); what's the point of this? if there's an exception the yield in add_task will reject and the test will fail already. @@ +60,5 @@ > + gBrowser.removeTab(tab); > + } catch (ex) { > + ok(false, ex); > + } finally { > + let id = PlacesUtils.getMostRecentBookmarkForURI(PlacesUtils._uri("about:home")); Ditto about new API @@ +64,5 @@ > + let id = PlacesUtils.getMostRecentBookmarkForURI(PlacesUtils._uri("about:home")); > + is(id > 0, !isBookmarkRemoved, > + "bookmark should not be present if a panel action should've removed it"); > + if (id > 0) { > + PlacesUtils.bookmarks.removeItem(id); yield PlacesUtils.bookmarks.remove(...) here you can directly pass a bookmark object you got from another API call, or a guid or an object like {url: ...} to remove all bookmarks for that url @@ +95,5 @@ > + > +add_task(function* panel_shown_once_for_slow_doubleclick_on_new_bookmark_star_and_autocloses() { > + yield test_bookmarks_popup({ > + isNewBookmark: true, > + popupShowFn() { should be *popupShownFn() @@ +167,5 @@ > + > +add_task(function* contextmenu_new_bookmark_click_no_autoclose() { > + yield test_bookmarks_popup({ > + isNewBookmark: true, > + popupShowFn(browser) { should be *popupShownFn @@ +222,5 @@ > + }); > +}); > + > +add_task(function* cleanup() { > + Services.prefs.clearUserPref("browser.bookmarks.closePanelQuickForTesting"); better do this in a registerCleanupFunction
Attachment #8713460 - Flags: review?(mak77)
Attached patch Patch v3 (obsolete) — — Splinter Review
Thanks for the review comments. I initially removed the beginBatch() calls because I didn't think they would be necessary now that we removed the "Cancel" from the bookmark star popup, but I didn't think about accessing this from the Library. I changed the duration of the popup to 3500ms to match the duration of the Pocket popup, and I also reduced the test delay to 400ms.
Attachment #8713460 - Attachment is obsolete: true
Attachment #8716479 - Flags: review?(mak77)
Comment on attachment 8716479 [details] [diff] [review] Patch v3 Review of attachment 8716479 [details] [diff] [review]: ----------------------------------------------------------------- I tried to test the patch but for some reason the first times I tried to bookmark things the star didn't color and the panel didn't open. I couldn't figure out the reason yet, it's intermittent. Regardless seems like it take a huge amount of time from when I click the star to when the panel is shown, like it's waiting for the whole star animation. The other problem is that when the use notices the panel and starts to mouse move towards one of the fields, it closes. I don't think there's a long enough timeout for this, some users are quite slow, the user could be thinking about a good name or about where to put the bookmark. The current timer is enough to move the mouse over the panel, but not enough to interact with it. Maybe we should stop closing the panel when the mouse moves over it, and then start the timer again once it moves out of it? handling mouse clicks doesn't look enough. ::: browser/base/content/browser-places.js @@ +79,5 @@ > this._itemId = -1; > if (this._batching) > this.endBatch(); > > + if (this._uriForRemoval) { we can probably be nicer to the user here. The current version of the patch adds a bookmark, and then here it removes it if the user clicks on the remove bookmark button. That creates an "add bookmark" and a "remove bookmark" entries in the undo/redo history. Instead we likely want the remove bookmark transaction only if the bookmark has not been just added, otherwise we just want to undo the bookmark creation transaction we just committed. I'd suggest to further split this if based on _isNewBookmark, something like: if (this._uriForRemoval) { if (this._isNewBookmark) { undoTransaction... } else { remove... } the undoTransaction code is the one that was defined before for the "cancel" case @@ +107,3 @@ > switch (aEvent.keyCode) { > case KeyEvent.DOM_VK_ESCAPE: > + this.panel.hidePopup(); Is this really needed now or will the panel code already do that? I'm asking cause I see ESC works on other panels like downloads or the menu panel, so could be we don't need to handle it at all now that it's the default action. @@ +346,1 @@ > // "Cancel" in that state removes the bookmark. this comment is now wrong, canel won't remove the bookmark, but we still want to batch to group related transactions into a single undo step. @@ +422,1 @@ > // "Cancel" in that state removes the bookmark. ditto
Attachment #8716479 - Flags: review?(mak77)
the opening problem is strange, if I click and don't move the mouse at all, the panel doesn't open, then if I move the mouse out the button and then back to it, it opens. I wonder if it's a different regression...
CTRL+D works properly instead.
Attachment #8716479 - Attachment is obsolete: true
Attachment #8711221 - Attachment is obsolete: false
Attachment #8711221 - Flags: review?(mak77)
Comment on attachment 8711221 [details] MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32023/diff/2-3/
Comment on attachment 8711221 [details] MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak https://reviewboard.mozilla.org/r/32023/#review31915 Did you already get a Try run to ensure these changes are not breaking other b-c tests? ::: browser/base/content/browser-places.js:28 (Diff revision 3) > + element.addEventListener("mouseleave", this, false); I just noticed these warnings: [Parent 21644] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file e:/mozilla/src/dom/events/EventListenerManager.cpp, line 393 [Parent 21644] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file e:/mozilla/src/dom/events/EventListenerManager.cpp, line 393 ::: browser/base/content/browser-places.js:103 (Diff revision 3) > - PlacesTransactions.RemoveBookmarksForUrls(this._uriForRemoval) > + PlacesTransactions.RemoveBookmarksForUrls(this._uriForRemoval) I think this is wrong, we should not run both asyncTransactions and legacy transactions code, only one should run, but you removed the break inside the if (!PlacesUIUtils.useAsyncTransactions) branch. to clarify, async transactions are the future replacement for transactions, but they are not ready yet, so they are behind the PlacesUIUtils.useAsyncTransactions checks for now. ::: browser/base/content/browser-places.js:103 (Diff revision 3) > - PlacesTransactions.RemoveBookmarksForUrls(this._uriForRemoval) > + PlacesTransactions.RemoveBookmarksForUrls(this._uriForRemoval) while here, please fix the argument, RemoveBookmarksForUrls takes an array, not a single url.
Attachment #8711221 - Flags: review?(mak77)
Comment on attachment 8711221 [details] MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32023/diff/3-4/
Attachment #8711221 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #18) > ::: browser/base/content/browser.xul > @@ -180,5 @@ > > <vbox> > > <label id="editBookmarkPanelTitle"/> > > <description id="editBookmarkPanelDescription"/> > > - <hbox> > > - <button id="editBookmarkPanelRemoveButton" > > browser_bug432599.js may need some tweaks now that we don't hide/unhide the > remove bookmarks button No errors with that test when run locally. Results from `mach mochitest browser/base/content/test/general`: 0 INFO TEST-START | Shutdown 1 INFO Passed: 3054 2 INFO Failed: 0 3 INFO Todo: 4
Comment on attachment 8711221 [details] MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32023/diff/4-5/
Attachment #8711221 - Flags: review?(mak77) → review+
Comment on attachment 8711221 [details] MozReview Request: Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r?mak https://reviewboard.mozilla.org/r/32023/#review32073 OK, let's proceed and see how users handle this, we can tweak their feedback in follow-ups, I can cc you in new bugs as I see them coming. ::: browser/base/content/browser-places.js:137 (Diff revisions 3 - 5) > - break; > + break; you are breaking out of the while here, I suppose the idea was to break out of the case instead? Otherwise we'd always fall-through. Plus, I think you meant to use relatedTarget, cause target is always the element we moused out, that will be == this.panel also for the panel itself. I guess something like: let target = event.relatedTarget; while (target && target != event.currentTarget) { target = target.parentNode; } if (target) break; From a quick jsfiddle sounds like working, please check.
Sorry for nagging, may I suggest closing issues (with or without a comment, I usually add a comment when I drop a comment) on mozReview when you address them, it helps the reviewer understanding what's left to do (plus it's a good way for the developer to track if all comments have been addressed)
https://reviewboard.mozilla.org/r/32023/#review32073 > you are breaking out of the while here, I suppose the idea was to break out of the case instead? Otherwise we'd always fall-through. > > Plus, I think you meant to use relatedTarget, cause target is always the element we moused out, that will be == this.panel also for the panel itself. > > I guess something like: > let target = event.relatedTarget; > while (target && target != event.currentTarget) { > target = target.parentNode; > } > if (target) > break; > From a quick jsfiddle sounds like working, please check. I looked back at this and realized I could write this a lot simpler. I've replaced it with: // Don't handle events for descendent elements. if (aEvent.target != aEvent.currentTarget) { break; }
I've marked all the other issues as fixed.
https://hg.mozilla.org/integration/fx-team/rev/b7450f64aa8754db12c5db879c809b6a13270015 Bug 1219794 - Show the bookmark popup for new bookmarks and autoclose if no interaction. r=mak
Depends on: 1250267
Try push with retriggers on Linux opt and debug bc7, https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bc93ae8a22d
Flags: needinfo?(jaws)
Attachment #8711221 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4720eaf2a456
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1250732
Depends on: 1251107
Depends on: 1251134
Depends on: 1251126
I have reproduced this bug on according to (2015-10-29) It's fixed on Latest Developer Edition: Build ID (20160326004034), User Agent : Mozilla/5.0 (Windows NT 6.3; rv:47.0) Gecko/20100101 Firefox/47.0 Tested OS-- Windows8.1 32bit
QA Whiteboard: [testday-20160401]
Thanks!
Status: RESOLVED → VERIFIED
No longer depends on: 1187647
Depends on: 1251071
(In reply to Marco Bonardo [::mak] from comment #10) > There's one thing that is unclear yet, the Cancel behavior. > 2. CTRL+D > * There's no Remove button > * Cancel is actually canceling the bookmarking operation and removing the > just created bookmark. > * Can use ESC to cancel bookmarking and remove the bookmark. I've recently noticed that there is no longer a cancel ability when using the hotkey. Why was this removed? Being able to press Esc to cancel/remove a bookmark when using the hotkey, while the edit window appears, was a very useful behavior that was in previous versions for years.
The bookmark can be removed via keyboard (on Windows en-US) using: Ctrl+D to add the bookmark Alt+R to select the Remove Bookmark button
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #44) > The bookmark can be removed via keyboard (on Windows en-US) using: > Ctrl+D to add the bookmark > Alt+R to select the Remove Bookmark button Thanks. I think I'll try to find an Autohotkey solution. To me the former behavior was not undoing or removing a bookmark but a canceling action during the initial bookmarking, which is why Esc was appropriate but c'est la vie.
There is no longer a way to undo changes to bookmark tags.
(In reply to Perry Wagle from comment #46) > There is no longer a way to undo changes to bookmark tags. You can simply edit the tags and click done. If you delete a tag and there is nothing else tagged with that tag it also gets removed from the list of tags.
When you hit star, you get a bookmark editor, which, among other things, lets you edit the bookmark's tags either via text, or a clickable list of tags. Click on one of the tags. There is now no way to undo that operation, nor a way to cancel a whole transaction of changes gone awry.
(In reply to Perry Wagle from comment #48) > Click on one of the tags. There is now no way to undo that > operation, nor a way to cancel a whole transaction of changes gone awry. I don't think I'm following you. If you click the star and edit a bookmark (including adding tags) and then click delete bookmark, it all gets canceled. If you check a tag you can uncheck to remove it. And if you manually type in a tag you can backspace to erase it. If you've already saved (by clicking done) your changes, you can click the star a second time as always and edit the bookmark, remove, edit or add tags, etc. https://www.dropbox.com/s/2yg6vat54jaevzi/tagedit.mp4?dl=0 I can check and uncheck tags, manually type in new tags and backspace them out
Use case: 1912 page wiki, each page is bookmarked with (say) 16 tags indicating when to see it next, what order to see it in, topic, category, when created, which major part of the wiki its from, and a flag to indicate the bookmark's tag list is in "wiki tag format", etc. If you open one of these bookmarks up, do something, and hit cancel now, it all get deleted. What?!? OOOH! CANCEL has been removed and replaced with DELETE. How nice. 10 years of muscle memory down the drain! Experimenting: (1) If you open up an existing bookmark, do nothing, and press ESC, then the dialog goes away. Ok so far. (2) If you open up an existing bookmark, type random crud into the tag list text, then press UNDO, then it reverts. Ok so far. (3) If you open up an existing bookmark, click on the box a random already-existing tag, then press UNDO, nothing happens! And there is no longer even a CANCEL to revert the whole session like ":q!" in VIM. And ESC is now disabled! The idea is you do massive typos to the (maybe the wrong) bookmark, and you just want to throw all the changes away. THAT was cancel. Depending on UNDO for total reversion of changes is a losing proposition, as anyone with experience with a text editor knows. "I just want to start over!" PS. With my version of the editmarkplus firefox extension, you not only can grow the bookmark editing window to a size that's not too tiny to actually use (crippleware), but lets you grow the list of tags box. It was NOT "too cluttered" in this very long requested state. And with my OldTagSpace plugin, you can do VERY interesting things with tags, enough so that they become useful and and essential part of your workflow.
s/editmarkplus/editbookmarkplus/
Another scenario, just now: Accidentally open an existing bookmark, go on for a while elsewhere. You want no changes. ESC is disabled. Undo is disabled. You are FORCED to hit DONE to commit any inadvertent changes!
Fwiw, you don't press DONE to commit changes, since the fields are instant-apply. Btw, any bookmark operation in the UI is handled through a transaction manager that allows to undo/redo any change. I know it's not handy, but opening the Library (Show all bookmarks) and undoing from Organize/Undo menu (provided you didn't close the browser), allows to undo any bookmark changes up to the wanted point. It's possible the undo field in text fields is not as reliable. If the browser has been restarted, it's also possible to restore bookmarks to the previous automatic backup. That said, I agree the tagging experience is horrible, but that's cause tags have never been handled as a priority. It's possible the QX team may want to do something about that, maybe with the help of the Taipei team. I think rather than trying to unfix the panel, we should make a decision about the future of tags, and provide a quality experience when using them.
I upgraded to FF 47.0 yesterday. Bookmarking has been changed/broken. This was working fine as it was. Now it's broken. Often I do searches, open some of the search links in tabs in the same FF window. Then I rapidly click the star to bookmark each tab. After your change it is not so rapid. In fact it is confusing when the "Page Bookmarked" bubble is popping up and I'm already in the next tab, at which point I have to click "Done" or wait until it goes away before I can star-click-bookmark the next tab. I do my organizing and tagging later. I can bookmark and organize much faster this way than if I organize each bookmark as I go, and even if I can't organize immediately following bookmarking, at least I have my bookmarks saved, and I can organize later. Is there anywhere in the FF Settings/Options that allows for a choice in which way bookmarking is performed? If not, please add one. In the meantime is there anything I can change in about:config? Or is there a work-around. This new behavior is driving me crazy!
(In reply to psuatocobra from comment #54) > ... Often I do searches, open > some of the search links in tabs in the same FF window. Then I rapidly click > the star to bookmark each tab. After your change it is not so rapid. In fact > it is confusing when the "Page Bookmarked" bubble is popping up and I'm > already in the next tab, at which point I have to click "Done" or wait until > it goes away before I can star-click-bookmark the next tab. When I click the next tab, the Page Bookmarked panel is canceled, but if I use Ctrl+Tab to navigate to the next tab, it isn't. It has the odd behavior you're describing. So I assume you're using Ctrl+Tab to navigate to the next tab. You may want to file a new bug along the lines of "Ctrl+Tab should close the Page Bookmarked panel" to address this.
(In reply to jscher2000 from comment #55) > You may want to file a new bug along the lines of "Ctrl+Tab should > close the Page Bookmarked panel" to address this. yes please.
(In reply to jscher2000 from comment #55) > When I click the next tab, the Page Bookmarked panel is canceled, > but if I use Ctrl+Tab to navigate to the next tab, it isn't That's already bug 1251046 -> bug 1171746. This patch "only" made that bug more visible. It's nice to know that normal users also notice other breakages, like I do. (In reply to psuatocobra from comment #54) > This was working fine as it was. Now it's broken. It's the most accurate description of this "improvement". It consists entirely of intentional bugs.
See Also: → 1251046
Depends on: 1279391
Depends on: 1283551
Depends on: 1285684
Depends on: 1290011
Depends on: 1294799
Depends on: 1300403
Depends on: 1322129
See Also: → 1326712
See Also: → 1327640
Depends on: 1327938
Depends on: 1331798
Depends on: 1331924
Depends on: 1335043
Depends on: 1346858
Depends on: 1448520
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: