1585482 - Stop using xul:dialog as a root element and migrate consumers to xul:window[role=dialog] with the dialog as the only child

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Brian Grinstead [:bgrins]]

+++ This bug was initially created as a clone of Bug #1584283 +++

Similar to xul:wizard and xul:page, I'd like to stop using non-window xul root elements in order to make migration to html roots easier.

There are a number of consumers of this element in m-c: https://searchfox.org/mozilla-central/search?q=%3Cdialog&path=.

• There are a few places in the platform that I see that treat XUL dialogs specially https://searchfox.org/mozilla-central/search?q=atoms%3A%3Adialog&path=.
  -- One is just because it's a root element: https://searchfox.org/mozilla-central/rev/f372e8a46ef7659ef61be9938ec2a3ea34d343c6/layout/xul/nsBoxFrame.cpp#953
  -- The other is an accessible place that I believe can be fixed by using [role=dialog] on the new root: https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/accessible/generic/RootAccessible.cpp#89
  -- There's another that looks for an anonymous button in the root <dialog> element. It's not clear to me if that's dead code or not (post-XBL) but would need some analysis: https://searchfox.org/mozilla-central/rev/f372e8a46ef7659ef61be9938ec2a3ea34d343c6/accessible/generic/Accessible.cpp#1803-1817.
• There are also hits in JS such as document.documentElement.openHelp, document.documentElement.cancelDialog, etc (https://searchfox.org/mozilla-central/search?q=documentElement&case=false&regexp=false&path=dialog.js) which depend on the documentElement being the <dialog> custom element with these properties. Those calls should be able to switch to this within the dialog CE and to a reference to the DOM node from any external callers (if any).
• Some of the CSS (when it's treating dialog as a XUL root element) can be removed as well: https://searchfox.org/mozilla-central/search?q=%5Edialog&regexp=true&path=.css.

Comment 1

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1585482 - Restructure all <xul:dialog> usages such that they are not the top level element

Still a work in progress! Not ready to merge.

Comment 2

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1585482 - Remove C++ special-casing of top-level <dialog> elements

Depends on D52411

Comment 3

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1585482 - Necessary test fixes following the change to stop using xul:dialog as a root element.

Most of these fixes involve fixing test XUL to not use <dialog> as a top level element or replacing calls to document.documentElement that expect it to return the dialog, now that the dialog is not the top level element anymore.

Depends on D53721

Comment 4

[Robin Steuber (she/her) [:bytesized]]

So, clearly these patches aren't quite ready: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1be99788a3cbdbc9e825ac3896327b6984dbde3c

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #4)

So, clearly these patches aren't quite ready: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1be99788a3cbdbc9e825ac3896327b6984dbde3c

Scanning some of those chunks it looks mostly like tests are expecting the documentElement to be a dialog that need updating. Hopefully when you fix helpers like in https://treeherder.mozilla.org/#/jobs?repo=try&revision=1be99788a3cbdbc9e825ac3896327b6984dbde3c&selectedJob=276861526 it will knock a bunch out at a time.

In general you should be able to find replace: doc.documentElement.cancelDialog with doc.querySelector("dialog").cancelDialog (and so on for other dialog functions).

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #5)

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #4)

So, clearly these patches aren't quite ready: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1be99788a3cbdbc9e825ac3896327b6984dbde3c

Scanning some of those chunks it looks mostly like tests are expecting the documentElement to be a dialog that need updating. Hopefully when you fix helpers like in https://treeherder.mozilla.org/#/jobs?repo=try&revision=1be99788a3cbdbc9e825ac3896327b6984dbde3c&selectedJob=276861526 it will knock a bunch out at a time.

In general you should be able to find replace: doc.documentElement.cancelDialog with doc.querySelector("dialog").cancelDialog (and so on for other dialog functions).

Or as I mentioned previously, you could make the dialog customElement set window.dialogElement = this in the constructor / connectedCallback and then update callers from document.documentElement to window.dialogElement.

Comment 7

[Robin Steuber (she/her) [:bytesized]]

Getting closer, but still things left to fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=070cac8de1a42e02330769698ec45e731e4bf6c2

Comment 8

[Robin Steuber (she/her) [:bytesized]]

I think the work is pretty much done. The last Try run shows two failures that I'm a bit unsure about, but I've discussed them with Brian and we think that they are existing problems, unrelated to this patch. I'm not quite sure who should review this, but we want to wait until after the code freeze (and the Thanksgiving holiday) to land this anyways so that this gets lots of testing time and we don't cause any surprises.

Comment 9

[Robin Steuber (she/her) [:bytesized]]

I'd like to get some input from a11y to make sure everything still looks alright. Marco, would you be able to take a look for me? We mostly just want to make sure that the accessibility structure of the dialogs hasn't been changed by these three commits. There are sort of two "types" of dialogs involved in this patch, it would probably be good to take a look at one of each. I'll give an example of each type.

A good example of the first type is the "Clear History" dialog. To open this one, go to "about:preferences" and search for "Clear History". Then click the button with the "Clear History" label.

A good example of the second type is "Bookmark Properties" dialog. To open this one, go to first open the Bookmarks Library either with Control+Shift+B or via Hamburger Menu->Library->Bookmarks->Show All Bookmarks. Once in the Bookmarks Library, click Organize->New Bookmark.

Thanks for your help!

Comment 10

[Marco Zehe (:MarcoZ)]

Thanks for reaching out! These dialogs still work fine with that try build. One thing I noticed: I assume this try build didn't yet include bug 1572677? Because I saw that bug in the try build, but assume it is that bug and not something related to this change to dialogs. Is that correct?

Comment 11

[Robin Steuber (she/her) [:bytesized]]

Correct. The try build does not include Bug 1572677, so I believe that is why you were seeing that bug.

Thanks for your help!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db3709c80ad1
Restructure all <xul:dialog> usages such that they are not the top level element r=bgrins
https://hg.mozilla.org/integration/autoland/rev/9ed1b123250d
Remove C++ special-casing of top-level <dialog> elements r=Jamie
https://hg.mozilla.org/integration/autoland/rev/e183cbb4983c
Necessary test fixes following the change to stop using xul:dialog as a root element. r=marionette-reviewers,ato,bgrins

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/db3709c80ad1
https://hg.mozilla.org/mozilla-central/rev/9ed1b123250d
https://hg.mozilla.org/mozilla-central/rev/e183cbb4983c