Closed Bug 1318491 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P2)

53 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- verified

People

(Reporter: kjozwiak, Assigned: jkt)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [userContextId][domsecurity-backlog])

Attachments

(1 file, 2 obsolete files)

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/
Has STR: --- → yes
Priority: -- → P2
Will take as I worked on this.
Assignee: nobody → jkt
Attachment #8814086 - Flags: review?(gijskruitbosch+bugs)
Attachment #8814086 - Attachment is obsolete: true
Attachment #8814086 - Flags: review?(gijskruitbosch+bugs)
Attachment #8814086 - Attachment is obsolete: false
Attachment #8814185 - Attachment is obsolete: true
Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review95558
Attachment #8814086 - Flags: review?(jkt)
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!
Attachment #8814086 - Flags: review?(jkt) → review?(gijskruitbosch+bugs)
Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review95594
Attachment #8814086 - Flags: review?(jkt)
Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review95670
Attachment #8814086 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8814086 - Flags: review?(jkt)
Keywords: checkin-needed
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?
Flags: needinfo?(jkt)
Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review96252
Attachment #8814086 - Flags: review?(jkt)
Comment on attachment 8814086 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/95384/#review96256
Attachment #8814086 - Attachment is obsolete: true
Flags: needinfo?(jkt)
Attachment #8814086 - Flags: review?(jkt)
Attachment #8815121 - Flags: review?(gijskruitbosch+bugs)
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!
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1320757
Please ensure that the review flags are properly set in MozReview so that Autoland can push this.
Keywords: checkin-needed
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?
Attachment #8815121 - Flags: review?(gijskruitbosch+bugs) → review-
Flags: needinfo?(gijskruitbosch+bugs)
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
Attachment #8815121 - Flags: review?(gijskruitbosch+bugs) → review+
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 on attachment 8815121 [details]
Bug 1318491 - Disable new tab longpress containers menu for private browsing mode

https://reviewboard.mozilla.org/r/96114/#review96748
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a37d5a9d30fc
Disable new tab longpress containers menu for private browsing mode r=Gijs
https://hg.mozilla.org/mozilla-central/rev/a37d5a9d30fc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.