1459878 - Add "Show editor when saving" checkbox to the Bookmark panel

Status: VERIFIED FIXED

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

Description

[Dão Gottwald [:dao]]

Attachment: Mockup

When unchecked, this will disable the dialog for the next time and show a "Saved to Bookmarks!" confirmation instead.

The checkbox should also be present when editing an existing bookmark, such that users can re-check it if they want.

Comment 1

[Dão Gottwald [:dao]]

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

Comment 2

[Dão Gottwald [:dao]]

Attachment: Bug 1459878 - Add "Show editor when saving" checkbox to the Bookmark panel.

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

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8988316 [details]
Bug 1459878 - Add "Show editor when saving" checkbox to the Bookmark panel.

https://reviewboard.mozilla.org/r/253576/#review260386

There is one thing a bit underspecified, that is whether we want the same behavior for mouse and keyboard interactions. For example in the past we used to not open the panel with the mouse but open it with the keyboard. But I think in general we are moving towards more coherence between those, so I assume it is ok to handle both through this checkbox.

::: browser/app/profile/firefox.js:989
(Diff revision 1)
>  pref("browser.security.newcerterrorpage.enabled", false);
>  
>  // Whether to start the private browsing mode at application startup
>  pref("browser.privatebrowsing.autostart", false);
>  
> +pref("browser.bookmarks.editDialog.showForNewBookmarks", true);

please add a brief comment

::: browser/base/content/browser-places.js:239
(Diff revision 1)
>          gNavigatorBundle.getString("editBookmarkPanel.newBookmarkTitle") :
>          gNavigatorBundle.getString("editBookmarkPanel.editBookmarkTitle");
>  
> -    // No description; show the Done, Remove;
> +    let showForNewBookmarksCheckbox = this._element("editBMPanel_showForNewBookmarks");
> +    showForNewBookmarksCheckbox.checked = this.showForNewBookmarks;
> +    showForNewBookmarksCheckbox.hidden = false;

is it hidden by default for perf reasons? does it actually matter, since the whole panel is hidden?

Actually I think editBookmarkPanelBottomButtons and editBookmarkPanelContent are hidden just for not showing stale info in case of problems, but this checkbox refers to the panel, more than the bookmark, so it probably doesn't need to be hidden.

::: browser/components/places/content/editBookmarkPanel.inc.xul:126
(Diff revision 1)
>        <textbox id="editBMPanel_keywordField"
>                 onchange="gEditItemOverlay.onKeywordFieldChange();"/>
>      </vbox>
>    </vbox>
>  
> +  <checkbox id="editBMPanel_showForNewBookmarks"

I don't think this should be in this overlay, Is there a specific reason to not put it directly into "editBookmarkPanel" in browser.xul?

The overlay here is about stuff shared across all the bookmark panels, while this is specific to the star panel

I uderstand there may be additional aligning needed for that, but I'd still take that hit for better code separation.

::: browser/components/places/content/editBookmarkPanel.inc.xul:130
(Diff revision 1)
>  
> +  <checkbox id="editBMPanel_showForNewBookmarks"
> +            hidden="true"
> +            label="&editBookmarkOverlay.showForNewBookmarks.label;"
> +            accesskey="&editBookmarkOverlay.showForNewBookmarks.accesskey;"
> +            oncommand="gEditItemOverlay.onShowForNewBookmarksCheckboxCommand();"/>

same way the handles should probably be in StarUI in browser-places.js

::: browser/locales/en-US/chrome/browser/places/editBookmarkOverlay.dtd:26
(Diff revision 1)
>  <!ENTITY editBookmarkOverlay.tagsExpanderDown.tooltip        "Show all tags">
>  <!ENTITY editBookmarkOverlay.choose.label                    "Choose…">
>  <!ENTITY editBookmarkOverlay.newFolderButton.label           "New Folder">
>  <!ENTITY editBookmarkOverlay.newFolderButton.accesskey       "o">
> +<!ENTITY editBookmarkOverlay.showForNewBookmarks.label       "Show editor when saving">
> +<!ENTITY editBookmarkOverlay.showForNewBookmarks.accesskey   "S">

and the labels in browser.dtd

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8988316 [details]
Bug 1459878 - Add "Show editor when saving" checkbox to the Bookmark panel.

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

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8988316 [details]
Bug 1459878 - Add "Show editor when saving" checkbox to the Bookmark panel.

https://reviewboard.mozilla.org/r/253576/#review260708

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7666316c4222
Add "Show editor when saving" checkbox to the Bookmark panel. r=mak

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/7666316c4222

Comment 8

[Mohammad Maruf Rahman]

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

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