1583481 - remove unnecessary observes= from calendar

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Magnus Melin [:mkmelin]]

I was looking at observes attribute usage in calendar, and noticed there's apparently been some misunderstanding of how that should be used: if you have a command on the element, you don't need to have observes= too. When command updating happens, that will also take care of enabling/disabling the element.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1583481_cal_observes_attr_cleanup.patch

Comment 2

[Magnus Melin [:mkmelin]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ad4bdf64c2501f57db1cad4b0a9dd4c2bd74d8c9

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9094868 [details] [diff] [review]
bug1583481_cal_observes_attr_cleanup.patch

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

Codewise this seems fine to me. One thing to consider: using `observes="foo"` when `foo` is a broadcaster would make that element an observer, which means it will watch some additional attributes, I believe including `disabled`. It doesn't seem we have any more broadcasters, but what you should make sure before pushing this is that the elements that had `observes=` aren't expecting additional attributes.

Comment 4

[Magnus Melin [:mkmelin]]

Thanks, there are only a few commands that have attributes, and looks like those work fine.

Comment 5

[Jorg K (CEST = GMT+2)]

Causes Mochitest (bct) failures locally. Did your look at your own try run?

Comment 6

[Magnus Melin [:mkmelin]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=470ef6c17377d599ef5d7638d27d8b6a78931d80

document.commandDispatcher.updateCommands("calendar_commands") didn't cause calendar/lightning/content/messenger-overlay-sidebar.xul specific (eh..!) calendar_commands commands to get updated...

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: bug1584720_search-textbox_context.patch

I think the calendar_commands not working as expected is a bug in Overlays.jsm.
Anyway, I also adjusted the context menu not to show disabled items when not on a calendar, but to hide them instead, like is usual practice for context menus. And dropped the manual logic to adjust the disabled states in the menu, since the command attribute will take care automatically now that it's working.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9097576 [details] [diff] [review]
bug1584720_search-textbox_context.patch

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

I think you meant to ask Jörg for review? This is in mail/

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Actually it may be the wrong patch, the bug numbers mismatch.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9097576 [details] [diff] [review]
bug1584720_search-textbox_context.patch

I've already reviewed this one and we landed it ;-) - Yes, wrong patch, we need the update to make the M(bct) pass, see comment #5.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: bug1583481_cal_observes_attr_cleanup.patch

The right patch, fixed and unbitrotted.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9099390 [details] [diff] [review]
bug1583481_cal_observes_attr_cleanup.patch

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

::: calendar/base/content/calendar-management.js
@@ +503,4 @@
>      setupDeleteMenuitem("list-calendars-context-delete", calendar);
> +    event.target.querySelectorAll(".needs-calendar").forEach(elem => {
> +      elem.removeAttribute("collapsed");
> +    });

Same as next comment

@@ +507,2 @@
>    } else {
> +    event.target.querySelectorAll(".needs-calendar").forEach(elem => {

Can we just use for..of here? Given we are using an anonymous function anyway I think we can skip the extra function calls.

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: bug1583481_cal_observes_attr_cleanup.patch

Made the change. Didn't realize for...of works for querySelectorAll.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e544df90a3f3
remove unnecessary observes= from calendar. r=Fallen