Closed Bug 1512807 Opened 6 years ago Closed 5 years ago

[de-xbl] Migrate modebox to a custom element

Categories

(Calendar :: Lightning Only, task)

Product:
Calendar
Component:
Lightning Only
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 70.0

People

(Reporter: arshad, Assigned: pmorris)

References

Details

Attachments

(1 file, 9 obsolete files)

modebox-de-xbl-5.patch
(deleted), patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Attached patch modebox_modevbox.patch (obsolete) (deleted) — — Splinter Review
Attached patch modebox_modevbox_WIP.patch (obsolete) (deleted) — — Splinter Review
something is off, the task toolbox's collapsed attr is not set true..
Attachment #9030091 - Attachment is obsolete: true
Blocks: tb-war-on-xbl
Attachment #9030095 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9030095 - Flags: feedback?(mkmelin+mozilla)
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
Attached patch modebox_wip.patch (obsolete) (deleted) — — Splinter Review
Attachment #9030095 - Attachment is obsolete: true

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.

Assignee: arshdkhn1 → khushil324
Status: ASSIGNED → NEW
Type: enhancement → task
Assignee: khushil324 → paul
Summary: [dexbl] Remove modevbox binding and migrate modebox to custom element. → [de-xbl] Remove modevbox binding and migrate modebox to custom element.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1484653
Summary: [de-xbl] Remove modevbox binding and migrate modebox to custom element. → [de-xbl] Migrate modebox to a custom element
Attached patch part1-modebox-de-xbl-0.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9080020 - Flags: review?(mkmelin+mozilla)
Attachment #9069807 - Attachment is obsolete: true
Attached patch part2-fix-xul-indentation-0.patch (obsolete) (deleted) — — Splinter Review

2 of 2. Just a minor XUL indentation fix.

Attachment #9080022 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED

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.

Flags: needinfo?(geoff)
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)
Attachment #9080022 - Flags: review?(mkmelin+mozilla) → review+

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.
Flags: needinfo?(geoff)
Attached patch modebox-de-xbl-1.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9080020 - Attachment is obsolete: true
Attachment #9080022 - Attachment is obsolete: true
Attachment #9080020 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9080813 - Flags: review?(mkmelin+mozilla)

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

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)
Attachment #9080813 - Flags: review?(mkmelin+mozilla) → review-

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

Attached patch modebox-de-xbl-2.patch (obsolete) (deleted) — — Splinter Review

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

Attachment #9080813 - Attachment is obsolete: true
Attachment #9081626 - Flags: review?(mkmelin+mozilla)
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
Attachment #9081626 - Flags: review?(mkmelin+mozilla)
Attachment #9081626 - Flags: review?(geoff)
Attachment #9081626 - Flags: feedback+

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.

Attached patch modebox-de-xbl-3.patch (obsolete) (deleted) — — Splinter Review

(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.
Attachment #9081626 - Attachment is obsolete: true
Attachment #9081626 - Flags: review?(geoff)
Attachment #9081978 - Flags: review?(geoff)
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?
Attachment #9081978 - Flags: review?(geoff) → feedback+
Attached patch modebox-de-xbl-4.patch (obsolete) (deleted) — — Splinter Review

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.

Attachment #9081978 - Attachment is obsolete: true
Attachment #9082725 - Flags: review?(geoff)
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.
Attachment #9082725 - Flags: review?(geoff) → review+
Attached patch modebox-de-xbl-5.patch (deleted) — — Splinter Review

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

Attachment #9082725 - Attachment is obsolete: true

Edit: just setting checkin-needed.

Keywords: checkin-needed

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

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

Attachment

General

Creator:
Created:
Updated:
Size: