[de-xbl] convert/remove/rework the toolbar binding
Categories
(Thunderbird :: Toolbars and Tabs, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: pmorris)
References
Details
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
Details | Diff | Splinter Review |
Convert the toolbar binding: https://searchfox.org/comm-central/rev/a2f5fb50e09945b629efa137df65814f3e648063/common/bindings/toolbar.xml#12
<toolbar> seems to be a build in element. What our binding does is allow for customization of it. It seems there would be two approaches possible:
- just make it a CE, <toolbar is="customizable-toolbar"> or
- go the firefox route, xref bug 1119948/bug 1011857.
#2 would probably be preferable, but hard to say how much work involved. It's complicating things that we have more than one toolbar that can be customized.
Paul, you've touched on customizableui, want to take this on?
|
||
Comment 1•5 years ago
|
||
The actual toolbar supports small and normal icon size and we use only one size. Because of this and it could make the conversion easier, this functionality could be removed.
|
Assignee | |
Comment 2•5 years ago
|
||
I looked into the Firefox route, and it will take a fair bit of work, especially since we have 25 <toolbar>s and the way toolbar items are handled is different (and we have many of them). Not all of those toolbars need to stay customizable, but enough of them will to make it more complicated. (Also, Firefox has already moved to XHTML for the relevant files, while we're still on XUL. That may or may not be a problem.)
Given that the CE conversion looks straightforward and we only have 6 XBL bindings left to do, I think it makes sense to take an incremental approach and just do a CE conversion to finish off the de-XBL work. I've started on this and should have a patch up soon.
Then we (I) can tackle porting Firefox's toolbars and customization code, which we'll want to do for the accessibility improvements (bug 1553231). For a more incremental approach, first convert just the main toolbar and then work on converting the others, a bit at a time. This will require allowing both systems to co-exist peacefully.
Edit: just for the sake of being more precise, currently 14 of the 25 <toolbar>s are customizable="true".
|
|
Assignee | |
Comment 3•5 years ago
|
||
Convert the toolbar binding to a customizable-toolbar custom element.
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 4•5 years ago
|
||
Try server run looks fine: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=31981f45898850843473db20de8f6f21417a79cf
|
Reporter | |
Comment 5•5 years ago
|
||
Comment on attachment 9084415 [details] [diff] [review] toolbar-de-xbl-0.patch Review of attachment 9084415 [details] [diff] [review]: ----------------------------------------------------------------- Seems fine, with some smaller adjustments. r=mkmelin ::: common/bindings/customizable-toolbar.js @@ +14,5 @@ > + * @extends {MozXULElement} > + */ > + class CustomizableToolbar extends MozXULElement { > + connectedCallback() { > + if (this.delayConnectedCallback()) { you need to also make sure this doesn't run more than once. something like an _hasConnected @@ +24,5 @@ > > + if (document.readyState == "complete") { > + this._init(); > + } else { > + // Need to wait until XUL overlays are loaded. See bug 554279. XUL overlays do not exist since over a year, so this can be scrapped, and _init() code moved directly here into connectedCallback() instead @@ +80,2 @@ > > + let ids = (val == "__empty") ? [] : val.split(","); can you move this down before the for loop where it's first used?
|
|
Assignee | |
Comment 6•5 years ago
|
||
Thanks for the review.
(In reply to Magnus Melin [:mkmelin] from comment #5)
// Need to wait until XUL overlays are loaded. See bug 554279.XUL overlays do not exist since over a year, so this can be scrapped, and
_init() code moved directly here into connectedCallback() instead
Hm, but we still have overlays in calendar (and add-ons), and calendar code uses this customizable toolbar CE. I'm reluctant to take this out before we've removed the overlays from calendar code. Maybe I'm missing something?
|
|
Assignee | |
Comment 7•5 years ago
|
||
Also, this code will get dropped soon when we port over Firefox's newer toolbar/customization code, so makes sense to just leave it as-is to avoid any possible regressions.
|
|
Reporter | |
Comment 8•5 years ago
|
||
The overlaying of calendar and other add-ons is done by Overlays.jsm instead nowadays, but this is run after those events, so there is no readystatechange after it finished inserting stuff.
|
|
Assignee | |
Comment 9•5 years ago
|
||
Ah, makes sense! Thanks, I've made the requested changes.
A new try server run for good measure: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d741572aebe01d00324b9c4fb11fa5aee7b6e352
|
||
Comment 11•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d43bed230ddf
[de-xbl] Convert the toolbar binding to a custom element. r=mkmelin DONTBUILD
|
||
Updated•5 years ago
|
Description
•