1941391 - Tab preview panel checks for open popups before initiating the timer, but not before actually opening, allowing other popups to open within this 500ms window

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Shane Hughes [:aminomancer]]

Attachment: A bad interaction between the preview panel and a feature callout message

See the attached screen recording (or bug 1932842) for the specific bug we encountered.

What's happening there is...

1. A Feature Callout message is set up to show when the new tab page opens.
2. Very quickly after clicking the new tab button, the mouse is now on top of the new tab, triggering the TabHoverStart event.
3. This calls previewPanel.activate(), which checks previewPanel._isDisabled(), which checks if there are open popups and returns false, because at this point, the callout is still in an asynchronous, comparatively slow process of opening.
4. Therefore, the TabPreviewPanelTimedFunction timer starts ticking.
5. After a short delay, the Feature Callout is rendered, which triggers a popupshowing event, but it's too late to influence the tab preview panel...
6. After the 500ms tooltip timer is finished ticking, the preview panel opens, even though there is already another popup open. This is because it only checks _onDisabled once: on previewPanel.activate(), and not on TabPreviewPanelTimedFunction._target(). In other words, it checks if there's an open popup before setting the timer, but it doesn't check for popups when the callback is actually executed.
7. This leaves a 500ms window, between hovering the tab and the timer finishing, in which other popups can open without stopping the preview panel from opening.
8. This is more problematic in this specific case, because both popups have a problem with showing while another popup is showing. The preview panel tries to avoid showing if there's another popup open. The Feature Callout doesn't have this type of check exactly. It's set up to allow showing a callout on top of other popups (say if we wanted to call out a new button in the App Menu, we'd need to layer it on top of the panel). But it has its own popupshowing event handler that dismisses the callout if another popup opens while it is open. This is to ensure the callout doesn't end up with other popups awkwardly stacked on top of it.
9. So, the callout is opening within the 500ms window, because both the preview panel and the callout are basically triggered by the same user interaction: clicking the new tab button, and thereby ending up with a new tab directly under the mouse cursor. The preview panel timer gets set. The callout shows. The preview panel timer finishes. Since there's no popup check at this point, the preview panel shows. This causes the callout to dismiss itself. And the user can never see it again, as it's limited to 1 impression, and this unfortunately counts as an impression even though it was visible for just a few hundred milliseconds.

This should be pretty simple to fix, just making sure we check for popups after the timer finishes:

@@ -61,7 +61,9 @@ export default class TabHoverPreviewPanel {    
 
     this._panelOpener = new TabPreviewPanelTimedFunction(       
       () => {
-        this._panel.openPopup(this._tab, this.#popupOptions);   
+        if (!this._isDisabled()) {
+          this._panel.openPopup(this._tab, this.#popupOptions); 
+        }
       },
       this._prefPreviewDelay,
       ZERO_DELAY_ACTIVATION_TIME,

Another, maybe supplementary, option would be to clear the timeout on popupshowing events:

@@ -297,6 +299,9 @@ export default class TabHoverPreviewPanel {
           ev.target !== this._panel &&
           (ev.target.nodeName == "panel" || ev.target.nodeName == "menupopup")
         ) {
+          if (setMethod === "add") {
+            this._panelOpener.clear();
+          }
           this._openPopups[setMethod](ev.target);
         }
       });

That's a little more awkward, and I'm not sure it adds any extra benefit. But just figured I'd point it out.

Comment 1

[Shane Hughes [:aminomancer]]

Attachment: A bad interaction between the preview panel and a feature callout message

(Attached screen recording)

Comment 2

[Shane Hughes [:aminomancer]]

Attachment: Bug 1941391 - Check for popups before opening tab preview panel. r=#tabbrowser-reviewers,dao

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b183bcfa6758
Check for popups before opening tab preview panel. r=dao,tabbrowser-reviewers

Comment 4

[Sandor Molnar[:smolnar]]

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