Migrate <notificationbox> to a JS class

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P5
normal
RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: bgrins, Assigned: Paolo)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

10 months ago
Even though this one uses two <children /> tag it still looks like a good candidate, because:

- the <notification> slotted into `<children includes="notification"/>` is only ever for DOM nodes created by the widget itself (so can be appended directly as a child node).
- The xul:stack created ahead of the rest of the `<children />` can just be prepended during the connectedCallback

A couple more observations:
- We'll need some small tweaks to the notificationshidden behavior since we don't have xbl:inherits available.
- It doesn't load xbl stylesheet and I'm not seeing a lot of CSS access into anon content other than https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/toolkit/themes/shared/notification.inc.css#18-24, so that should make things easier.

Links:
- https://searchfox.org/mozilla-central/source/toolkit/content/widgets/notification.xml
- https://searchfox.org/mozilla-central/search?q=%3Cnotification&path=xul
(Reporter)

Comment 1

10 months ago
Here's an example of what the Custom Element would look like: https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/Notificationbox.js

Updated

10 months ago
Priority: -- → P5
(Reporter)

Updated

7 months ago
See Also: → 1496827
(Reporter)

Comment 3

7 months ago
I put up a pretty minimal migration patch. I'm seeing just a couple of test failures, although I haven't done manual testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96b7b8002e91bae46cb172d9c6561b2a4f83b466&selectedJob=204866539

Updated

6 months ago
QA Contact: gwimberly
(Reporter)

Updated

6 months ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
(Assignee)

Updated

6 months ago
Attachment #9016439 - Attachment is obsolete: true
(Assignee)

Comment 4

6 months ago
This clarifies the intention of each caller, and opens up the possibility of converting the notificationbox element to a class that creates the DOM nodes on demand.
(Assignee)

Comment 5

6 months ago
This also simplifies how Print Preview hides the chrome, and removes some leftover code.

Depends on D10577
(Assignee)

Comment 6

6 months ago
It seems that comm-central would be affected by Part 2 in a few cases. I found this one:

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/suite/mailnews/messenger.xul#237
(Assignee)

Comment 7

6 months ago
So, the preliminary work above allows keeping "notificationbox" as a lazily registered Custom Element, but avoid potential performance regressions by lazifying the creation of the actual "notificationbox" elements in browser.xul. These elements are the two global notification boxes and the notification box for each browser.
(Assignee)

Comment 8

6 months ago
So the element wouldn't exist until getNotificationBox() is called for the first time.
(Assignee)

Comment 10

6 months ago
Should removePreloadedBrowser actually remove the entire panel, rather than just the <browser> element?

https://searchfox.org/mozilla-central/rev/efc0d9172cb6a5849c6c4fc0f19d7fd5a2da9643/browser/base/content/tabbrowser.js#1737-1739

If so, we can probably fix the function and call it from here instead of using _preloadedBrowser:

https://searchfox.org/mozilla-central/rev/efc0d9172cb6a5849c6c4fc0f19d7fd5a2da9643/testing/mochitest/browser-test.js#919-924
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 12

6 months ago
This provides a way to access specific element classes before any associated Custom Element is instantiated, without creating a new global for each class. This can be useful to access static methods, create derived classes in a page, or allow lazy custom constructors.

Depends on D10892
Attachment #9021872 - Attachment description: Bug 1471403 - Part 1 - Separate the outer browser box from the notification box. r=dao,bgrins → Bug 1471403 - Part 1 - Separate the browser panel from the notification box. r=dao,bgrins
Attachment #9021873 - Attachment description: Bug 1471403 - Part 2 - Remove support for notificationHidden and for children that are not notifications. r=dao,bgrins → Bug 1471403 - Part 2 - Remove support for notificationsHidden and for children that are not notifications. r=dao,bgrins
Attachment #9022583 - Attachment description: Bug 1471403 - Part 3 - Lazify the creation of "notificationbox" elements. r=dao!,bgrins! → Bug 1471403 - Part 3 - Lazify the creation of "notificationbox" elements. r=dao,bgrins
Attachment #9022584 - Attachment description: Bug 1471403 - Part 4 - Add a MozElements namespace for element classes. r=bgrins! → Bug 1471403 - Part 4 - Add a MozElements namespace for element classes. r=bgrins
Attachment #9022585 - Attachment description: Bug 1471403 - Part 5 - Convert "notificationbox" to a custom class. r=bgrins! → Bug 1471403 - Part 5 - Convert "notificationbox" to a custom class. r=bgrins
(Assignee)

Comment 14

6 months ago
Note that I might remove everything but the test changes from Part 2, since we delete the binding in Part 5 anyways, and only there I fixed some other consumers so they don't rely on the notificationbox being a container with anonymous children.
(Reporter)

Comment 16

6 months ago
Just a heads up that the API for <notificationbox> is going to change. Rather than going from XBL -> Custom Element it's going from XBL -> JS class that builds the DOM. And the element will only contain notifications instead of containing notifications and children. Parts 1 and Part 5 are probably the most relevant changesets to TB. Alternatively you can temporarily restore the <notification> and <notificationbox> bindings and continue to bind them to the DOM nodes.
Flags: needinfo?(jorgk)

Updated

6 months ago
Depends on: 1505288

Comment 17

6 months ago
Thanks, filed bug 1505288.
Flags: needinfo?(jorgk)
(In reply to :Paolo Amadini from comment #10)
> Should removePreloadedBrowser actually remove the entire panel, rather than
> just the <browser> element?

Hmm, yeah, looks like it...
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 20

6 months ago
I'll move Part 1 to its own bug before landing, so we can better isolate any regressions.
(Assignee)

Updated

6 months ago
Depends on: 1505791
Attachment #9021872 - Attachment is obsolete: true
(Reporter)

Updated

6 months ago
Summary: Migrate <notificationbox> to a Custom Element → Migrate <notificationbox> to a JS class
(Assignee)

Comment 21

6 months ago
Review needinfo for the remaining parts.
Flags: needinfo?(dao+bmo)
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 23

6 months ago
Combining the lazy notification box with the removal of the outer box probably pushes the performance wins further, although they are still on Windows only:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=95ae3a10a8d79204436e877923fe6120237d1164&newProject=try&newRevision=8ae3deba07e8cb3c49aa35c568489b4ad1195b30&framework=1&showOnlyConfident=1

Comment 24

6 months ago
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c594f12cc04
Part 1 - Stop using notificationsHidden and children that are not notifications in most places. r=dao,bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a92421b50f
Part 2 - Lazify the creation of "notificationbox" elements. r=dao,bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc40fe6b994
Part 3 - Add a MozElements namespace for element classes. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6676334369f
Part 4 - Convert "notificationbox" to a custom class. r=bgrins
Depends on: 1506584
You need to log in before you can comment on or make changes to this bug.