[de-xbl] convert the dragndropContainer binding and its two derivatives calendar-header-container and calendar-month-day-box

RESOLVED FIXED in 7.1

Status

task
RESOLVED FIXED
2 months ago
16 days ago

People

(Reporter: mkmelin, Assigned: aleca)

Tracking

(Blocks 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 6 obsolete attachments)

Assignee: nobody → alessandro

I need a little bit of help with this one.
Overall, I'm uploading the WIP patch to gather feedback regarding the approach, if the abstract class is correct and everything looks properly written.

Onto the problem I have.
The XBL version of this uses the <children/> markup, which automatically adds and removes child nodes in that location.
This used to work perfectly with the current structure, even if in the calendar box we have other child nodes, like the week and day label.

After converting this to CE, it doesn't work anymore as the child nodes (events) are appended in the wrong location, and when a date is set and all the child should be removed, also the week and day label are removed.

I tried to update the code in order to properly append the events inside the day-items container, but that causes many problems since every single event is tightly bounded to its parent node and its attributes.

I need some feedback regarding the proper approach to follow, since any sort of change to the way the events are appended to the calendar box will affect many files.

Attachment #9072390 - Flags: feedback?(philipp)
Attachment #9072390 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9072390 [details] [diff] [review]
dexbl-dragndropcontainer.patch

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

You write "ffter converting this to CE, it doesn't work anymore as the child nodes (events) are appended in the wrong location, and when a date is set and all the child should be removed, also the week and day label are removed. "

But why does that happen?

