1480529 - Tab Context Menu – Change “Bookmark All Tabs” to “Bookmark Tab” for single tab and “Bookmark Tabs” for multi-selected tabs

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Amy Lee [:amylee] UX]

Please change “Bookmark All Tabs” to “Bookmark Tab” for single tab and “Bookmark Tabs” for multi-selected tabs. See spec for reference.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

"Bookmark Tabs" has been implemented already. This bug will just be changing the singular tab context menu from "Bookmark All Tabs" to "Bookmark Tab"

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1480529 - Changed 'Bookmark All Tabs' to 'Bookmark Tab' for single tab selections. r?Felipe

I changed the toolbar context menuitem from 'Bookmark All Tabs' to 'Bookmark Selected Tabs' because it is separated from a specific tab and thus it is not clear which tab would get bookmarked if only one is selected. This seemed much clearer to me in my testing. The wording of 'Bookmark Selected Tabs' is already used elsewhere where we have 'Reload Selected Tabs', 'Close Selected Tabs', etc.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Also, "Bookmark All Tabs..." still exists in the Bookmark menu if accessed via keyboard, though I did remove the code that disabled the menuitem if only one tab exists. I don't know why we wouldn't allow that single tab to be bookmarked through the menuitem, and the code will work regardless.

One subtle change in behavior is that Bookmark All Tabs excludes pinned tabs (this change was made when pinned tabs were called App Tabs and we had increased expectations about their lifetime, nowadays we just think of pinned tabs as narrower tabs but don't do anything special with lifetime across restarts etc). However, if a user chooses Select All Tabs, all tabs are selected including pinned tabs, and then if the user chooses Bookmark Selected Tabs the bookmark folder that is created will include the pinned tabs.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> nowadays we just think of
> pinned tabs as narrower tabs but don't do anything special with lifetime
> across restarts etc

We still preserve pinned tabs across restarts, but I agree that the behavior here is better (i.e., bookmark all tabs that are selected).

Perhaps it would be useful to re-think if the "Select All Tabs" action should skip pinned tabs (it makes sense in some situations, but not in others). In any case, it's easy to unselect a tab if the user chooses to.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 9013001 [details]
Bug 1480529 - Changed 'Bookmark All Tabs' to 'Bookmark Tab' for single tab selections. r?Felipe

:Felipe Gomes (needinfo me!) has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f4d7ab6cd6d
Changed 'Bookmark All Tabs' to 'Bookmark Tab' for single tab selections. r=Felipe

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 1f4d7ab6cd6d (Bug 1480529) for perma failing on browser_multiselect_tabs_bookmark.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1f4d7ab6cd6daceaa33c294c7c078ac0ccb7f708

Backout link: https://hg.mozilla.org/integration/autoland/rev/7a728776d81231f97d7e126cdff83c6935bbc158

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=202890128&repo=autoland&lineNumber=2316

[task 2018-10-02T15:05:23.765Z] 15:05:23     INFO - TEST-PASS | browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js | Tab2 is multiselected - 
[task 2018-10-02T15:05:23.766Z] 15:05:23     INFO - TEST-PASS | browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js | Tab3 is not multiselected - 
[task 2018-10-02T15:05:23.767Z] 15:05:23     INFO - TEST-PASS | browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js | TabContextMenu context is the expected tab - 
[task 2018-10-02T15:05:23.768Z] 15:05:23     INFO - Buffered messages finished
[task 2018-10-02T15:05:23.768Z] 15:05:23     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js | Uncaught exception - at chrome://mochitests/content/browser/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js:40 - TypeError: menuItemBookmarkAllTabs is null; can't access its "hidden" property
[task 2018-10-02T15:05:23.769Z] 15:05:23     INFO - Stack trace:
[task 2018-10-02T15:05:23.769Z] 15:05:23     INFO - test@chrome://mochitests/content/browser/browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js:40:3
[task 2018-10-02T15:05:23.770Z] 15:05:23     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1093:34
[task 2018-10-02T15:05:23.770Z] 15:05:23     INFO - async*Tester_execTest@chrome://mochikit/content/browser-test.js:1084:16
[task 2018-10-02T15:05:23.771Z] 15:05:23     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:986:9
[task 2018-10-02T15:05:23.771Z] 15:05:23     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2018-10-02T15:05:23.772Z] 15:05:23     INFO - Leaving test bound test
[task 2018-10-02T15:05:23.773Z] 15:05:23     INFO - GECKO(2942) | MEMORY STAT | vsize 805MB | residentFast 275MB | heapAllocated 85MB
[task 2018-10-02T15:05:23.773Z] 15:05:23     INFO - TEST-OK | browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js | took 1536ms
[task 2018-10-02T15:05:23.774Z] 15:05:23     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-10-02T15:05:23.776Z] 15:05:23     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/ - 
[task 2018-10-02T15:05:23.777Z] 15:05:23     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-10-02T15:05:23.778Z] 15:05:23     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/ - 
[task 2018-10-02T15:05:23.779Z] 15:05:23     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-10-02T15:05:23.780Z] 15:05:23     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/tabs/browser_multiselect_tabs_bookmark.js | Found an unexpected tab at the end of test run: http://mochi.test:8888/ -

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de5c2979877f
Changed 'Bookmark All Tabs' to 'Bookmark Tab' for single tab selections. r=Felipe

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/de5c2979877f

Comment 10

[Damira Akh]

Verified in latest Nightly. 
Build ID 	20181003100127
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Comment 11

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Verified fixed in latest nightly 65.0a1(2018-11-08) and latest Beta 64.0b8 on Windows 10x64, macOS 10.13 and Ubuntu 16.04x64.