Closed Bug 416594 Opened 12 years ago Closed 12 years ago

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

Categories

(Calendar :: Tasks, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: omarb.public, Assigned: berend.cornelius09)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Flags: blocking-calendar0.8?
I can confirm this, but it is IMO not serious enough to block the release.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
Flags: blocking-calendar0.8- → wanted-calendar0.9?
I can no longer reproduce this. Marking WFM. Omar, please verify. Thanks.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: wanted-calendar0.9?
Resolution: --- → WORKSFORME
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.
Duplicate of this bug: 433570
Argh, I totally misunderstood this bug. Sorry.
Status: RESOLVED → REOPENED
Flags: wanted-calendar0.9+
Resolution: WORKSFORME → ---
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.
I would favour approach 1)
Assignee: nobody → Berend.Cornelius
Status: REOPENED → NEW
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?
Status: NEW → ASSIGNED
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?
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.
in reply to comment #9
The mode-buttons are in messenger-overlay-sidebar.xul. I am neutral whether to leave them there or not.
Attached patch patch v. #1 (obsolete) — Splinter Review
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).
Attachment #325505 - Flags: review?(philipp)
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → 0.9
Attached patch patch v. #2Splinter Review
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.
Attachment #325505 - Attachment is obsolete: true
Attachment #326034 - Flags: review?(philipp)
Attachment #325505 - Flags: review?(philipp)
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
Attachment #326034 - Flags: review?(philipp) → review+
>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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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?
Attached patch patch v. #3Splinter Review
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.
Attachment #326265 - Flags: review?(philipp)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 326265 [details] [diff] [review]
patch v. #3

r=philipp
Attachment #326265 - Flags: review?(philipp) → review+
patch v. #3 checked in on trunk and MOZILLA_1_8_BRANCH

->FIXED
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Checked in lightning build 2008062403 -> VERIFIED
Status: RESOLVED → VERIFIED
Depends on: 443117
This checkin regressed Bug 443117.
You need to log in before you can comment on or make changes to this bug.