1459887 - Relabel the "Remove Bookmark" button to "Cancel" in the New Bookmark dialog

Status: RESOLVED FIXED

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

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1459887 - Relabel the "Remove Bookmark" button to "Cancel" in the New Bookmark dialog.

Review commit: https://reviewboard.mozilla.org/r/246370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246370/

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8980224 [details]
Bug 1459887 - Relabel the "Remove Bookmark" button to "Cancel" in the New Bookmark dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246370/diff/1-2/

Comment 3

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a59dd02af32e7d9f6f4f25623e7f36f1f1d0a0cd

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8980224 [details]
Bug 1459887 - Relabel the "Remove Bookmark" button to "Cancel" in the New Bookmark dialog.

https://reviewboard.mozilla.org/r/246370/#review252542

::: browser/base/content/browser-places.js:131
(Diff revision 2)
>          }
>  
>          switch (aEvent.keyCode) {
>            case KeyEvent.DOM_VK_ESCAPE:
> +            if (this._isNewBookmark) {
> +              this._removeBookmarksOnPopupHidden = true;

I'm a little surprised by this change. The reason is, that although we're changing to save/cancel buttons, we're still going to be auto-saving after a time.

If the user is used to pressing escape to dismiss the dialog (to get it out the way), then they're now going to be removing the bookmark and use some other action.

If UX has cleared/requested this, that's fine, if not, I think we should check with them.

Also, we should include a test for this change if we keep it.

::: browser/base/content/browser-places.js:248
(Diff revision 2)
> -
>      await PlacesUtils.bookmarks.fetch({url: aUrl},
>        bookmark => this._itemGuids.push(bookmark.guid));
>  
> -    let forms = gNavigatorBundle.getString("editBookmark.removeBookmarks.label");
> +    let removeButton = this._element("editBookmarkPanelRemoveButton");
> +    if (this._isNewBookmark) {

I guess we haven't always done extensive tests, but it would be nice to get a test added for checking the button label has been set correctly, just in case something breaks it in future.

Probably could do it quite simply in browser_bookmark_popup.js.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Mark Banner (:standard8) from comment #4)
> Comment on attachment 8980224 [details]
> Bug 1459887 - Relabel the "Remove Bookmark" button to "Cancel" in the New
> Bookmark dialog.
> 
> https://reviewboard.mozilla.org/r/246370/#review252542
> 
> ::: browser/base/content/browser-places.js:131
> (Diff revision 2)
> >          }
> >  
> >          switch (aEvent.keyCode) {
> >            case KeyEvent.DOM_VK_ESCAPE:
> > +            if (this._isNewBookmark) {
> > +              this._removeBookmarksOnPopupHidden = true;
> 
> I'm a little surprised by this change. The reason is, that although we're
> changing to save/cancel buttons, we're still going to be auto-saving after a
> time.

When a dialog has a cancel button, then the escape key doing the same thing really seems like the only sensible option to me; users will expect this from interacting with other dialogs. Enter still works for dismissing the dialog while keeping the bookmark.

Comment 6

[Mark Banner (:standard8)]

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to Mark Banner (:standard8) from comment #4)
> > Comment on attachment 8980224 [details]
> > Bug 1459887 - Relabel the "Remove Bookmark" button to "Cancel" in the New
> > Bookmark dialog.
> > 
> > https://reviewboard.mozilla.org/r/246370/#review252542
> > 
> > ::: browser/base/content/browser-places.js:131
> > (Diff revision 2)
> > >          }
> > >  
> > >          switch (aEvent.keyCode) {
> > >            case KeyEvent.DOM_VK_ESCAPE:
> > > +            if (this._isNewBookmark) {
> > > +              this._removeBookmarksOnPopupHidden = true;
> > 
> > I'm a little surprised by this change. The reason is, that although we're
> > changing to save/cancel buttons, we're still going to be auto-saving after a
> > time.
> 
> When a dialog has a cancel button, then the escape key doing the same thing
> really seems like the only sensible option to me; users will expect this
> from interacting with other dialogs. Enter still works for dismissing the
> dialog while keeping the bookmark.

Ok, I hadn't thought of it like that. Lets keep it like you've got it, but just with the added tests.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8980224 [details]
Bug 1459887 - Relabel the "Remove Bookmark" button to "Cancel" in the New Bookmark dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246370/diff/2-3/

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8980224 [details]
Bug 1459887 - Relabel the "Remove Bookmark" button to "Cancel" in the New Bookmark dialog.

https://reviewboard.mozilla.org/r/246370/#review252834

Great, thank you for the additional test changes. r=Standard8

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/210b7cb896da
Relabel the "Remove Bookmark" button to "Cancel" in the New Bookmark dialog. r=standard8

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/210b7cb896da

Comment 11

[Md Maruf Rahman]

I have reproduced this bug with Nightly 62.0a1 (2018-05-24) on Windows 10, 64 Bit!
This bug's fix is verified with latest Beta!

Build ID 	20180702164905
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0