Closed Bug 1528201 Opened 6 years ago Closed 6 years ago

[de-xbl] convert calendar-snooze-popup to custom element

Categories

(Calendar :: General, defect)

Product:
Calendar
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: pmorris)

References

Details

Attachments

(1 file, 8 obsolete files)

calendar-snooze-popup-8.patch
(deleted), patch
Fallen
: review+
Details | Diff | Splinter Review

https://searchfox.org/comm-central/source/calendar/base/content/widgets/calendar-alarm-widget.xml#201

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

Thanks for the steps to get started on this. I've reassigned it to my account (:pmorris).

Assignee: paulmorriss → paul
Attached patch wip-calendar-snooze-popup.patch (obsolete) (deleted) — — Splinter Review

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?

Attachment #9045430 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9045430 [details] [diff] [review] wip-calendar-snooze-popup.patch Review of attachment 9045430 [details] [diff] [review]: ----------------------------------------------------------------- Some quick comments, it's very late here. ::: calendar/base/content/dialogs/calendar-alarm-dialog.xul @@ +28,5 @@ > width="600" > height="300"> > <script type="application/javascript" src="chrome://calendar/content/calendar-alarm-dialog.js"/> > <script type="application/javascript" src="chrome://calendar/content/calendar-item-editing.js"/> > + <script type="application/javascript" src="chrome://messenger/content/customElements.js"/> don't think you need this @@ +39,5 @@ > <button id="alarm-snooze-all-button" > type="menu" > label="&calendar.alarm.snoozeallfor.label;"> > + <menupopup is="calendar-snooze-popup" > + id="alarm-snooze-all-popup" please format these as <menupopup is="calendar-snooze-popup" id="alarm-snooze-all-popup" ::: calendar/base/content/widgets/calendar-alarm-widget.js @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +("use strict"); no parenthesis please @@ +24,5 @@ > + <menuitem label="&calendar.alarm.snooze.45minutes.label;" value="45" oncommand="snoozeItem(event)"></menuitem> > + <menuitem label="&calendar.alarm.snooze.1hour.label;" value="60" oncommand="snoozeItem(event)"></menuitem> > + <menuitem label="&calendar.alarm.snooze.2hours.label;" value="120" oncommand="snoozeItem(event)"></menuitem> > + <menuitem label="&calendar.alarm.snooze.1day.label;" value="1440" oncommand="snoozeItem(event)"></menuitem> > + <children></children> you'll have to verify, but IIRC this never really had any children (or you'd have to handle them) @@ +28,5 @@ > + <children></children> > + <menuseparator></menuseparator> > + <hbox class="snooze-options-box"> > + <textbox anonid="snooze-value-textbox" oninput="updateAccessibleName()" onselect="updateAccessibleName()" type="number" size="3"></textbox> > + <menulist anonid="snooze-unit-menulist" class="snooze-unit-menulist" allowevents="true"> anonid is an xbl thing, we usually remove them as we go @@ +36,5 @@ > + <menuitem closemenu="single" class="unit-menuitem" value="1440"></menuitem> > + </menupopup> > + </menulist> > + <toolbarbutton anonid="snooze-popup-ok" class="snooze-popup-button snooze-popup-ok-button" oncommand="snoozeOk()"></toolbarbutton> > + <toolbarbutton anonid="snooze-popup-cancel" class="snooze-popup-button snooze-popup-cancel-button" aria-label="FROM-DTD-calendar-alarm-snooze-cancel" oncommand="snoozeCancel()"></toolbarbutton> when you get the the FROM-DTD stuff, you need to fix that manually MozXULElement.parseXULToFragment takes a second argument for the chrome urls to find the dtds @@ +44,4 @@ > > + const { Preferences } = ChromeUtils.import( > + "resource://gre/modules/Preferences.jsm" > + ); these go onto one line I think @@ +56,2 @@ > > + let unitList = document.getAnonymousElementByAttribute( .getAnonymousElementByAttribute is for anonymous content, that is, xbl internal content. You need to do something like this.querySelector(".snooze-unit-menulist") @@ +171,5 @@ > + for (let menuItem of items) { > + pluralString = cal.l10n.getCalString(unitName(menuItem)); > + menuItem.label = PluralForm.get(unitValue.value, pluralString) > + .replace("#1", "") > + .trim(); I think trim fits well on the previous line
Attachment #9045430 - Flags: feedback?(mkmelin+mozilla)

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.

