Closed Bug 1272256 Opened 8 years ago Closed 8 years ago

Long press on "plus button" should allow the user to open a container tab

Categories

(Firefox :: Tabbed Browser, defect, P2)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: baku, Assigned: jkt)

References

(Blocks 3 open bugs)

Details

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

Attachments

(2 files, 8 obsolete files)

containers - tab bar context menu - i04
30.51 KB, image/png
Details
bug-1272256.3.patch
23.44 KB, patch
Gijs
: review+
Details | Diff | Splinter Review 
      No description provided.
Whiteboard: [domsecurity-active]
The idea here is: on long-press of the plus button, we could open the containers submenu to select which container we wanted to open.
Mike, do you know if we have an existing "widget" for this?
Flags: needinfo?(mconley)
Whiteboard: [domsecurity-active] → [domsecurity-active][userContextId]
Marking this as P2, as it would be nice to have for the initial launch, but isn't necessary since we have the down arrow.
Priority: -- → P2
Not an existing XUL widget, no, I don't think so. The thing I was thinking of was the back button - if you press and hold the back button, we show a list of recent shistory for where the browser can go back to.

Here's the code that does that for the back / forward buttons: http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#312

I notice, however, that there's a comment about maybe moving this into an XBL binding, which might be useful for our case here.
Flags: needinfo?(mconley)
If we are going to always show the down arrow when containers is enabled and use that for open new container tab, then is this bug still valid?  Do we want long press on the plus button to open a menu under the plus button?  Or do we want it to open the down arrow menu?
Let’s stick with exclusively using the down arrow. Click-and-hold isn’t obvious enough, and the arrow menu already works well when there are too many tabs present.

I remember another bug where I’ve proposed changing the plus button’s tooltip – something like “Open a new tab (accel-t), or click and hold to open a new container tab” – but this is still not an obvious hint, because you need to hover your mouse over the area for a bit.
After hearing from shorlander, he'd like us to go with this long press bug instead of the drop down bug https://bugzilla.mozilla.org/show_bug.cgi?id=1272416.

Bram, can you give us a UX design for the long press.  How should the containers menu be anchored by the plus button?  With an arrow of some sort?  Or should it look like a thought bubble/square coming from the plus button?
Flags: needinfo?(bram)
Hi Tanvi, thanks for filing this bug!

The design should be as unobstrusive as possible, just like the back button. This means that we won’t show an arrow or a thought bubble.

What would happen is, when you hover over the plus symbol, a tooltip appears that says:

Open a new tab (accel-T)
Click and hold to open a new container tab

And, just like the back button history, when you click and hold, the list of container tabs appears as shown.
Flags: needinfo?(bram)
We've gotten some feedback from users that request right click on the plus button instead of long press:
https://blog.mozilla.org/tanvi/2016/06/16/contextual-identities-on-the-web/#comment-31041

