Closed
Bug 416594
Opened 16 years ago
Closed 16 years ago
[Task Mode] Priority and progress buttons are broken after customizing toolbar
Categories
(Calendar :: Tasks, defect)
Calendar
Tasks
Tracking
(Not tracked)
VERIFIED
FIXED
0.9
People
(Reporter: omar.bajraszewski, Assigned: berend.cornelius09)
References
Details
Attachments
(2 files, 1 obsolete file)
25.54 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
2.22 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•16 years ago
|
||
I can confirm this, but it is IMO not serious enough to block the release.
Flags: blocking-calendar0.8? → blocking-calendar0.8-
Updated•16 years ago
|
Flags: blocking-calendar0.8- → wanted-calendar0.9?
Comment 2•16 years ago
|
||
I can no longer reproduce this. Marking WFM. Omar, please verify. Thanks.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: wanted-calendar0.9?
Resolution: --- → WORKSFORME
Comment 3•16 years ago
|
||
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•16 years ago
|
||
Argh, I totally misunderstood this bug. Sorry.
Status: RESOLVED → REOPENED
Flags: wanted-calendar0.9+
Resolution: WORKSFORME → ---
Assignee | ||
Comment 6•16 years ago
|
||
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•16 years ago
|
||
I would favour approach 1)
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → Berend.Cornelius
Status: REOPENED → NEW
Assignee | ||
Comment 8•16 years ago
|
||
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?
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment 9•16 years ago
|
||
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?
Assignee | ||
Comment 10•16 years ago
|
||
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.
Assignee | ||
Comment 11•16 years ago
|
||
in reply to comment #9 The mode-buttons are in messenger-overlay-sidebar.xul. I am neutral whether to leave them there or not.
Assignee | ||
Comment 12•16 years ago
|
||
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)
Updated•16 years ago
|
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → 0.9
Assignee | ||
Comment 13•16 years ago
|
||
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 14•16 years ago
|
||
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+
Assignee | ||
Comment 15•16 years ago
|
||
>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: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 16•16 years ago
|
||
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?
Assignee | ||
Comment 17•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 18•16 years ago
|
||
Comment on attachment 326265 [details] [diff] [review] patch v. #3 r=philipp
Attachment #326265 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 19•16 years ago
|
||
patch v. #3 checked in on trunk and MOZILLA_1_8_BRANCH ->FIXED
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 20•16 years ago
|
||
Checked in lightning build 2008062403 -> VERIFIED
Status: RESOLVED → VERIFIED
Comment 21•16 years ago
|
||
This checkin regressed Bug 443117.
You need to log in
before you can comment on or make changes to this bug.
Description
•