Closed Bug 969823 Opened 10 years ago Closed 10 years ago

Attendee Dialog: attendee list entries are not properly aligned with time grid

Categories

(Calendar :: Dialogs, defect)

Lightning 2.6.4
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

1. Create a new event
2. Press invite attendee button


Actual results:

The attendee list box and the free/busy time grid box are top aligned, so the free/busy information of an attendee in the time grid are not displayed in the one row with the respective attendee in the list (see screenshot).


Expected results:

Attendee and his/her free/busy information should be displayed at the row.
Confirmed.
The bug occurs only when setting the zoom to 100%. If the dialog had a different zoom factor, after setting it to 100% it has to be closed and reopened.


It happens because this piece of code is not executed:
http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#86

the cause is the function  scrollToCurrentTime() that doesn't return any value when the zoom is 100% because of timebar.step: 
http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#724

A patch that deletes the "if" block at the line 747
http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#747
would fix the issue because probably it allows to initialize some elements even in the case zoom = 100%, but it needs more investigation.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I agree with comment #2. The "timebar" is'nt yet initialized when trying to read timebar.step and the removal of the mentioned if-block will fix the issue. From what I can see in the code, I wouldn't expect any side effect if we do so.
The problem does not occure, if the event is not within the daystart/-end hours defined in the preferences. In this case setForce24Hours() is called in propagateDateTime(), which ends up in an initialized timebar later on.

The attached patch splits up setZoomFactor() and makes sure the initialiazing part is called onLoad. Just removing the if-block would apply instead on each function call (and there are some in the code).
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8376818 - Flags: review?(bv1578)
Comment on attachment 8376818 [details] [diff] [review]
MakeSureTimebarIsInitializedOnce-V1.diff.diff

The patch works fine, but, if I don't miss something, it seems that this comparison:

