416594 - [Task Mode] Priority and progress buttons are broken after customizing toolbar

Status: VERIFIED FIXED

(Calendar :: Tasks, defect)

Description

[Omar Bajraszewski]

When you customize a toolbar the buttons don't work

Steps to reproduce:
1)Create new profile and switch to task mode
2)Add a task
3)Open custom toolbar dialog and select cancel
4)Select drop down menu for priority or progress button

Result:
The drop down menu doesn't work

Lt 2008020902/tb 2.0.0.9

Comment 1

[Simon Paquet [:sipaq]]

I can confirm this, but it is IMO not serious enough to block the release.

Comment 2

[Simon Paquet [:sipaq]]

I can no longer reproduce this. Marking WFM. Omar, please verify. Thanks.

Comment 3

[Stefan Sitter [:ssitter]]

I can still reproduce the issue using Thunderbird 2.0.0.15pre (20080513) with Lightning 0.9pre (2008051219) on Windows XP for both priority and progress button.

Comment 5

[Simon Paquet [:sipaq]]

Argh, I totally misunderstood this bug. Sorry.

Comment 6

[Berend Cornelius]

The menupopup are obviously getting lost after the customization. This is not astonishing since they are added during runtime (see "ltnInitializeMenus()" in messenger-overlay-sidebar.js).
As far as I see there are two ways to fix it:
1) Add the menupopups directly to the toolbarbuttons in the xul-file "lightning-toolbar.inc and clone these menus to wherever they are also needed - instead of doing this the other way around as it is currently done.
2) readd the menupopups within the respective "customizedone" function.

Comment 7

[Simon Paquet [:sipaq]]

I would favour approach 1)

Comment 8

[Berend Cornelius]

I would also favour approach 1), because this solution is simpler. But there is one problem: the toolbar-buttons are defined in "lightning-toolbar.inc" and therefor the shared popup-menus underneath them are not available for Sunbird.
Because of this I would vote for merging the whole content of lightning-toolbar.inc to calendar-toolbar.inc -sooner or later I reckon that we will provide them in Sunbird anyway.
Any objections or comments?

Comment 9

[Simon Paquet [:sipaq]]

We can certainly move those items over to calendar-toolbar.inc that we think would benefit Sunbird as well. I would oppose to move items into calendar-toolbar.inc that have no purpose there, as that would only bloat Sunbird's calendar.xul needlessly.

BTW is there a good reason why the mode-buttons are not in lightning-toolbar.inc?

Comment 10

[Berend Cornelius]

In reply to comment #9:
It would be a bit confusing to separate the the progress-toolbarbutton and priority-toolbarbutton from the other toolbar-buttons of the task-toolbar. None of them are (so far) used in Sunbird - only their included menupopups are needed there.

Comment 11

[Berend Cornelius]

in reply to comment #9
The mode-buttons are in messenger-overlay-sidebar.xul. I am neutral whether to leave them there or not.

Comment 12

[Berend Cornelius]

Attachment: patch v. #1

This patch should fix the bug. I added the menupopup to the toolbarbuttons and copied this the the other menus. To make this working for Sunbird too I had to add a task toolbar for that application (which is not yet visible).

Comment 13

[Berend Cornelius]

Attachment: patch v. #2

I think my first patch solved the issue but anyway I was not quite content with it because I was duplicating the popup-menus instead of creating a binding. The patch attached creates two reusable bindings (one for the priority menu-popup and one for the progress menu-popup). Thank you to Philipp who gave me a necessary hint on how to attach the bindings to the toolbarbutton.

Comment 14

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 326034 [details] [diff] [review]
patch v. #2

>+menupopup[type="progress"] > arrowscrollbox,
>+toolbarbutton > menupopup[type="progress"] > arrowscrollbox {
>+  -moz-binding: url(chrome://calendar/content/calendar-menus.xml#task-progress-menupopup);
>+}
I'd prefer something more calendar specific, i.e
toolbarbutton > menupopup[type="calendar-task-progress"] > arrowscrollbox {
toolbarbutton > menupopup[type="calendar-task-priority"] > arrowscrollbox {



>+      <constructor><![CDATA[
>+        var self = this;
>+        this.mPopupHandler = function popupHandler() { self.changeMenuByPropertyName(); };
>+        window.addEventListener("popupshowing", this.mPopupHandler, true);
A popupshowing handler for the whole window? Not sure I understand this part of the code correctly, but can't you use a <handler> ?

>+<!-- This highly specialized function checks a command which naming follows
>+ *  the notation 'calendar_' +  propertyname + ' + '-' + propertvalue + 'command',
>+ *  when its propertyvalue part matches the propertyvalue of the selected tasks
>+ *  as long as the selected tasks share common propertyValues.
>+ *  @param aEvent the event that contains a target from which the child elements
>+ *  are retrieved and unchecked.
>+ *  @param aPropertyName the name of the property that is available at a task
>+ * -->
Please add some more spaces after the parameter names, and indent wrapped lines after @param

>+  <binding id="task-progress-menupopup" extends="chrome://calendar/content/calendar-menus.xml#task-menupopup">
>+    <content>
...
>+      <xul:menuitem anonid="percent-100-menuitem"
>+                type="checkbox"
>+                label="&progress.level.100;"
>+                accesskey="&progress.level.100.accesskey;"
>+                observes="calendar_percentComplete-100_command"
>+                command="calendar_percentComplete-100_command"/>
Please add a <children/> tag here, to allow extensions to add things. The same goes for the other bindings

Comment 15

[Berend Cornelius]

>Please add a <children/> tag here, to allow extensions to add things. The same
>goes for the other bindings
Good idea! We should have this always in mind, when creating bindings.

>A popupshowing handler for the whole window? Not sure I understand this part of
>the code correctly, but can't you use a <handler> ?

Of course a <handler> was my first choice but for reasons that I could not find out this handler was never triggered and that's why I came up with my solution.
If somebody can show me how to get the <handler> going with "popupshowing" I can reopen the issue and improve my implementation.

patch that considerd the other issues of comment #13 checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 16

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

I assume that the problem is that your binding is on the arrowscrollbox, which doesn't really have a popupshowing. Possibly the menupopup does a stopPropagation() or such.

Maybe it works with

<handler event="popupshowing" phase="capturing">...</handler>

Note also that if you have the popupshowing event on the window, then any menupopup that is opened will trigger mPopupHandler. I didn't look too closely, but did you make sure that the right node is targeted in the popuphandler?

Comment 17

[Berend Cornelius]

Attachment: patch v. #3

In reply to commment #16

>Maybe it works with

><handler event="popupshowing" phase="capturing">...</handler>

No I have already checked this beforehand

>Note also that if you have the popupshowing event on the window, then any
>menupopup that is opened will trigger mPopupHandler. I didn't look too closely,
>but did you make sure that the right node is targeted in the popuphandler?

No actually I did not. The patch attached should make sure that the binding listens to the right parental menupopup.

Comment 18

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 326265 [details] [diff] [review]
patch v. #3

r=philipp

Comment 19

[Berend Cornelius]

patch v. #3 checked in on trunk and MOZILLA_1_8_BRANCH

->FIXED

Comment 20

[Andreas Treumann]

Checked in lightning build 2008062403 -> VERIFIED

Comment 21

[Stefan Sitter [:ssitter]]

This checkin regressed Bug 443117.