1686315 - Adapt modal tests so that they work with Proton modals enabled or disabled

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect, P1)

Description

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

The new tab modal UI used for content prompts should also test the behavior expected of the old modal UI.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

As per guidance from Vicky, for tracking, we're marking all the bugs that people are working on as P1.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Toolkit::Notifications and Alerts' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

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

I've created a spreadsheet tracking the bugs that are failing for content prompts opened with TabDialogBox.

Comment 4

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

Attachment: Proton Modal Failing Tests

Comment 5

[Romain Testard [:RT]]

Marking as P2. Per experience review we agreed to mark as P1 bugs only bugs that will block MR1.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Re-marking as P1, because tests will start failing when we flip the pref, so this is really blocking…

Comment 7

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

Stealing, per agreement with mtigley.

Comment 8

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

Attachment: Bug 1686315 - Fix up some tests to pass with content prompt subdialogs enabled or disabled. r?Gijs!

Batch 1 of 2.

Comment 9

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

Attachment: Bug 1686315 - Fix up more tests to pass with content prompt subdialogs enabled or disabled. r?Gijs!

Batch 2 of 2.

Comment 10

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

Notably absent from these two batches are fixes for:

• browser/components/extensions/test/browser/browser_ext_optionsPage_modals.js (covered by bug 1702059)
• accessible/tests/mochitest/relations/test_ui_modalprompt.html

The test_ui_modalprompt.html test failure is unclear to me, and I'm consulting with morgan who's dealing with similar issues in bug 1700735 and bug 1699053. I don't know if fixing that test should block MR1, but if it doesn't, I can spin out a new bug for it.

Comment 11

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

From morgan in Matrix:

so it looks like one of the changes that pref causes is our dialog elements are now in internal frames in the a11y tree. I think those frames might be eating the events that we're waiting for in that test (we get a show for the frame, but not the dialog). not sure what needs to be changed there just yet, but sounds like we'll have to modify how we handle events for dialogs. I'll keep ya updated if I find a solution

We might want to file a follow-up bug for test_ui_modalprompt.html, unless fixing it as an accessibility blocker for MR1.

Asa, putting this on your radar in case this needs to be factored into MR1, accessibility-wise.

Comment 12

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

The patches here have approval to land, but they cause a shutdown window leak in debug builds, just like bug 1686743. I'm reasonably confident that they have the same root cause, so hopefully once we figure out what's happening in bug 1686743, we can get these patches landed.

Comment 13

[Morgan Reschenberg [:morgan]]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #11)

From morgan in Matrix:

so it looks like one of the changes that pref causes is our dialog elements are now in internal frames in the a11y tree. I think those frames might be eating the events that we're waiting for in that test (we get a show for the frame, but not the dialog). not sure what needs to be changed there just yet, but sounds like we'll have to modify how we handle events for dialogs. I'll keep ya updated if I find a solution

We might want to file a follow-up bug for test_ui_modalprompt.html, unless fixing it as an accessibility blocker for MR1.

Asa, putting this on your radar in case this needs to be factored into MR1, accessibility-wise.

Hello! I've got a fix for this test -- should I attach a patch to your stack here? Or should I file something else as a dependent? :)

Comment 14

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

(In reply to Morgan Reschenberg [:morgan] from comment #13)

Hello! I've got a fix for this test -- should I attach a patch to your stack here? Or should I file something else as a dependent? :)

Hi! Sorry for the late reply. Let's file a separate bug, if that's alright.

Comment 15

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

These patches are r+'d, but a recent try push shows leaking issues: https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=cvdLWuzQR9ChKy3pzJlFHg.0&revision=70c876c73dc9c1f953e3ec2d46979121936ab1f1

Comment 16

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

I've got an additional patch which fixes some of the leaks. I have one last bc1 Fission-only leak left.

Comment 17

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

Attachment: Bug 1686315 - Fix up the last few tests to pass with content prompt subdialogs enabled or disabled. r?pbz

Depends on D110483

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd716bdb723b
Fix up some tests to pass with content prompt subdialogs enabled or disabled. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2d10c21fd73f
Fix up more tests to pass with content prompt subdialogs enabled or disabled. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/9da736ac2cee
Fix up the last few tests to pass with content prompt subdialogs enabled or disabled. r=Gijs

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/bd716bdb723b
https://hg.mozilla.org/mozilla-central/rev/2d10c21fd73f
https://hg.mozilla.org/mozilla-central/rev/9da736ac2cee