1737569 - multiday selection not cleared when switching between weeks, and selection appears locked

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Henry Wilkes (they/them) [:henry-x]]

Steps to Reproduce

1. Open calendar week view.
2. Create an event (not all day).
3. Select the event with a mouse click.
4. Move one week forward using the arrows.
5. Move one week back.

Result

The event is still selected. Moreover, it cannot be visually unselected.

Expect

The event doesn't need to still be selected, and it should be possible to unselect it.

Origin

When the view is refreshed, the calendar-event-column mEventInfo is emptied https://searchfox.org/comm-central/rev/a2472c026fcf9bc25733330e9fa1e26c0429be3c/calendar/base/content/calendar-multiday-view.js#3402 but the other metadata is not. Specifically mSelectedItemIds is not emptied, which means that it accumulates event ids, and reselects them when they are displayed again https://searchfox.org/comm-central/rev/a2472c026fcf9bc25733330e9fa1e26c0429be3c/calendar/base/content/calendar-multiday-view.js#646

Comment 1

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1737569 - Drop startMinute and endMinute properties. r=darktrojan

These are unused.

Comment 2

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1737569 - Move calendar-event-column event metadata into eventDataMap. r=darktrojan

We keep all the event metadata in one object, which is accessed through the event's hashId. This ensures each event has one set of data, which is easier to track.

Also, stopped using properties for some variables that are only used in one method.

Depends on D129491

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4f752113a2ed
Drop startMinute and endMinute properties. r=darktrojan
https://hg.mozilla.org/comm-central/rev/b1cd2e09b0e3
Move calendar-event-column event metadata into eventDataMap. r=darktrojan

Comment 4

[Geoff Lankow (:darktrojan)]

You broke a test, so something isn't quite right.

Sample test log: https://treeherder.mozilla.org/logviewer?job_id=356232871&repo=comm-central&lineNumber=2647
There's two failures in the log, only the first one is because of this bug.

Comment 5

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1737569 - Allow selecting an element in calendar-event-column before it has been created. r=darktrojan

There is a small window between an event item being added to the column and it's corresponding element being created. In this window, it is technically possible that selectEvent is called, so we no longer assume the element is created in this method.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c091d2828691
Allow selecting an element in calendar-event-column before it has been created. r=darktrojan

Comment 7

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9247725 [details]
Bug 1737569 - Drop startMinute and endMinute properties. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Calendar event selection can appear locked, and then does not correspond to the actual underlying selection status.
Testing completed (on c-c, etc.): No known tests.
Risk to taking this patch (and alternatives if risky): Medium risk. The patches introduce some refactoring.
NOTE: land with the other patches.

Comment 8

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9247726 [details]
Bug 1737569 - Move calendar-event-column event metadata into eventDataMap. r=darktrojan

[Approval Request Comment]
See comment 7

Comment 9

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9248223 [details]
Bug 1737569 - Allow selecting an element in calendar-event-column before it has been created. r=darktrojan

[Approval Request Comment]
See comment 7

Comment 10

[Henry Wilkes (they/them) [:henry-x]]

Sorry, just realised 94 was already marked as wontfix, and these are already in 95, so I changed the request to esr91.

Comment 11

[Wayne Mery (:wsmwk)]

Comment on attachment 9247725 [details]
Bug 1737569 - Drop startMinute and endMinute properties. r=darktrojan

[Triage Comment]
Approved for esr91

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9247726 [details]
Bug 1737569 - Move calendar-event-column event metadata into eventDataMap. r=darktrojan

[Triage Comment]
Approved for esr91

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9248223 [details]
Bug 1737569 - Allow selecting an element in calendar-event-column before it has been created. r=darktrojan

[Triage Comment]
Approved for esr91

Comment 14

[Rob Lemley [:rjl]]

Comment on attachment 9248223 [details]
Bug 1737569 - Allow selecting an element in calendar-event-column before it has been created. r=darktrojan

The patches in this bug will need to be reworked for comm-esr91 due to changes in bug 1731209 where calendar-multiday-view.js was merged with calendar-event-column.js.
Please provide a patch and re-request uplift. Thanks.

Comment 15

[Henry Wilkes (they/them) [:henry-x]]

Attachment: bug1737569-esr91.patch

Here's the patches merged together and 'rebased' onto 91. It just required applying the patches to 91, and then applying the rejected hunks to calendar-event-column.js.

Before I request the esr91 approval, given that I didn't have to make any manual changes, does this need a separate review?

Comment 16

[Rob Lemley [:rjl]]

No, a second review after a rebase isn't usually necessary go ahead and request uplift.

Comment 17

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9251876 [details] [diff] [review]
bug1737569-esr91.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Calendar event selection can appear locked, and then does not correspond to the actual underlying selection status.
Testing completed (on c-c, etc.): No known tests.
Risk to taking this patch (and alternatives if risky): Medium risk. The patch introduce some refactoring.

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9251876 [details] [diff] [review]
bug1737569-esr91.patch

[Triage Comment]
Approved for esr91

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 91.4.0:
https://hg.mozilla.org/releases/comm-esr91/rev/f9bae337b66f