Bram or Bryan, do you have any thoughts on this? Why does the back button use long press instead of right click?
Flags: needinfo?(bram)
Flags: needinfo?(bbell)
(In reply to Tanvi Vyas - out until 6/27 [:tanvi] from comment #8)
> Why does the back button use long press instead of right click?

The back button supports both long press and right-click.

Sebastian
(In reply to Tanvi Vyas - out until 6/27 [:tanvi] from comment #8)
> Bram or Bryan, do you have any thoughts on this? Why does the back button
> use long press instead of right click?

As :sebo has mentioned on comment 9, the back button supports both long press and right-click. I was well aware of the long press, but never thought that right-click would do the same thing, too.

For consistency’s sake, I think we should follow this behaviour on the plus button. If a menu appear on long press, that same menu should also appear on right-click.

And then the tooltip can say:

“Open a new tab (accel-T)
Click and hold or right-click to open a new container tab”

What do you think?
Flags: needinfo?(bram)
Flags: needinfo?(bbell)
How about if we open another bug with the same mission but for the new window button?

The new window button can be dropped in all the same places as the new tab button (including the tab bar).

Beyond the idea using the same reasoning as this bug, some users may prefer to delineate their contexts into separate windows, thereby increasing the use of new windows for this purpose.
Assignee: amarchesini → jkt
Status: NEW → ASSIGNED
The current issue with the submitted patch is this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf880317fab0&selectedJob=24229401

When I inspect the new tab button and "use in console" and temp1.click() I end up with two tabs. I think this is why the test(s) are failing.
Depends on: 1290965
This patch is ready to go (may need a rebase). However the patch makes tests fail regarding the new tab click. Bug 1290965 is what in XUL is happening here which is blocking this really.
Comment on attachment 8773066 [details]
Bug 1272256 - Adding in longpress new tab button container menu

Hey Baku would you be able to review this also?
The DOM patch landed which was breaking tests when this was added. It's the dependent bug however not essential to review to the functionality to the user just the tests.
Thanks
Attachment #8773066 - Flags: review?(amarchesini)
Comment on attachment 8773066 [details]
Bug 1272256 - Adding in longpress new tab button container menu

Sorry after rebase notice an issue.
Attachment #8773066 - Flags: review?(amarchesini)
Comment on attachment 8773066 [details]
Bug 1272256 - Adding in longpress new tab button container menu

I cannot review this code. Gijs, can you help here?
Attachment #8773066 - Flags: review?(amarchesini) → review?(gijskruitbosch+bugs)
Comment on attachment 8773066 [details]
Bug 1272256 - Adding in longpress new tab button container menu

https://reviewboard.mozilla.org/r/65708/#review69834

I think we should refactor the mouse hold code in browser.js so that we can 'attach' that code to buttons, rather than the encapsulated function design that makes it impossible to reuse the code. Right now it seems you're duplicating a lot of that logic. With that refactor, we can also dynamically add/remove the menu kid to/from the button, which will make more sense of the a11y change and the duplication of "are we using a menu" state between the popup being disabled and the type=menu attribute. Hopefully it will also end up being a smaller patch in some respects, because we won't need the utilityOverlay.js duplicate set of code to deal with the tabbrowser.xml binding case.

::: accessible/tests/mochitest/tree/test_tabbrowser.xul:122
(Diff revision 3)
> +              children: [
> +                {
> +                  role: ROLE_MENUPOPUP,
> -              children: []
> +                  children: []
> -            }
> +                }
> +              ]

Shouldn't this be conditional on the pref?

Can you get a11y-review from MarcoZ or someone else who can comment on whether having the menupopup as a permanent kid makes sense? IMHO it would make more sense to add it dynamically when we use containers and set type=menu on it, but if it doesn't make a difference for the a11y side of things I guess we can potentially leave it like this. It's just that this change implies that it *does* make a difference... :-)

::: browser/base/content/browser.js:275
(Diff revision 3)
> +    if (aEvent.currentTarget.firstChild.disabled) {
> +      aEvent.preventDefault();
> +      return;

This is somewhat confusing. The markup doesn't have the kid disabled, but equally, the markup doesn't have the type=menu attribute. So in effect, the "current state" of the markup is in limbo (menu not disabled, but type=menu not set).

On top of that, this code is already in use for the back/fwd button, and modifying it in this way seems... odd.

Wouldn't it be simpler to just not have these handlers attached if containers are disabled?

::: browser/base/content/browser.js:323
(Diff revision 3)
>        cmdEvent.initCommandEvent("command", true, true, window, 0,
>                                  aEvent.ctrlKey, aEvent.altKey, aEvent.shiftKey,
>                                  aEvent.metaKey, null);
>        aEvent.currentTarget.dispatchEvent(cmdEvent);
>      }
> +    aEvent.preventDefault();

Why is this necessary?

::: browser/base/content/browser.js:348
(Diff revision 3)
> +  let newTabButton = document.getElementById("new-tab-button");
> +  newTabButton.setAttribute("type", "menu");
> +  _addClickAndHoldListenersOnElement(newTabButton);

