1318491 - opening containers via the long press on the "plus button" shouldn't work under PB

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

We shouldn't be allowing the creation of containers via the long press on the "plus button" in private browsing. We should check to see if we're running in PB, and disable the long press accordingly. Similar to what we do with the file menu under PB.

STR:

* launch the latest version of m-c
* open a PB container via Hamburger Menu -> New Private Window
* long press the new tab button (+) and select click on any container

Build Used:
* https://archive.mozilla.org/pub/firefox/nightly/2016/11/2016-11-17-03-02-12-mozilla-central/

Comment 1

[Jonathan Kingston [:jkt] he/him]

Will take as I worked on this.

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

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

Comment 3

[Jonathan Kingston [:jkt] he/him]

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

Comment 4

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1318491 - Adding in container name to the tab title tooltip

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

Comment 5

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review95558

Comment 6

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

Hi Gijs,

Mozreview might be messed up as I pushed a wrong commit to this bug, however the patch is named according to the bug and I obsoleted the other.
Thanks!

Comment 7

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review95594

Comment 8

[:Gijs (he/him)]

Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review95670

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Reviewboard says the patch has been discarded because it's no longer required and also doesn't show the r+ from Gijs. Can you fix this in mozreview or should this land on inbound?

Comment 10

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review96252

Comment 11

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review96256

Comment 12

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

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

Comment 13

[Jonathan Kingston [:jkt] he/him]

I have resubmitted the review which is the same as the previous patch. Hopefully this should clear up the autoland issue here. Could I get another r+ please.

Review is here: https://reviewboard.mozilla.org/r/96114/diff/1#index_header

Thank you!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please ensure that the review flags are properly set in MozReview so that Autoland can push this.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8815121 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/96114/#review96428

::: browser/components/contextualidentity/test/browser/browser_newtabButton.js:40
(Diff revision 1)
> +  let privateDocument = privateWindow.document;
> +  let newTab = privateDocument.getElementById('tabbrowser-tabs');
> +  ok(!newTab.hidden, "new tabs should not be hidden");

Some small issues here:

1) `tabbrowser-tabs` is just the tab container, so:

```js
let {tabContainer} = privateWindow.gBrowser;
```

should work.

2) `newTab` isn't actually the new tab button... did you mean to select that anon node? Did you verify that this test fails if you remove the `&& isPrivateWindow` check?

Comment 16

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8815121 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

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

Comment 17

[:Gijs (he/him)]

Comment on attachment 8815121 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/96114/#review96744

::: browser/components/contextualidentity/test/browser/browser_newtabButton.js:45
(Diff revision 2)
> +  ok(!!newTab.clientWidth, "new tabs should not be hidden");
> +  ok(!newTab2.clientWidth, "new tabs overflow should be hidden");

Nit: for these two messages, please use "new tab button" and "overflow new tab button", respectively

Comment 18

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8815121 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

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

Comment 19

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8815121 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/96114/#review96746

Comment 20

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8815121 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/96114/#review96748

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a37d5a9d30fc
Disable new tab longpress containers menu for private browsing mode r=Gijs

Comment 22

[Phil Ringnalda (:philor)]

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

Comment 23

[Kamil Jozwiak [:kjozwiak]]

Went through verifcation using the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2017/01/2017-01-22-03-02-12-mozilla-central/

Platforms Used:

* macOS 10.12.2 x64 - PASSED
* Win 10 Pro x64 - PASSED
* Ubuntu 16.04.1 LTS x64 - PASSED

Test Cases Used:

* ensured that the long press on the "plus button" doesn't work under PB
** ensured it's still working in non-pb windows/tabs
* ensured the following access points are still being disabled while using PB
** "File -> New Container Tab"
** "Open Link in New Container Tab" via the right click context menu
** "Open Container Tab" button under the hamburger menu
** "New Container Tab" under the drop down menu that appears when there's a lot of tabs opened