[de-xbl] convert calendar-snooze-popup to custom element
Categories
(Calendar :: General, defect)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: pmorris)
References
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
As I wrote in my email, this is probably what you want to do first:
Go to https://bgrins.github.io/xbl-analysis/converter/ and paste in the XML binding. You then
- hg cp calendar/base/content/widgets/calendar-alarm-widget.xml calendar/base/content/widgets/calendar-alarm-widget.js (to preserve some blame)
- add calendar-alarm-widget.js to appropriate jar.mn files, include calendar-alarm-widget.js where the calendar-alarm-widget.xml existed earlier
- open calendar-alarm-widget.js and paste in the code you got from the converter
- go through the code and adjust it as needed (it's not always perfect), try it out
- generate a patch and go for review
Assignee | ||
Comment 1•6 years ago
|
||
Thanks for the steps to get started on this. I've reassigned it to my account (:pmorris).
Assignee | ||
Comment 2•6 years ago
|
||
I'm attaching a patch that's work in progress so far. I'm still getting up to speed, and may be missing something, but this seems trickier than it first appears. I've looked through similar bugs and tried various things but I have not gotten the snooze popup to appear as expected when you click the snooze button. Here's the issue:
The calendar-snooze-popup
/ MozCalendarSnoozePopup
custom element class currently extends MozXULElement
. But ideally it should extend the menupopup
element so that it can inherit the menupopup behavior, namely appearing and disappearing as expected when its parent (a button in this case) has type=menu
. (Then it can be used like this: <menupopup is="calendar-snooze-popup"
.)
But there's not yet a menupopup
custom element to inherit from, and it appears that menupopup
is defined in C++ code, not at the XBL/XUL level (allowing it to be converted with the automatic converter).
Nevertheless, I think the path forward is to create a menupopup custom element to inherit from. (That's assuming we want a 1-to-1 XBL-to-custom-element approach, that preserves the same structure and behavior.) Am I on the right track here?
Reporter | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
Thanks for quick comments. I've made those changes. (On the formatting I've gotten spoiled by using Prettier and just letting it do the formatting for me.)
Also, after stepping away for a bit I figured out a way around the issue in comment 2. Basically use menupopup as a wrapper:
<menupopup>
<calendar-snooze-popup />
</menupopup>
That way we keep the menupopup behavior (which is just XUL so won't need converting), while converting the snooze popup away from XBL. With a few adjustments, this approach is working. Well, so far just for the "snooze all" button. I'll post another patch when I get the single event snooze button(s) working with the custom element.
Assignee | ||
Comment 5•6 years ago
|
||
Ready for review.
In comment 2, I was missing that the XBL for menupopup
is done via popup
:
https://searchfox.org/mozilla-central/source/toolkit/content/xul.css#302
https://searchfox.org/mozilla-central/source/toolkit/content/widgets/popup.xml#14
But presumably the de-XBL-ing of menupopup
will eventually happen in mozilla-central, so I think the approach of making calendar-snooze-popup
a child of menupopup
still makes sense.
Reporter | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Thanks for the review. I've made those changes.
I went with the full "function" syntax, which seemed easier to read, since we were including the "return". Also, I added some items to the "global" comment at the top (ChromeUtils, etc.). Might have been overkill, not sure if those things are to be included there.
What happens next? Does Philipp do a review? I requested one from him, but not sure about that.
Comment 8•6 years ago
|
||
Reporter | ||
Comment 9•6 years ago
|
||
After addressing the review comments, send off a try run to see there's no new failures. (You may have to file a bug to re-enable your hg privileges.)
When everything is ready, you add the checkin-needed keyword and someone will check it in to trunk for you.
Assignee | ||
Comment 10•6 years ago
|
||
Hi darktrojan, a question for you from the review in comment 8:
Should we avoid using a popup inside a popup here? When you click snooze for a calendar alarm, a popup appears with options for how long to snooze, and it currently contains another popup to select minutes, hours, or days for entering a custom amount. But that may be asking for trouble.
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Philipp Kewisch [:Fallen] [:π] from comment #8)
Since this is all in a js file now I think you can move this toplevel(ish) within the scope block. You may want to turn it into a lazy getter as well.
A lazy getter sounds good. I see there is XPCOMUtils.defineLazyGetter so I'll use that. (I considered just coding it up in place to avoid adding XPCOMUtils, but since there's already a utility function for this and I see it's used in the chat code, then it makes sense to use it.)
Assignee | ||
Comment 12•6 years ago
|
||
This patch addresses Philipp's review (except for popup in a popup question). I noticed that "cal" and "Preferences" are actually already in scope from "calendar-alarm-dialog.js". So no need to pull them in here (either with or without a lazy getter). The only thing remaining is the popup in a popup UI question.
Assignee | ||
Updated•6 years ago
|
Comment 13•6 years ago
|
||
You seem to have avoided the problem I had with nested popups, which is that clicking an item on the inner popup closes the outer popup. When the mouse hovers on the inner menulist, it opens, which is weird but acceptable IMO. It works, so I say go for it.
Assignee | ||
Comment 14•6 years ago
|
||
Hmm, that is odd. However, looks like the 'inner popup appearing on mouse hover' predates this patch. At least I see it on my release install of Thunderbird 60.4.0. The triggering mechanism is standard XUL stuff... shrug
So this is ready for a try server run. I'll do that once my privileges are restored.
Assignee | ||
Comment 15•6 years ago
|
||
A couple of cosmetic changes: better indentation of XUL string, and move the MozXULElement.parseXULToFragment
call directly into this.appendChild
for consistency with other custom elements. Sending this to the try server shortly.
Assignee | ||
Comment 16•6 years ago
|
||
Now that bug 1531870 has landed, have the calendar-snooze-popup inherit from MozElements.MozMenuPopup. (Rather than being a child of menupopup, which was the previous approach.)
I also switched to using "new Event()" rather than the old way of "document.createEvent()" in the snoozeAlarm method. That entailed adding some code to have the right event.target, see comment in the code.
(I tried making calendar-alarm-widget.js an ES6 module, but it didn't work because module execution is automatically deferred until the document has been loaded and parsed, which is too late to define the custom element since it is used in the document. See "Module Execution is Deferred" here: https://www.sitepoint.com/understanding-es6-modules/ )
(Now, why didn't they just make them like scripts, with an optional 'defer' attribute, so you could control when they are run? Or 'nodefer' if defaulting to defering? sigh...)
Reporter | ||
Comment 17•6 years ago
|
||
Assignee | ||
Comment 18•6 years ago
|
||
Thanks for the review. All good points. I've made the changes.
(Apparently you can use <script async type="module"
to override the 'defer by default' behavior, but I tried it and 'async' doesn't work in XUL files. So no luck. See: https://medium.com/ghostcoder/using-es6-modules-in-the-browser-5dce9ca9e911)
Assignee | ||
Comment 19•6 years ago
|
||
Fixes a mozmill test (testAlarmDialog.js). Try server run.
Assignee | ||
Comment 20•6 years ago
|
||
Some refactoring (in MozCalendarSnoozePopup connectedCallback) that I got into while working on bug 1530503.
Updated•6 years ago
|
Assignee | ||
Comment 21•6 years ago
|
||
In order to better preserve history, I've combined the patch for this bug with the related patch for bug 1530503. The combined patch is over on that bug as calendar-alarm-widget-de-xbl-5.patch, see comment 8.
Comment 22•6 years ago
|
||
https://hg.mozilla.org/comm-central/rev/b8523a5ed09c
[de-xbl] convert calendar-alarm-widget binding to custom element. r=philipp
As per comment #21, this was landed in bug 1530503.
Description
•