1512807 - [de-xbl] Migrate modebox to a custom element

Status: RESOLVED FIXED

(Calendar :: Lightning Only, task)

Comment 1

[Arshad Khan [:arshad]]

Attachment: modebox_modevbox.patch

Comment 2

[Arshad Khan [:arshad]]

Attachment: modebox_modevbox_WIP.patch

something is off, the task toolbox's collapsed attr is not set true..

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9030095 [details] [diff] [review]
modebox_modevbox_WIP.patch

Review of attachment 9030095 [details] [diff] [review]:
-----------------------------------------------------------------

applying timepicker-grids-combined.patch
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh timepicker-grids-combined.patch

::: calendar/base/content/today-pane.xul
@@ +26,5 @@
>  
> +  <modebox id="today-pane-panel" orient="vertical"
> +           mode="mail,calendar,task" modewidths="200,200,200" modesplitterstates="open,open,open"
> +           refcontrol="calendar_toggle_todaypane_command"
> +           broadcaster="modeBroadcaster" persist="modewidths">

don't know what changed in these hunks, seems like nothing?

::: calendar/base/content/widgets/calendar-widgets.js
@@ +31,5 @@
> +        if (this.broadcaster && this.broadcaster.hasAttribute("mode")) {
> +            return this.broadcaster.getAttribute("mode");
> +        } else {
> +            return "";
> +        }

while you're here, no else after return. so

if (...)
  return...
return "";

@@ +44,5 @@
> +        return !collapsedModes.includes(lMode);
> +    }
> +
> +    setModeAttribute(modeAttribute, modeValue, amode) {
> +        if (this.hasAttribute(modeAttribute)) {

we could just do 

if (!this.hasAttribute(modeAttribute)) {
  return
}

@@ +54,5 @@
> +        }
> +    }
> +
> +    getModeAttribute(modeAttribute, attribute, amode) {
> +        if (this.hasAttribute(modeAttribute)) {

here too

@@ +61,5 @@
> +            let modes = this.getAttribute("mode").split(",");
> +            return modeAttributeValues[modes.indexOf(lMode)];
> +        } else {
> +            return "";
> +        }

no else after return

@@ +98,5 @@
> +                }
> +            }
> +            this.setAttribute("collapsedinmodes", collapsedModes.join(","));
> +
> +            // This binding is used all over the place. We can't guarantee that Services is available.

I think it's safe to assume Services is available. If not, let's import it at the start of the file

@@ +103,5 @@
> +            ChromeUtils.import("resource://gre/modules/Services.jsm");
> +            Services.xulStore.persist(this, "collapsedinmodes");
> +        }
> +        if (notifyRefControl === true) {
> +            if (this.hasAttribute("refcontrol")) {

please combine this ifs into one

::: mail/base/content/customElements.js
@@ +10,5 @@
>    "chrome://messenger/content/mailWidgets.js",
>    "chrome://messenger/content/generalBindings.js",
>    "chrome://messenger/content/statuspanel.js",
>    "chrome://messenger/content/foldersummary.js",
> +  "chrome://calendar/content/widgets/calendar-widgets.js",

this is lightning, so load from there

Comment 4

[Magnus Melin [:mkmelin]]

The part about the patch not applying got pasted into the wrong bug. 
But the patch here doesn't apply at all. Almost all hunks fail

Comment 5

[Arshad Khan [:arshad]]

Attachment: modebox_wip.patch

Comment 6

[Magnus Melin [:mkmelin]]

Over to Khushil, since Arshad is not working for Thunderbird anymore.
There is no direct rush with this bug, since it doesn't depend on other bindings.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part1-modebox-de-xbl-0.patch

1 of 2. This is the main one. Some thoughts:

I thought about renaming "modebox" to "modehbox" for a clearer parallel/contrast with "modevbox", but have left it as it is.

It's a bit awkward to have the tiny calendar-modebox.css file that has to accompany the custom element file. (That CSS-in-JS approach may be on to something.) I was tempted to set those styles in the connectedCallback (as an inline "style" property), but left it as is.

Comment 8

[Paul Morris [:pmorris]]

Attachment: part2-fix-xul-indentation-0.patch

2 of 2. Just a minor XUL indentation fix.

Comment 9

[Paul Morris [:pmorris]]

Try server run looks good: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0f780244320ce870a89cc376d8c0294feab433bb

Comment 10

[Geoff Lankow (:darktrojan)]

I'd like to have a close look at this before it lands, setting NI? on myself because I'm not going to do it now.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9080020 [details] [diff] [review]
part1-modebox-de-xbl-0.patch

Review of attachment 9080020 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/widgets/calendar-modebox.js
@@ +281,5 @@
> +            }
> +        }
> +    }
> +
> +    customElements.define("modebox", CalendarModebox);

we should take the opportunity to rename them to calnendar-modebox
CEs need to have a hypen in their name (and some things will break when they do not)

Comment 12

[Geoff Lankow (:darktrojan)]

A few comments. I was going to assign this bug to myself, if you hadn't been working on it already.

• I think we can get rid of -moz-user-focus: normal. I'm not sure why it exists but there's no need (IMO) for any of these elements to be focusable.
• We can get rid of -moz-box-orient: vertical – do this.orient = "vertical" or this.setAttribute("orient", "vertical") where necessary.
• With those gone the stylesheet is unnecessary.
• There's no need for a subclass for vertical boxes (unless custom elements doesn't allow defining two things to the same class, which would be weird) – you can check this.localName to find out which one you have, which is only needed to set the orientation.
• I don't like the use of DOMAttrModified event, which has been considered a bad idea™ since ages ago. Also I wouldn't be surprised if <broadcaster> and <observer> are endangered species. These are the problems I came here to look at, but I'll deal with them in another bug.

Comment 13

[Paul Morris [:pmorris]]

Attachment: modebox-de-xbl-1.patch

Thanks for the review and the bonus comments.

(In reply to Magnus Melin [:mkmelin] from comment #11)

we should take the opportunity to rename them to calnendar-modebox
CEs need to have a hypen in their name (and some things will break when they
do not)

Good call, and done. And now there's only a single patch since the rename involved re-indentation of the code that was in the part2 patch.

(In reply to Geoff Lankow (:darktrojan) from comment #12)

• I think we can get rid of -moz-user-focus: normal. I'm not sure why it exists but there's no need (IMO) for any of these elements to be focusable.
• We can get rid of -moz-box-orient: vertical – do this.orient = "vertical" or this.setAttribute("orient", "vertical") where necessary.
• With those gone the stylesheet is unnecessary.

That is much better, thanks.

• There's no need for a subclass for vertical boxes (unless custom elements doesn't allow defining two things to the same class, which would be weird) – you can check this.localName to find out which one you have, which is only needed to set the orientation.

I had tried this before and turns out different Custom Elements can't use the same class, for some silly reason. You get an error like:
NotSupportedError: 'calendar-modevbox' and 'calendar-modebox' have the same constructor
So I've kept the subclass and put setting its orient to vertical in its connectedCallback.

• I don't like the use of DOMAttrModified event, which has been considered a bad idea™ since ages ago. Also I wouldn't be surprised if <broadcaster> and <observer> are endangered species. These are the problems I came here to look at, but I'll deal with them in another bug.

Sounds good. I've seen the bug about <broadcaster> going away and was thinking that we'd need to revisit this as part of that.

Comment 14

[Paul Morris [:pmorris]]

Comment on attachment 9080813 [details] [diff] [review]
modebox-de-xbl-1.patch

Oh wait... mkmelin's r+ was only for the old obsolete part2 patch, so re-requesting review for this one.

Comment 15

[Magnus Melin [:mkmelin]]

Re broadcaster, I'm working on the last pieces of removing that in bug 1567424.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9080813 [details] [diff] [review]
modebox-de-xbl-1.patch

Review of attachment 9080813 [details] [diff] [review]:
-----------------------------------------------------------------

It seems that for multi-week and month views, rotate doesn't work properly. Rotate View should apparently be disabled for those, it isn't with this patch). And also something fishy for the other views with rotate, and then trying to disable it again. (Not sure about the details, and could be resolved by resolving the other problem?)

::: calendar/base/content/widgets/calendar-modebox.js
@@ +6,2 @@
>  
> +/* globals MozXULElement Services setBooleanAttribute */

instead of putting Services as global, please import Services (inside the block)

Comment 17

[Magnus Melin [:mkmelin]]

Found this in the console, maybe related

JavaScript error: chrome://calendar/content/calendar-multiday-base-view.js, line 1002: TypeError: item is undefined
JavaScript error: chrome://calendar/content/calendar-common-sets.js, line 941: TypeError: item is undefined

Comment 18

[Paul Morris [:pmorris]]

Attachment: modebox-de-xbl-2.patch

(In reply to Magnus Melin [:mkmelin] from comment #16)

It seems that for multi-week and month views, rotate doesn't work properly.
Rotate View should apparently be disabled for those, it isn't with this
patch). And also something fishy for the other views with rotate, and then
trying to disable it again. (Not sure about the details, and could be
resolved by resolving the other problem?)

Hm, I wasn't able to reproduce this. I'm using artifact builds and when I built with this patch on top of this changeset:

(050d0ef127f2) Bug 1569399 - openConversationButton.tooltip changed to openMsgConversationButton.tooltip for messenger.dtd to fix...

then rotate view is disabled (in appmenu and menubar view menu) for month and multi-week, and works as expected for day and week views. I also didn't see any errors appearing in the console. Any ideas?

instead of putting Services as global, please import Services (inside the block)

OK, done in this new version of the patch.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9081626 [details] [diff] [review]
modebox-de-xbl-2.patch

Review of attachment 9081626 [details] [diff] [review]:
-----------------------------------------------------------------

Can't reproduce the problem now, so looks good to me. I'll let Geoff to the final review

::: calendar/base/content/widgets/calendar-modebox.js
@@ +203,2 @@
>              }
> +            if (modes && modes.length > 0) {

modes will always be true, and then you don't really need the length > 0 test either

Comment 20

[Magnus Melin [:mkmelin]]

I suppose it's possible the problem could have been when I run with the patch the first time in the profile, related to mode persistence? Can you double check that works.

Comment 21

[Paul Morris [:pmorris]]

Attachment: modebox-de-xbl-3.patch

(In reply to Magnus Melin [:mkmelin] from comment #19)

modes will always be true, and then you don't really need the length > 0 test either

Thanks, I've made this change, simplified the function.

(In reply to Magnus Melin [:mkmelin] from comment #20)

I suppose it's possible the problem could have been when I run with the patch the first time in the profile, related to mode persistence? Can you double check that works.

Good thought. I checked this and wasn't able to reproduce the problem. I tried a couple things:

• Starting with a fresh profile with the patch applied, and restarting to install calendar.
• Starting with another fresh profile without the patch applied, restarting to install calendar, then applying the patch and running with that same profile.

Comment 22

[Geoff Lankow (:darktrojan)]

Comment on attachment 9081978 [details] [diff] [review]
modebox-de-xbl-3.patch

Review of attachment 9081978 [details] [diff] [review]:
-----------------------------------------------------------------

I'm happy with where this is heading but there's some tidying up to do.

::: calendar/base/content/widgets/calendar-modebox.js
@@ +59,5 @@
> +                    this.mControlHandler = {
> +                        modebox: this,
> +                        handleEvent: function(event, handled) {
> +                            return this.modebox.onCheckboxStateChange(event, this.modebox);
> +                        }

Add a handleEvent function to CalendarModebox that checks event.type and passes event on to the right function. The we can stop mucking around with handler objects and just pass 'this' to add/removeEventListener. Same goes for the DOMAttrModified handling.

@@ +69,2 @@
>  
> +            this.dispatchEvent(new CustomEvent("bindingattached", { bubbles: false }));

This event can be removed. It's a hack to ensure an XBL binding has loaded when we need it.

@@ +86,5 @@
> +         * @param {string} modeAttribute  A mode attribute in which to set a new value.
> +         * @param {string} value  A new value to set.
> +         * @param {string} [mode=this.currentMode]  Set the value for this mode.
> +         */
> +        setModeAttribute(modeAttribute, value, mode = this.currentMode) {

Can we rename modeAttribute to attributeName? This is confusing.

@@ +106,5 @@
> +         * @param {string} modeAttribute  A mode attribute to get a value from.
> +         * @param {string} [mode=this.currentMode]  Get the value for this mode.
> +         * @return {string}  The value found in the mode attribute or an empty string.
> +         */
> +        getModeAttribute(modeAttribute, mode = this.currentMode) {

Same here.

@@ +139,5 @@
>                  collapsedInMode = modeIndex > -1;
>              }
> +
> +            const display = (visible === true && !pushModeCollapsedAttribute) ?
> +                (!collapsedInMode) : visible;

I think I prefer this as a block, it's more readable:

if (display && …
    display = …

@@ +195,5 @@
> +         * @return {boolean}  Whether this modebox is visible for the given mode.
> +         */
> +        isVisibleInMode(mode = this.currentMode) {
> +            return this.hasAttribute("mode") ?
> +                this.getAttribute("mode").split(",").includes(mode) : false;

It seems this should default to true. I'm not sure if we're ever in a state where there is no mode attribute, but just in case we'd better keep the same behaviour.

@@ +204,5 @@
> +         *
> +         * @param {Event} event  An event, e.g. "DOMAttributeModified".
> +         * @param {CalendarModebox} modebox  A modebox element to make visible or not.
> +         */
> +        onModeModified(event, modebox) {

modebox will always be 'this'.

@@ +219,5 @@
> +         * @param {Event} event  An event with a command (with a checked attribute) as its target.
> +         */
> +        togglePane(event) {
> +            let command = event.target;
> +            let newValue = command.getAttribute("checked") == "true" ? "false" : "true";

It's common in our code to put brackets around the first part, to make it more obvious what's happening, like this:
(command.getAttribute("checked") == "true") ? "false" : "true"

I'm not sure why the brackets in the original line are the way they are.

@@ +230,5 @@
> +         *
> +         * @param {Event} event  An event with a target that has a `checked` attribute.
> +         * @param {CalendarModebox} modebox  A modebox.
> +         */
> +        onCheckboxStateChange(event, modebox) {

modebox is unused, and it's always 'this' anyway.

@@ +256,4 @@
>                      this.mModHandler = {
> +                        modebox: this,
> +                        handleEvent: function(event, handled) {
> +                            return this.modebox.onModeModified(event, this.modebox);

See the comment about handlers above.

@@ +261,5 @@
>                      };
>                      this.mBroadcaster.addEventListener("DOMAttrModified", this.mModHandler, true);
>                  }
>              }
> +            return XULElement.prototype.setAttribute.call(this, attr, val);

I'm guessing you could not use super.setAttribute?

Comment 23

[Paul Morris [:pmorris]]

Attachment: modebox-de-xbl-4.patch

Thanks for the review.

(In reply to Geoff Lankow (:darktrojan) from comment #22)

Add a handleEvent function to CalendarModebox that checks event.type and
passes event on to the right function. The we can stop mucking around with
handler objects and just pass 'this' to add/removeEventListener. Same goes
for the DOMAttrModified handling.

Good call, this is much better!

This event can be removed. It's a hack to ensure an XBL binding has loaded
when we need it.

Done.

Can we rename modeAttribute to attributeName? This is confusing.

Done.

I think I prefer this as a block, it's more readable:

Done.

It seems this should default to true. I'm not sure if we're ever in a state
where there is no mode attribute, but just in case we'd better keep the same
behaviour.

Oooh, good catch, fixed.

modebox will always be 'this'.

Ok, dropped the modebox arg to use 'this' instead.

It's common in our code to put brackets around the first part, to make it
more obvious what's happening, like this:
(command.getAttribute("checked") == "true") ? "false" : "true"

Ok, done.

modebox is unused, and it's always 'this' anyway.

Removed.

       return XULElement.prototype.setAttribute.call(this, attr, val);

I'm guessing you could not use super.setAttribute?

Well, I just hadn't changed the code from the XBL binding version. I've switched it to super.setAttribute now. Good call.

Comment 24

[Geoff Lankow (:darktrojan)]

Comment on attachment 9082725 [details] [diff] [review]
modebox-de-xbl-4.patch

Review of attachment 9082725 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, just watch your comment line-length. Some of them are creeping past 100 characters in a line.

Comment 25

[Paul Morris [:pmorris]]

Attachment: modebox-de-xbl-5.patch

(In reply to Geoff Lankow (:darktrojan) from comment #24)

Thanks, just watch your comment line-length. Some of them are creeping past
100 characters in a line.

Huh, then my editor is lying to me. It said I was right up to 100 but not past. I re-formatted a few to make sure they're under 100.

Comment 26

[Paul Morris [:pmorris]]

Edit: just setting checkin-needed.

Comment 27

[Paul Morris [:pmorris]]

Forgot to post this link to successful try server run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=97fc7770f89d3965b0efa9cf49ba68f62283809e

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1879fcb9f785
[de-xbl] Migrate modebox and modevbox to custom elements. r=darktrojan,mkmelin