Closed Bug 1459878 Opened 2 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

Attached image 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.
Priority: -- → P1
Priority: P1 → P2
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Depends on: 1471580
Depends on: 1471585
Summary: Add "Show editor when saving" checkbox to the Bookmark dialog → Add "Show editor when saving" checkbox to the Bookmark panel
Whiteboard: [fxsearch]
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
Attachment #8988316 - Flags: review?(mak77)
Comment on attachment 8988316 [details]
Bug 1459878 - Add "Show editor when saving" checkbox to the Bookmark panel.

https://reviewboard.mozilla.org/r/253576/#review260708
Attachment #8988316 - Flags: review?(mak77) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7666316c4222
Add "Show editor when saving" checkbox to the Bookmark panel. r=mak
Blocks: 1472275
https://hg.mozilla.org/mozilla-central/rev/7666316c4222
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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
QA Whiteboard: [bugday-20180704]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.