Attached patch calendar-snooze-popup.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9045430 - Attachment is obsolete: true
Attachment #9045943 - Flags: review?(philipp)
Attachment #9045943 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9045943 [details] [diff] [review] calendar-snooze-popup.patch Review of attachment 9045943 [details] [diff] [review]: ----------------------------------------------------------------- Looks like it works, thx! For the commit message, always start with the bug number, like Bug 1528201 - ::: calendar/base/content/widgets/calendar-alarm-widget.js @@ +8,5 @@ > > +// Wrap in a block to prevent leaking to window scope. > +{ > + /** > + * To be used as direct child of 'menupopup' or similar. please also add description of what it actually does @@ +122,5 @@ > + let okButton = this.querySelector(".snooze-popup-ok-button"); > + > + const unitName = list => > + ({ 1: "unitMinutes", 60: "unitHours", 1440: "unitDays" }[list.value] || > + "unitMinutes"); I would prefer not using the super short version. (list) => { return ... };
Attachment #9045943 - Flags: review?(mkmelin+mozilla) → review+
Attached patch calendar-snooze-popup-2.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9045943 - Attachment is obsolete: true
Attachment #9045943 - Flags: review?(philipp)
Attachment #9046062 - Flags: review?(philipp)
Comment on attachment 9046062 [details] [diff] [review] calendar-snooze-popup-2.patch Review of attachment 9046062 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these comments. If it turns out you need to remove the inner menupopup, please do give me another chance to review. ::: calendar/base/content/widgets/calendar-alarm-widget.js @@ +19,5 @@ > return; > } > + this.textContent = ""; > + const fragment = MozXULElement.parseXULToFragment( > + ` Nit, you can save a line here by putting the template string opener on the previous line. @@ +31,5 @@ > + <menuitem label="&calendar.alarm.snooze.1day.label;" value="1440" oncommand="snoozeItem(event)"></menuitem> > + <menuseparator></menuseparator> > + <hbox class="snooze-options-box"> > + <textbox class="snooze-value-textbox" oninput="updateAccessibleName()" onselect="updateAccessibleName()" type="number" size="3"></textbox> > + <menulist class="snooze-unit-menulist" allowevents="true"> You should chat with darktrojan about using popups in a popup. I know this was already that way before and it was already flaky, but I think recently something changed that made it less possible. Richard (Paenglab) can help you with UI suggestions. What you could possibly do is turn the popup into a button with a dropdown arrow on it. When you click on it, the contents of the inner popup would instead appear as part of the outer popup, expanding it in size. Hope this explains it somewhat ok. @@ +52,2 @@ > > + const { Preferences } = ChromeUtils.import("resource://gre/modules/Preferences.jsm"); 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. @@ +54,5 @@ > + > + let snoozePref = Preferences.get( > + "calendar.alarms.defaultsnoozelength", > + 0 > + ); This looks like it would all fit on one line. We try to stay under 100 characters in a line. @@ +105,2 @@ > > + snoozeOk() { Can you add jsdoc documentation for all methods that don't belong to custom elements (e.g. skip connectedCallback et al)? @@ +117,3 @@ > > + updateAccessibleName() { > + const { cal } = ChromeUtils.import("resource://calendar/modules/calUtils.jsm"); Same comment about the import.
Attachment #9046062 - Flags: review?(philipp) → review+

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.

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.

Flags: needinfo?(geoff)

(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.)

Attached patch calendar-snooze-popup-3.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9046062 - Attachment is obsolete: true
Blocks: 1530503
Status: NEW → ASSIGNED

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.

Flags: needinfo?(geoff)

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.

Attached patch calendar-snooze-popup-4.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9046429 - Attachment is obsolete: true
Depends on: 1531870
Attached patch calendar-snooze-popup-5.patch (obsolete) (deleted) — — Splinter Review

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...)

Attachment #9047255 - Attachment is obsolete: true
Attachment #9054684 - Flags: review?(philipp)
Comment on attachment 9054684 [details] [diff] [review] calendar-snooze-popup-5.patch Review of attachment 9054684 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/widgets/calendar-alarm-widget.js @@ +18,3 @@ > return; > } > + this.appendChild(MozXULElement.parseXULToFragment(` don't forget to protect against adding more than once @@ +19,5 @@ > } > + this.appendChild(MozXULElement.parseXULToFragment(` > + <menuitem label="&calendar.alarm.snooze.5minutes.label;" > + value="5" > + oncommand="snoozeItem(event)"></menuitem> in xul selfclosing tags like is is the normal way to write them
Attached patch calendar-snooze-popup-6.patch (obsolete) (deleted) — — Splinter Review

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)

Attachment #9054684 - Attachment is obsolete: true
Attachment #9054684 - Flags: review?(philipp)
Attachment #9054690 - Flags: review?(philipp)
Attached patch calendar-snooze-popup-7.patch (obsolete) (deleted) — — Splinter Review

Fixes a mozmill test (testAlarmDialog.js). Try server run.

Attachment #9054690 - Attachment is obsolete: true
Attachment #9054690 - Flags: review?(philipp)
Attachment #9054793 - Flags: review?(philipp)
Attached patch calendar-snooze-popup-8.patch (deleted) — — Splinter Review

Some refactoring (in MozCalendarSnoozePopup connectedCallback) that I got into while working on bug 1530503.

Attachment #9054793 - Attachment is obsolete: true
Attachment #9054793 - Flags: review?(philipp)
Attachment #9056163 - Flags: review?(philipp)
Attachment #9056163 - Flags: review?(philipp) → review+

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.

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.

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 7.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: