Closed Bug 1477478 Opened 3 years ago Closed 3 years ago

The event attendees dialog contains three listboxes


(Calendar :: Dialogs, enhancement)

Not set


(Not tracked)



(Reporter: darktrojan, Assigned: darktrojan)




(1 file, 4 obsolete files)

As we all know, listboxes aren't a thing any more. I thought I'd better file a bug on this before somebody else wastes more time fixing it.

Warning: monster patch incoming, when I've finished it.
Yes, I've convert the attendees-lsit already here already, fb is a little more challenging. However, I'm fine if you fix it, but keep it simple for the bustage fix - we need to rework the whole thing anyway. There is bug 995746 on file for it.
Blocks: 1447830
I can't count.
Summary: The event attendees dialog contains two listboxes → The event attendees dialog contains three listboxes
Attached patch 1477478-attendees-1.diff (obsolete) — Splinter Review
I wasn't going to post this tonight, but since you've commented, MMD. You probably know this dialog way better than I do, so you can see what still needs to be done.

>  keep it simple for the bustage fix

Hahaha… things got a little… out of hand. I had a fight with XBL. :-)
Attachment #8993908 - Flags: feedback?(makemyday)
Blocks: 1475817
Comment on attachment 8993908 [details] [diff] [review]

Review of attachment 8993908 [details] [diff] [review]:

Thanks, this looks good already and restores most of the previous behaviour (including some predating shortcomings like the not exact match of grid header and boxes, but we leave them beside for this bug). 
Bug 1447830 landed already, so you need to adapt your patch a little to still apply.

Remaining issue:

Freebusy lookup results are not displayed (you can only observe this if you have a server at hand which support fb requests - result is always displayed as unknown). This is due to the removal of XBL-inherit="range" in the freebusy-row definition. You can e.g. replace |this.mRange = Number(this.getAttribute("range"));| by |this.mRange = document.querySelector("#freebusy-grid").getAttribute("range");| in the constructor of the freebusy-row binding to resolve this.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +455,5 @@
> +    height: var(--spacer-top-height);
> +}
> +
> +.attendee-spacer-bottom {
> +    height: var(--spacer-bottom-height);

Can you add a comment that these vars are defined at run-time?
Attachment #8993908 - Flags: feedback?(makemyday) → feedback+
Attached patch 1477478-attendees-2.diff (obsolete) — Splinter Review
Thanks for that, MakeMyDay. I'd agree that this dialog needs an overhaul, hopefully these changes have made it a bit more resilient in the meantime.
Attachment #8993908 - Attachment is obsolete: true
Attachment #8994087 - Flags: review?(philipp)
Attached patch 1477478-attendees-3.diff (obsolete) — Splinter Review
Update because m-c removed another richlistbox method we used here.
Attachment #8994087 - Attachment is obsolete: true
Attachment #8994087 - Flags: review?(philipp)
Attachment #8995397 - Flags: review?(philipp)
Comment on attachment 8995397 [details] [diff] [review]

Review of attachment 8995397 [details] [diff] [review]:

Just a few suggestions, I'll also accept if you don't take care of them:

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xml
@@ +750,5 @@
>        <method name="getRoleElement">
>          <parameter name="aRow"/>
>          <body><![CDATA[
> +            return document.getElementById("attendeeCol1#" + aRow);

I never liked that we are using ids here. Any chance we can use querySelector relative to the binding? If its not a quick fix we can do it another day (tm)

::: calendar/base/content/dialogs/calendar-event-dialog-freebusy.xml
@@ +1318,2 @@
>              for (let row = aRow + 1; row <= max; row++) {
> +                let colID = "attendeeCol" + col + "#" + row;

I know this was the same as before, but you could make it all snazzy with |let colID = `attendeeCol${col}#${row}`;|
Attachment #8995397 - Flags: review?(philipp) → review+
Attached patch 1477478-attendees-4.diff (obsolete) — Splinter Review
With pleasure!
Attachment #8995397 - Attachment is obsolete: true
Attachment #8997225 - Flags: review?(philipp)
Oops, left some unused vars behind.
Attachment #8997225 - Attachment is obsolete: true
Attachment #8997225 - Flags: review?(philipp)
Attachment #8997226 - Flags: review?(philipp)
Comment on attachment 8997226 [details] [diff] [review]

Review of attachment 8997226 [details] [diff] [review]:

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +454,5 @@
>      padding-inline-start: 8px;
>      padding-inline-end: 10px;
>  }
> +/* --spacer-top-height and --spacer-bottom-height are defined at runtime */

I'm wondering if we should define a default value for in case it wasn't defined yet, I think var() has a second parameter for that?
Attachment #8997226 - Flags: review?(philipp) → review+
I think it's okay. If everything works as intended it's called at the window load event, and if not, we have bigger problems than the spacer being the wrong size.
This should be the last of the listboxes, unless I've forgotten something. R.I.P. listbox.
Keywords: checkin-needed
Pushed by
Remove listboxes from event attendees dialog. r=philipp
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.5
Depends on: 1482817
Depends on: 1482818
You need to log in before you can comment on or make changes to this bug.