>+    if(setZoomFactor(zoom.value) == zoom.value) {

is always "true" because zoom.value is being passed to setZoomFactor() where it is converted from string to integer and returned as it is without additional changes. Is it correct?


Apart from that, I think that reasoning on the zoom is some kind of working around the issue. Yes, it allows to initialize the freebusy-timebar, but actually IMHO it should be already initialized when the code has to use it.

Looking into the file calendar-event-dialog-freebusy.xml, the element that causes the initialization is the method onLoad():

http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml#454

(which is also called from the properties zoomFactor and force24Hours, lines 253 and 277, that's why they cause the initialization), but it seems it's not called from the constructor when the timebar is created:

http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml#411

In the constructor it has been added a listener to the the "load" event that should initialize the free-busy-timebar when the dialog is opened, but this happens only when the code of the function onload() (that one related to the dialog window in the file calendar-event-dialog-attendees.js) has been executed, i.e. when it's too late:

http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#25


A patch like the following seems to fix the bug with nothing else to add but I have to admit I have not fully understood if there is a reason why in the constructor the method onLoad() has not been directly called:

diff --git a/calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml b/calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml
--- a/calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml
+++ b/calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml
@@ -423,8 +423,10 @@
         this.startDate = startTime.getInTimezone(kDefaultTimezone);
         this.endDate = endTime.getInTimezone(kDefaultTimezone);
         this.mRange = Number(this.getAttribute("range"));
 
+        this.onLoad();
+
         var self = this;
         var load = function loadHandler() {
             self.onLoad();
         };


might you be so kind to try to investigate a bit in this direction too?
The problem was introduced with adding the scrollToCurrentTime() to onLoad() in calendar-event-dialog-attendees.js with this changeset: http://hg.mozilla.org/comm-central/rev/ec54df727248

I think the your proposed patch would be the best way to resolve the issue at its root. Do we still need the loadHandler after adding this.onLoad() - as the timbar would be initialized then in any case?
Flags: needinfo?(philipp)
I've taken a look into this strange onLoad thing, here are a few more details as to why it has been written this way. The code dates back to gecko 1.8, some ugly workarounds were needed to make sure sizing works at all.

The code does a lot of dynamic resizing and tries to separate logical elements into different XBL bindings as far as possible. The separating is generally good, but the dynamic resizing is pretty painful. Using a xul:grid would allow automatically aligning the panes, but iirc this didn't work with the splitter to allow changing the width.

When the dialog loads, the binding is attached and constructed before page load completes. At this time, the page layout has not been calculated, therefore properties like contentWidth and template.dayHeight are zero. I also get this with the suggested patch and removing the extra "load" event handler.

Nevertheless, I am very much in favor of removing that load handler, I am just not sure how to do it yet. From simple testing, it seems that if onLoad is called using setTimeout(() => this.onLoad(), 0) in the constructor, it also works. This is quite fragile though because we never know how early it will actually run. 

Next up you could move the code related to resizing those elements outside of the xbl bindings and into calendar-event-dialog-attendees.js. This would also improve reusability because all this resizing is pretty specific to the attendees dialog.

Another way we could potentially fix this is to avoid the need for these calculations altogether. This requires a fair bit of experimenting, but would improve the code quality by far. Its quite possible that some of the workarounds we needed back then are no longer needed. You could experiment with different layouts of the xul:grid, which can be filled with content the way we have it in other dialogs, but also by adding the content in the <column> instead. Its also possible to add elements other than <row> as a child to the <rows> element (same for <columns>). Other possibilities include using flex="1" and/or equalsize="always" on various elements where it makes sense. There are likely more options here. 


PS: Here is my long term vision for this dialog, I just thought I'd let you know :-)
We should turn the freebusy view into an endless scrolling pane that covers the whole window width. The mouse wheel would horizontally scroll the view, revealing new freebusy periods as we go. Zooming works with Cmd+Scroll and allows a lot more levels of granularity. For better re-use, that endless scrolling pane should be its own generic binding, maybe one day we can use it in the views too. The day headers and the freebusy rows should use a xul:grid, this would avoid misalignment issues I currently see. I'm not quite clear on how to align the attendee-list, but I was thinking about sticking it inside the endless view and having it auto-collapse to only show the name part in name@example.com. It would overlap the actual view, but since its endless scrolling thats ok and provides a great experience when it is resized or auto-collapsed.
Flags: needinfo?(philipp)
@ Philipp
Thanks for your explanation.
Changing the dialog's structure in order to avoid dynamic resizing and related calculation sounds a more difficult and insidious work, and long as well. In this case, I think would be better restoring the functioning as before with the patch proposed by MakeMyDay (which might be rigth for the 2.6.5 too because basically it should restore the behavior as it has ever been before the patch for bug 694024) and opening another bug for a different XUL structure for the attendee dialog.
Do you agree?
(In reply to Decathlon from comment #9)
> @ Philipp
> Thanks for your explanation.
> Changing the dialog's structure in order to avoid dynamic resizing and
> related calculation sounds a more difficult and insidious work, and long as
> well. In this case, I think would be better restoring the functioning as
> before with the patch proposed by MakeMyDay (which might be rigth for the
> 2.6.5 too because basically it should restore the behavior as it has ever
> been before the patch for bug 694024) and opening another bug for a
> different XUL structure for the attendee dialog.
> Do you agree?

Sounds good, lets do that. I believe there was still a review comment to take care of, so I'll leave the review and followup filing to you two.
Updated patch with a modified comparison.
Attachment #8376818 - Attachment is obsolete: true
Attachment #8376818 - Flags: review?(bv1578)
Attachment #8402403 - Flags: review?(bv1578)
Just for the records: I guess bug 993881 not only covers the alignment issue. Pat Malay (reporter) also discribed an organizer duplicate in the attendee list - I don't see this here based on the bugs STR, did you, Decathlon? If so - or if Pat can eleborate the STR with scope to that issue, we should file that as a separate bug.
It looks like the organizer is an attendee in that case, this might be done by the caldav server in case its a modification?
Maybe it's the caldav server, maybe it's one of the edge cases I filed in bug 994205 or maybe it's an add-on (I noticed there is an additional label/link in the lower left corner of the screenshot in bug 993881. Or is that related to caldav scheduling?)
(In reply to MakeMyDay from comment #13)
> Just for the records: I guess bug 993881 not only covers the alignment
> issue.

You are right, I've just changed the status to unconfirmed.
Comment on attachment 8402403 [details] [diff] [review]
MakeSureTimebarIsInitializedOnce-V2.diff

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

Looks good and works fine.


This patch seemed causing a wrong time positioning of the freebusy-timebar when the zoom is 100% and for events with start/end time such as 23:00-0:00 because the positioning doesn't allow to see the time slot without scrolling to the left (the same problem doesn't happen without the patch), but actually I've found out this is caused by the fact that the timebar doesn't expand to 24 hours in that case. I'm going to open a bug about that.
Attachment #8402403 - Flags: review?(bv1578) → review+
@Philipp: did comment #10 include the approval for backporting to 2.6.5 as proposed by Decathlon?
Flags: needinfo?(philipp)
Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/2fd4762ea2bb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.3
Attachment #8402403 - Flags: approval-calendar-release?(philipp)
Attachment #8402403 - Flags: approval-calendar-beta?(philipp)
Attachment #8402403 - Flags: approval-calendar-aurora?(philipp)
Comment on attachment 8402403 [details] [diff] [review]
MakeSureTimebarIsInitializedOnce-V2.diff

Wasn't backported yet, feel free to push this to all branches.
Attachment #8402403 - Flags: approval-calendar-release?(philipp)
Attachment #8402403 - Flags: approval-calendar-release+
Attachment #8402403 - Flags: approval-calendar-beta?(philipp)
Attachment #8402403 - Flags: approval-calendar-beta+
Attachment #8402403 - Flags: approval-calendar-aurora?(philipp)
Attachment #8402403 - Flags: approval-calendar-aurora+
Flags: needinfo?(philipp)
You need to log in before you can comment on or make changes to this bug.