::: calendar/base/content/calendar-base-view.js
@@ +908,4 @@
>          }
>  
>          removeDropShadows() {
> +            const dropbox = document.querySelectorAll("[dropbox=\"true\"]");

should be this.querySelectorAll, I think

for readability when there are quotes, you can use ` instaed of escaping, like querySelectorAll(`[dropbox="true"]`);

that also works with forEach, so you can 

this. querySelectorAll(`[dropbox="true"]`).forEach((d) => {
  d.setAttribute("dropbox", "false");
});

::: calendar/base/content/widgets/calendar-dnd-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/. */
> +
> +/* globals cal currentView MozElements MozXULElement Services */

you should instead import the cal and Services modules

@@ +53,5 @@
> +            return this.mDropShadows;
> +        }
> +
> +        // Adds the dropshadows to the children of the binding. The dropshadows
> +        // are added at the first position of the children

Please change the method documentation style to jsdoc style /** */. 
Also proper sentences with capital first letter, and period at end.

@@ +273,5 @@
> +            monthDayLabels.appendChild(weekLabel);
> +            monthDayLabels.appendChild(dayLabel);
> +
> +            this.dayItems = document.createXULElement("vbox");
> +            this.dayItems.setAttribute("anonid", "day-items");

please remove the anonid, that's basically used for ids in anonymous content (== xbl).

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

You write "after converting this to CE, it doesn't work anymore as the child
nodes (events) are appended in the wrong location, and when a date is set
and all the child should be removed, also the week and day label are
removed. "

But why does that happen?

I know it seems weird, let me give you some visual examples and see if I can explain myself properly.

This is the current XBL structure of the calendar box: https://searchfox.org/comm-central/source/calendar/base/content/calendar-month-view.xml#125

As you can see inside the main container, we have an hbox to render the week and day label, then a scrollable area wrapping around the <children/> tag to accommodate all the events.

When the calendar is generated, or a new event is set, all the previous events in each box are removed: https://searchfox.org/comm-central/source/calendar/base/content/calendar-month-view.xml#210

Moreover, when a new item is added, the calendar expects the event to have the main day box container as its parent, in order to inherit the necessary attributes, even if the actual event ends up being appended in the <children/> tag location: https://searchfox.org/comm-central/source/calendar/base/content/calendar-month-view.xml#248

This approach doesn't work in a CE because:

  • The removeChildren() method in a CE removes everything, including the other child nodes for the week and day label.
  • The new items are appended after all the other child nodes, outside the supposed scrollable area that needs to accommodate them.
  • Updating the code to remove and append the events inside the this.dayItems element, creates a cascading series of bugs because the entire calendar is wired to tap the events as direct children of the main day box container.

Therefore my question regarding what should I do. Should I go around the whole calendar and consequentially update all the methods interacting with events, or is there something obvious I'm missing and this is just a mess in my head?

for readability when there are quotes, you can use ` instaed of escaping,

I originally did that, but then I was having eslint flagging it as linting error as it expects double quotes for strings.
Do I ignore it?

Flags: needinfo?(philipp)
Flags: needinfo?(mkmelin+mozilla)

I think I identified the core problem and found a solution.
The main issue I was having is the fact that the same abstract class is used by the header container and the single day box, which one has no child nodes while the other has a whole different structure with nested nodes.
I implemented a couple of extra conditions to handle the exceptions and everything seems to work.

Attachment #9072390 - Attachment is obsolete: true
Attachment #9072390 - Flags: feedback?(philipp)
Attachment #9072390 - Flags: feedback?(mkmelin+mozilla)
Flags: needinfo?(philipp)
Flags: needinfo?(mkmelin+mozilla)
Attachment #9073695 - Flags: review?(philipp)
Attachment #9073695 - Flags: review?(mkmelin+mozilla)

I have a couple of failing mozmill tests due to the change of container.
Fixing those right now.

Done!

Attachment #9073695 - Attachment is obsolete: true
Attachment #9073695 - Flags: review?(philipp)
Attachment #9073695 - Flags: review?(mkmelin+mozilla)
Attachment #9073911 - Flags: review?(philipp)
Attachment #9073911 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9073911 [details] [diff] [review]
dexbl-dragndropcontainer.patch

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

I think it should be  calendar-dnd-widgets.js (with an s.)

Looks good, afaict. I've done some unbitrotting, so attaching a patch for that next.

Please check the comments below and let Philipp have a look then

::: calendar/base/content/widgets/calendar-dnd-widget.js
@@ +9,5 @@
> +"use strict";
> +
> +// Wrap in a block to prevent leaking to window scope.
> +{
> +    var { cal } = ChromeUtils.import("resource://calendar/modules/calUtils.jsm");

please add a blank line after

@@ +11,5 @@
> +// Wrap in a block to prevent leaking to window scope.
> +{
> +    var { cal } = ChromeUtils.import("resource://calendar/modules/calUtils.jsm");
> +    /**
> +     * Creates a new abstract class to handle the calendar drag and drop

An abstract class to handle drag on drop for calendar.

(Please add final periods for all the doc comment sentences.)

@@ +36,5 @@
> +            this.mCalendarView = null;
> +        }
> +
> +        /**
> +         * The ViewController that supports the interface 'calICalendarView'

@returns {calICalendarView}

@@ +62,5 @@
> +         * Adds the dropshadows to the children of the binding.
> +         * The dropshadows are added at the first position of the children.
> +         */
> +        addDropShadows() {
> +            if (this.mDropShadows) {

you could change this to return early instead

@@ +228,5 @@
> +            currentView().removeDropShadows();
> +        }
> +    }
> +
> +    MozElements.CalendarDnDContainer = CalendarDnDContainer;

why this?
Attachment #9073911 - Flags: review?(philipp)
Attachment #9073911 - Flags: review?(mkmelin+mozilla)
Attachment #9073911 - Flags: feedback+
Attachment #9073911 - Attachment is obsolete: true
Attachment #9074700 - Flags: feedback+
Attachment #9074700 - Attachment is obsolete: true
Attachment #9074701 - Flags: feedback+

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

I think it should be calendar-dnd-widgets.js (with an s.)

Updated!

Looks good, afaict. I've done some unbitrotting, so attaching a patch for
that next.

Thanks for doing that.

@@ +62,5 @@

  addDropShadows() {
      if (this.mDropShadows) {

you could change this to return early instead

Good call, done.

@@ +228,5 @@
MozElements.CalendarDnDContainer = CalendarDnDContainer;

why this?

A leftover from when I started working on the patch as I was expecting to be needing to extend this class on multiple other files.

Ready for a full review from Philipp.

Attachment #9074701 - Attachment is obsolete: true
Attachment #9074853 - Flags: review?(philipp)
Attachment #9074853 - Flags: feedback+
Comment on attachment 9074853 [details] [diff] [review]
dexbl-dragndropcontainer.patch

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

Maybe see if you can preserve some history using hg copy. I'm not quite sure that works though. r=philipp with that and these changes:

::: calendar/base/content/calendar-base-view.js
@@ +908,4 @@
>          }
>  
>          removeDropShadows() {
> +            this.querySelectorAll(`[dropbox="true"]`).forEach(dbox => {

I think we can go with "[dropbox='true']" here, as you do later on.

::: calendar/base/content/widgets/calendar-dnd-widgets.js
@@ +240,5 @@
> +     * Implements the Drag and Drop class for the Month Day Box view.
> +     *
> +     * @extends {CalendarDnDContainer}
> +     */
> +    class CalendarMonthDayBox extends CalendarDnDContainer {

I think I'd rather see the remaining classes in a different file. The parent is definitely a dnd-widget, but the month day boxes and such have a lot of other non-dnd functionality.
Attachment #9074853 - Flags: review?(philipp) → review+

I updated the patch as suggested.
After unbitrotting, also a lot of mozmill tests were failing. I fixed those.

I'm requesting a review from Magnus and Philipp again since some substantial things have changed.

Here's the try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7b79b5c460c2a2cc39969836298731d493392a37

Attachment #9074853 - Attachment is obsolete: true
Attachment #9075256 - Flags: review?(philipp)
Attachment #9075256 - Flags: review?(mkmelin+mozilla)
Attachment #9075256 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9075256 [details] [diff] [review]
dexbl-dragndropcontainer.patch

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

Looks fine to me. Could you check if hg copy will preserve history for the new files? You may have answered this in another bug but I haven't checked all my bugmail yet.
Attachment #9075256 - Flags: review?(philipp) → review+

No, it doesn't you need to rename the file.

EDIT: Oops, spoke too early. Rename (or copy) can be used for a straight XML to JS translation. But here, three bindings are removed from three different XML files leaving some content in those files and a JS file is created instead. So there's no option to just creating the new file.

Alex, can you please submit your patches with 8 lines of context. Add this to your mercurial.ini:
[diff]
git = 1
showfunc = 1
unified = 8

At times I need to compare patches and they are totally different if they have different lines of context.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5ce69de5c6c6
[de-xbl] convert the dragndropContainer binding. r=mkmelin,philipp DONTBUILD

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