This always sets the type=menu attribute, irrespective of the state of the pref, which I think is wrong.

::: browser/base/content/browser.xul:609
(Diff revision 3)
>                       ondragexit="newTabButtonObserver.onDragExit(event)"
>                       cui-areatype="toolbar"
> -                     removable="true"/>
> +                     removable="true">
> +        <menupopup id="newtab-popup"
> +                   oncommand="event.stopPropagation();"
> +                   class="newtab-popup"

This doesn't match the class you're using in the XBL binding, which is why you're now having to duplicate things in the CSS code later on...

::: browser/base/content/tabbrowser.xml:5035
(Diff revision 3)
> +              const newTabPopup = document.getElementById("newtab-popup");
> +              const newTab2 = document.getAnonymousElementByAttribute(this, "anonid", "tabs-newtab-button")
> +              const newTabPopup2 = document.getAnonymousElementByAttribute(this, "anonid", "newtab-popup")

Isn't the popup just button.firstChild all the time?

::: browser/base/content/tabbrowser.xml:6740
(Diff revision 3)
>            createUserContextMenu(event);
>            return;
>          }
>  
>          let containersEnabled = Services.prefs.getBoolPref("privacy.userContext.enabled");
> +        let undoTabDisabled = SessionStore.getClosedTabCount(window) == 0;

Why did you touch this code and then not use `undoTabDisabled` anywhere else?

::: browser/base/content/utilityOverlay.js:413
(Diff revision 3)
>      // (Menus close automatically with left-click but not with middle-click.)
>      closeMenus(event.target);
>    }
>  }
>  
> +function checkForHold(node, aEvent) {

This code looks duplicated from the existing click-and-hold code. Can you instead refactor that code to use an object structure that makes it easier to add those handlers to these anon buttons, without duplicating the code?
Attachment #8773066 - Flags: review?(gijskruitbosch+bugs) → review-
Comment on attachment 8773066 [details]
Bug 1272256 - Adding in longpress new tab button container menu

https://reviewboard.mozilla.org/r/65708/#review69834

> Why is this necessary?

This is to prevent the DOM fails which was causing the duplication of events.
Comment on attachment 8773066 [details]
Bug 1272256 - Adding in longpress new tab button container menu

https://reviewboard.mozilla.org/r/65708/#review71700

With the below changes the code looks OK to me. Note that I haven't tested it - I'm assuming that by now you have practical tested confidence that it does the job. :-)

::: browser/base/content/tabbrowser.xml:5014
(Diff revision 4)
> +          switch (aTopic) {
> +            case "nsPref:changed":
> +              // This is the only pref observed.
> +              let containersEnabled = Services.prefs.getBoolPref("privacy.userContext.enabled");
> +
> +              const newTab = document.getElementById("new-tab-button");

Note that this can be null. Let's file a followup to deal with the case where the button is added to an existing window using customize mode.

::: browser/base/content/tabbrowser.xml:5018
(Diff revision 4)
> +                const newTabPopup = document.createElementNS(
> +                  "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> +                  "menupopup");
> +                newTabPopup.id = "newtab-popup";
> +                newTabPopup.oncommand="event.stopPropagation();";
> +                newTabPopup.className = "new-tab-popup";
> +                newTabPopup.setAttribute("position", "after_end");
> +                newTab.appendChild(newTabPopup);
> +
> +                const newTabPopup2 = document.createElementNS(
> +                  "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> +                  "menupopup");
> +                newTabPopup2.setAttribute("anonid", "newtab-popup");
> +                newTabPopup2.oncommand="event.stopPropagation();";
> +                newTabPopup2.className = "new-tab-popup";
> +                newTabPopup2.setAttribute("position", "after_end");
> +                newTab2.appendChild(newTabPopup2);

Instead of the repetition, you could do:

```
for (let parent of [newTab, newTab2]) {
  if (!parent)
    continue;
  let popup = document.createElementNS(...);
  if (parent.id) {
    popup.id = "newtab-popup";
  } else {
    popup.setAttribute("anonid", "newtab-popup");
  }
  ...
  addClickAndHoldListenersOnElement(parent);
  parent.setAttribute("type", "menu");
}
```

ditto for the removal case.

::: browser/base/content/tabbrowser.xml
(Diff revision 4)
> -          containersTab.setAttribute("disabled", "true");
> +            containersTab.setAttribute("disabled", "true");
> -        }
> +          }
>  
> -        document.getElementById("alltabs_undoCloseTab").disabled =
> +          document.getElementById("alltabs_undoCloseTab").disabled =
> -          SessionStore.getClosedTabCount(window) == 0;
> +            SessionStore.getClosedTabCount(window) == 0;
> -

Nit: The line above and below this whitespace are unrelated, so please leave the empty line.
Attachment #8773066 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1298064
Keywords: checkin-needed
This can't be autolanded because all outstanding issues haven't been marked as resolved in MozReview.
Flags: needinfo?(jkt)
Keywords: checkin-needed
Final whitespace issue resolved in latest push.
Flags: needinfo?(jkt)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/08e4eadbb77d
Adding in longpress new tab button container menu r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08e4eadbb77d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1299667
Depends on: 1299669
Please make an effort to file bugs in the right component so that the people maintaining that component get a chance to follow along with your changes. This is 100% front-end and not at all a Core/DOM: Security bug.
Component: DOM: Security → Tabbed Browser
Product: Core → Firefox
Target Milestone: mozilla51 → Firefox 51
Version: unspecified → Trunk
I've backed this out for causing bug 1299667:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d96c286f93e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
While you're working on fixing the regressions, please don't spread out SetClickAndHoldHandlers into a slew of global functions / variables that are only needed privately. This code needs to be contained somehow.
Attachment #8773066 - Attachment is obsolete: true
Was unable to use mozreview as it's already landed.
This should fix the other two regressions.
Dao are you available to review this or to pass it on to someone relevant?
Attachment #8788131 - Flags: review?(dao+bmo)
Hey Dao, did you get chance to have a look at this change?

Thanks
Flags: needinfo?(dao+bmo)
Comment on attachment 8788131 [details] [diff] [review]
Adding a longpress menu to the new tab button for containers

>+  cancelHold(aButton) {
>+    clearTimeout(this.timers.get(aButton));
>+    aButton.removeEventListener("mouseout", (e) => this.mouseoutHandler(e), false);
>+    aButton.removeEventListener("mouseup", (e) => this.mouseupHandler(e), false);
>+  },
>+
>+  remove(aButton) {
>+    aButton.removeEventListener("mousedown", (e) => this.mousedownHandler(e), true);
>+    aButton.removeEventListener("click", (e) => this.clickHandler(e), true);
>+  },

These removeEventListener calls don't work because (e) => this.foo(e) creates a new function. The cleanest way to do this would be to implement handleEvent on gClickAndHoldListenersOnElement and pass 'this' as the listener.

>+      <field name="newtabUndoCloseTab" readonly="true">
>+        document.getAnonymousElementByAttribute(this, "anonid", "newtab_undoCloseTab");
>+      </field>

This appears to be unused.

>+                  popup.oncommand="event.stopPropagation();";

Does this work? It looks like it shouldn't, because element.oncommand is not a thing in XUL.

>+        if (event.target.getAttribute('anonid') == "newtab-popup" ||
>+            event.target.id == "newtab-popup") {
>+          createUserContextMenu(event);
>+        } else {

Please use double quotes consistently and return early instead of the 'else'.
Flags: needinfo?(dao+bmo)
Attachment #8788131 - Flags: review?(dao+bmo) → review-
I'm 99% sure the oncommand did work, it was however for the undo button so not needed anyway.

        Attachment #8788131 -
        Attachment is obsolete: true

        Attachment #8789830 -
        Flags: review?(dao+bmo)

    
    

    
  


  
Rebased to latest central

        Attachment #8789830 -
        Attachment is obsolete: true

        Attachment #8789830 -
        Flags: review?(dao+bmo)

        Attachment #8790269 -
        Flags: review?(dao+bmo)

    
    

    
  
Comment on attachment 8790269 [details] [diff] [review]
Fixing contextual identity menu icon to be visible by using the favicon class

This is the wrong patch. I'll just review the previous one assuming nothing drastically changed.

        Attachment #8790269 -
        Attachment is obsolete: true

        Attachment #8790269 -
        Flags: review?(dao+bmo)

        Attachment #8789830 -
        Attachment is obsolete: false

    
    

    
  
Comment on attachment 8789830 [details] [diff] [review]
Adding a longpress menu to the new tab button for containers

>+const gClickAndHoldListenersOnElement = {
>+  timers: new Map(),
>+
>+  mousedownHandler(aEvent) {

nit: please prefix "private" methods (those that are not supposed to be called from outside this object) with _

        Attachment #8789830 -
        Flags: review+

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #47)
> >+const gClickAndHoldListenersOnElement = {
> >+  timers: new Map(),
> >+
> >+  mousedownHandler(aEvent) {
> 
> nit: please prefix "private" methods (those that are not supposed to be
> called from outside this object) with _

The same applies to properties like "timers".

    
    

    
  


  
The differences in the patch were reordering a test line and in this one fixing a code nit.
Sorry for uploading the wrong patch.

Thanks!

        Attachment #8789830 -
        Attachment is obsolete: true

        Attachment #8790352 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/7826ad7df05f
Adding a longpress menu to the new tab button for containers r=dao
Keywords: checkin-needed

    
    

    
  


  
Fixes test and eslint issue, pushed to try to double check.

        Attachment #8790352 -
        Attachment is obsolete: true
Flags: needinfo?(jkt)

        Attachment #8790638 -
        Flags: review+

    
    

    
  
(In reply to Wes Kocher (:KWierso) from comment #53)
> There was also an ESLint failure from this
> https://treeherder.mozilla.org/logviewer.html#?job_id=11546177&repo=fx-team

Nice! I was going to complain about those return statements but forgot about it when r+'ing the patch.

    
    

    
  


  
Fixes test and linting issue, no other changes.

        Attachment #8790638 -
        Attachment is obsolete: true

        Attachment #8791085 -
        Flags: review+

    
    

    
  
Linting fixed and tests are passing.
Keywords: checkin-needed

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/ce601f208476
Adding a longpress menu to the new tab button for containers. r=dao
Keywords: checkin-needed

    
    

    
  
Hey Kamil, if you have a chance would you be able to look at getting a replication on this? I can't get one in linux or windows.

I'm pushing again to try, the screenshot looks like the main process being blocked however I can't see this with the patch.
Thanks
Flags: needinfo?(jkt) → needinfo?(kjozwiak)

    
    

    
  
I retriggered the Win7VMOpt m-e10s(bc7) job a bunch and got some of the failures on your try push.

    
    

    
  
Clearing ni? for now as Jonathan is going to try getting a Win 7 development VM running. Jonathan, let me know if you need any help!
Flags: needinfo?(kjozwiak)

    
    

    
  
Still unable to replicate with the latest rebase the best error I have seen is a replication of this (however happens about every 40 runs of the test that is failing):
https://bugzilla.mozilla.org/show_bug.cgi?id=983948#c96

Will wait for the latest try.

    
    

    
  
Hey Gijs,

This bug got backed out due to test failure timeouts in Windows.

The only way I have been able to prevent the timeout is with a Promise settimeout: https://hg.mozilla.org/try/rev/f90031ae3c683c7a5ec0d277406dae7b84e50b79#l6.43 Is there anything you could think of that could be needed to be checked for in windows?

Thanks
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to Jonathan Kingston [:jkt] from comment #74)
> Hey Gijs,
> 
> This bug got backed out due to test failure timeouts in Windows.
> 
> The only way I have been able to prevent the timeout is with a Promise
> settimeout:
> https://hg.mozilla.org/try/rev/f90031ae3c683c7a5ec0d277406dae7b84e50b79#l6.
> 43 Is there anything you could think of that could be needed to be checked
> for in windows?
> 
> Thanks

Not off-hand. It looks like the context menu isn't opening. I tried to look at screenshots for the failure but the wifi I'm on is too terrible to do so (server not found errors). In the screenshot for the timeout, is the back/fwd button disabled? It that case it sounds like you're not waiting to be sure that there are in fact session history items available so that the back/fwd buttons are enabled and visible. You could verify this by logging the bounding client rects of the buttons before synthesizing the clicks (with an appropriate info() call) and/or checking the browser's sessionHistory property.
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  


  
Updated patch fixes tests in Windows.

        Attachment #8791085 -
        Attachment is obsolete: true

        Attachment #8804210 -
        Flags: review?(dao+bmo)

    
    

    
  
Would you be able to review? the changes are small since last time just fixing tests really for Windows.
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)

        Attachment #8804210 -
        Flags: review?(dao+bmo) → review+

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/309e32ebbc3a
Adding a longpress menu to the new tab button for containers. r=dao
Keywords: checkin-needed

    
    

    
  
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/822379ac9dbb
Backed out changeset 309e32ebbc3a for eslint failure

    
    

    
  
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f19157ac2e48
Adding a longpress menu to the new tab button for containers. r=dao

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/f19157ac2e48
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago

          status-firefox52:
          --- → fixed
Resolution: --- → FIXED

    
    

    
  
backed out for possibly causing timeouts
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/8d988053f47b
Backed out changeset f19157ac2e48 for possibly causing timeouts on trunk trees on a CLOSED TREE

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/e00ed3bd29f9
Adding a longpress menu to the new tab button for containers because innocent of Bustage on a CLOSED TREE. r=dao

    
    

    
  
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/67f0d823967c
Backed out changeset e00ed3bd29f9 fix eslint issues

    
    

    
  


  
Hey :Tomcat,
Sorry I was off ill most of last week.
I have fixed the test that had an ESLint fail, the patch is a 3 line change only in a test does this need another r+? No functionality has changed even in the test.

Seems the backouts were only related to this and the others were just checking if the timeouts were caused by this patch.

Thanks
Jonathan

        Attachment #8804210 -
        Attachment is obsolete: true
Flags: needinfo?(jkt) → needinfo?(cbook)

        Attachment #8808174 -
        Flags: review+

    
    

    
  
Comment on attachment 8808174 [details] [diff] [review]
bug-1272256.3.patch

Hey Gijs,

The change here is:

- var backButton = document.getElementById("back-button");
+ let backButton = document.getElementById("back-button");
- var contextMenu = document.getElementById("backForwardMenu");
+ let contextMenu = document.getElementById("backForwardMenu");
- var rect = backButton.getBoundingClientRect();


Could you give me an review for this please :)
Thanks

        Attachment #8808174 -
        Flags: review+ → review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8808174 [details] [diff] [review]
bug-1272256.3.patch


- var backButton = document.getElementById("back-button");
+ let backButton = document.getElementById("back-button");
- var contextMenu = document.getElementById("backForwardMenu");
+ let contextMenu = document.getElementById("backForwardMenu");
- var rect = backButton.getBoundingClientRect();

^^ r=me for that. I can't work out how to get interdiff on bugzilla to do the right thing because all the attachments have the same name :-( . So I'm just assuming there's nothing else. FWIW, in future you don't need a re-review for those kinds of changes.

        Attachment #8808174 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93361acd5e00
Add a longpress menu to the new tab button for containers. r=dao, r=Gijs
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/93361acd5e00
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED

    
    

    
  
Successfully managed to reproduce this bug on Nightly 49.0a1 (2016-05-12) from Ubuntu 14.04.5 (64 Bit)

This issue is now verified as fixed on Latest Firefox Nightly 52.0a1 (2016-11-12) (64-bit)

Build ID: 20161112030203
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
OS: Linux 4.4.0-47-generic; Ubuntu 14.04.5
QA Whiteboard: [bugday-20161109]
Status: RESOLVED → VERIFIED

          status-firefox52:
          fixed → verified
Flags: needinfo?(cbook)
Depends on: 1317317
Depends on: 1318491

    
    

    
  
Jonathan, it's irritating that you didn't make sure to fix all regressions that got reported the first time this landed. Please double down on your efforts to fix bug 1299669 or we may have to back this out again especially since it's already on aurora now.

    
    

    
  
The patches were tested on all platforms, it's likely something changed between them landing for Windows. I'm on PTO at the moment and unlikely to have a Windows machine to look at until Wednesday. The patch likely won't be more than a one liner when I have a machine. I will take a look now to see if I can spot anything obvious that has changed.
Flags: needinfo?(dao+bmo)

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #96)
> especially
> since it's already on aurora now.

containers are disabled on aurora. I would not expect the patch to have any side-effects on aurora. Am I wrong?

    
    

    
  
Other than users can enable it via the pref: privacy.userContext.enabled

As far as I'm aware: privacy.userContext.ui.enabled is disabled on aurora which is the about:preferences way of enabling containers.

There is some direction to pref this on via test pilot but not anytime soon.
Flags: needinfo?(dao+bmo)

    
    

    
  
(In reply to Jonathan Kingston [:jkt] from comment #99)
> There is some direction to pref this on via test pilot but not anytime soon.

We will enable the pref in Firefox 50+ in Test Pilot studies.  The Long Press might get overriden by the addon (we aren't sure yet; waiting for a UX).

    
  
Depends on: 1327949

    
  
Target Milestone: Firefox 51 → Firefox 52

    
    

    
  
We are doing a Test Pilot experiment on Containers in Firefox 51 in late January.  It doesn't look like an addon can replicate this plus button behavior, as we previously thought it could.  Without an easy way to create a new container tab, we may have limited results from the experiment. What would be the risk to uplifting this to Firefox 51/beta?  Would we also have to uplift https://bugzilla.mozilla.org/show_bug.cgi?id=1299669?
Flags: needinfo?(jkt)
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to Tanvi Vyas[:tanvi] from comment #101)
> We are doing a Test Pilot experiment on Containers in Firefox 51 in late
> January.  It doesn't look like an addon can replicate this plus button
> behavior, as we previously thought it could.

Off-hand, it seems to me that a XUL-based add-on should be able to. Who knows more about the claim that this is not possible and what it's based on?

> Without an easy way to create
> a new container tab, we may have limited results from the experiment. What
> would be the risk to uplifting this to Firefox 51/beta?

Well, it's a relatively large patch to things with relatively poor test coverage (given it initially caused stuff like bug 1299667 without that being caught by automated testing). There are also unresolved and not-well-understood bugs still (e.g. bug 1298064, bug 1327949). I also don't know what the state of containers is on 51 - a lot of the other container-related fixes haven't gone into it, I should think.

Could we run this test pilot experiment against 52 beta instead?

>  Would we also have
> to uplift https://bugzilla.mozilla.org/show_bug.cgi?id=1299669?

Yes, and a fix for bug 1327949, which at the moment is still unfixed on Nightly, but without which this really can't ship. Plus uplifting bug 1318491. Plus maybe other things (I haven't done an exhaustive search). I'm not super excited about that prospect, at least not until we understand the remaining open bugs better and have had some QA checks on some of this functionality... :-(
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
Thanks Gijs!  We won't try uplifting this patch then.  Plus you are right that an addon can do more than what we had thought.
Flags: needinfo?(jkt)

    
  
Blocks: 1331595

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: