Bugzilla

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #8)
> (In reply to :Gijs (he/him) from comment #7)
> > Not long-term, though we probably don't have time to remove it for 89, I don't think that means we need to support it though.
> 
> Ok, but do we see any risks so far in a possible disabling of the feature in a late beta? I know that this is impossible to say

"never say never", but the number of things that depend on this shipping is pretty large at this point.

> but if there are risks I would feel better to keep the support for the old modals in Marionette until all the old code gets removed. If we turn it off late in beta or even in a release this might have an affect for thousands of external CI systems. I would kinda like to avoid this situation.

Sure, up to you.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)
> So over on bug 1686743 the filtering of newly opened tab modals is done via:
> 
> ```
> const dialogs = browser.tabDialogBox.getContentDialogManager().dialogs;
> dialog = dialogs.find(dialog => dialog.frameContentWindow === dialogWindow);
> ```
> If 2) is true how could we best check if the dialog is part of the current browser? Would `dialogWindow.previousSibling === browser` be a good check?

I think for both cases it'd be better to find the container of the dialog in the parent document, and ensure it is a descendant of the same container as the current browser. In particular, given a `browser`, you can do:

```
let container = browser.closest(".browserSidebarContainer");
```

and given a dialog window with the new dialogs, you can get its framing element using `let frame = win.docShell.chromeEventHandler`. Then `container.contains(frame)` will tell you if it's for the current tab. For the old dialogs, you get some prompt element as the `subject` of the observer notification, and you can use that directly with `container.contains(subject)`, I think.

Does that help?

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #8)
> (In reply to :Gijs (he/him) from comment #7)
> > Not long-term, though we probably don't have time to remove it for 89, I don't think that means we need to support it though.
> 
> Ok, but do we see any risks so far in a possible disabling of the feature in a late beta? I know that this is impossible to say

"never say never", but the number of things that depend on this shipping is pretty large at this point.

> but if there are risks I would feel better to keep the support for the old modals in Marionette until all the old code gets removed. If we turn it off late in beta or even in a release this might have an affect for thousands of external CI systems. I would kinda like to avoid this situation.

Sure, up to you.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #5)
> So over on bug 1686743 the filtering of newly opened tab modals is done via:
> 
> ```
> const dialogs = browser.tabDialogBox.getContentDialogManager().dialogs;
> dialog = dialogs.find(dialog => dialog.frameContentWindow === dialogWindow);
> ```
> If 2) is true how could we best check if the dialog is part of the current browser? Would `dialogWindow.previousSibling === browser` be a good check?

I think for both cases it'd be better to find the container of the dialog in the parent document, and ensure it is a descendant of the same container as the current browser. This avoids depending a lot on the current DOM hierarchy and having to change the code every time specifics in nesting levels or whatever vary. In particular, given a `browser`, you can do:

```
let container = browser.closest(".browserSidebarContainer");
```

and given a dialog window with the new dialogs, you can get its framing element using `let frame = win.docShell.chromeEventHandler`. Then `container.contains(frame)` will tell you if it's for the current tab. For the old dialogs, you get some prompt element as the `subject` of the observer notification, and you can use that directly with `container.contains(subject)`, I think.

Does that help?