1669205 - PIP icon will disappear when dragging the tab to create a new window

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Attachment: PIP tab icon.gif

Affected versions

• Firefox 82.0b7
• Firefox 83.0a1

Affected platforms

• Windows 7
• macOS 11
• Ubuntu 20.04

Steps to reproduce

1. Launch Firefox
2. Access a video from Twitch for example
3. Put the video in PIP mode and drag the current tab out of the window to create a new one

Expected result

• The PIP icon in tab is still displayed

Actual result

• The PIP icon will disappear

Regression range

• I will see for a regression if there is one.

Additional notes

• The issue can be seen in the attachment

Suggested severity

• S3

Comment 1

[Cristian Fogel [:cfogel]]

Checked with builds from 2019 January (around the date of feature being launched) the pip-indicator was not displayed.

It's not a regressed behaviour:

• checked with mozregression and located builds between 2019-05-22 / 2019-05-23 on having or not the tab indicator so it became visible with addition of that element;
• most likely visible as mentioned above, with patch from bug 1528459 [Show PiP Icon in parent tab of media] / [Tab should indicate that a <video> inside of it is being viewed in Picture-in-Picture, and perhaps not display the audio indicator];
• pushlog URL.

Comment 2

[C.M.Chang[:chunmin]]

Mike, would you mind having a look?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I'll see if one of our MSU students has time for this.

Comment 4

[Reid Shinabarker]

Attachment: Bug 1669205 - PIP icon will disappear when dragging the tab to create a new window. r?mconley,gijs

Comment 5

[Micah [:mtigley] (she/her)]

I wanted to write down some things we learned while investigating this issue before I forget.

1. We're adopting the approach shown in D99018 by creating a "TabClose" event handler on the PictureInPicture object and setting an event listener for "TabClose" on the tab PiP is launched from. This works the first time the PiP tab is moved out of its original window. The second time, not so much. I've observed that the issue is reproducible when moving the PiP tab out into its own window (and not into another existing one).

2. The tab browser is responsible for firing the TabClose event when a tab is about to be removed from its original window and moved to the new one. Because of this, we'd need to set another listener for "TabClose" on the new tab the old PiP tab is being moved to. This is done inside the handler for "TabClose" we have on the PictureInPicture object:

// in PictureInPicture

handleEvent(event) {
  switch (event.type) {
    case "TabClose":
      ...
      let adoptedBy = event.detail.adoptedBy;
      adoptedBy.addEventListener("TabClose", this);
      ...
      break;
  }
}

The adoptedBy object here refers to the new tab the old PiP tab will be moving to. But it seems like the "TabClose" event is lost after it's fired from tabbrowser.js when moving the tab the second time.

3. This might mean adoptedBy isn't the new tab we think we are moving PiP to. The next steps are to observe the lifecycle of these <tab> element when they're removed. What's the difference between moving a tab into its own window vs moving a tab into an existing one?

Comment 6

[Micah [:mtigley] (she/her)]

It might be easier to set our new browser to the existing PiP window by firing a CustomEvent after we swap around the "pictureinpicture" attributes in swapBrowsersAndCloseOther. That way, we are guaranteeing the adopted tab is ready when browsers are swapped.

Let's try this:

1. Make PictureInPicture listen for a custom event called "PictureInPicture:SwappedBrowsers"

tab.addEventListener("PictureInPicture:SwappedBrowsers", this);

2. Create a handler called onPipSwappedBrowsers. This is responsible for setting the new browser to the current PiP player window:

    let otherTab = e.detail;
    if (otherTab) {
      for (let win of Services.wm.getEnumerator(WINDOW_TYPE)) {
        if (this.weakWinToBrowser.get(win) === e.target.linkedBrowser) {
          this.weakWinToBrowser.set(win, otherTab.linkedBrowser);
        }
      }
    }

3. Have browser fire "PictureInPicture:SwappedBrowsers" after we have swapped the "pictureinpicture" attributes with the closing tab.

      if (aOtherTab.hasAttribute("pictureinpicture")) {
        aOurTab.setAttribute("pictureinpicture", "true");
        modifiedAttrs.push("pictureinpicture");

        let event = new CustomEvent("PictureInPicture:SwappedBrowsers", {
          detail: aOurTab,
        });
        aOtherTab.dispatchEvent(event);
      }

Let's do this around here: https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#3974.

With this, we can probably remove the listener for "TabClose" since we are having PiP react only when swapping browsers.

Comment 7

[David]

Attachment: Bug 1669205 - Ensure PictureinPicture attribute is preserved when parent tab is changed

Comment 8

[David]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23528396a5dc69e60b846359b152948eed6ab770

Comment 9

[David]

Attachment: Bug 1669205 Ensure PictureinPicture attribute is preserved when parent tab is changed

Comment 10

[David]

https://treeherder.mozilla.org/jobs?repo=try&revision=7e11f4ec12d1099364b96a22069e7521dc422410

Comment 11

[David]

https://treeherder.mozilla.org/jobs?repo=try&revision=34b5e1dea1d524d78c2fd151617b8b255cbd53c3

Comment 12

[Micah [:mtigley] (she/her)]

(In reply to David from comment #11)

https://treeherder.mozilla.org/jobs?repo=try&revision=34b5e1dea1d524d78c2fd151617b8b255cbd53c3

Failures for linux-debug. Since we're both using either Windows or MacOSX, we didn't catch these failures when running the test locally.

I pushed another "try" push with more logging to help narrow down issue. It seems like we're timing out during detachTab : https://treeherder.mozilla.org/push-health/push?repo=try&revision=351e90ec3c9bbe0cae1cb5e1eabe690db3708841&testGroup=pr&selectedTest=toolkitcomponentspictureinpicturetestsbrowserpreserveTabPipIconOverlayjs&selectedTaskId=332753936&selectedJobName=toolkit%2Fcomponents%2Fpictureinpicture%2Ftests%2Fbrowser_preserveTabPipIconOverlay.js+test-linux1804-64%2Fdebug-mochitest-browser-chrome-e10s-6

Comment 13

[David]

Attachment: Bug 1669205 - added potential fix for linux hangup on tab tearout for test

Depends on D105831

Comment 14

[David]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f282ad82034b70c3e54215adcf321bf556d957f

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b96ac04b2a3
Ensure PictureinPicture attribute is preserved when parent tab is changed r=mhowell,mtigley

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5b96ac04b2a3

Comment 17

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Verified that the issue is fixed on latest Nightly 89.0a1 (2021-04-06) and Firefox 88.0b7. Tests were performed under macOS 11.2.3, Ubuntu 20.04 and Windows 10.