Last Comment Bug 572682 - Move popups, panels, and tooltips into popupsets (e.g. mainPopupset)
: Move popups, panels, and tooltips into popupsets (e.g. mainPopupset)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a3
Assigned To: Philip Chee
:
Mentors:
Depends on: 578775
Blocks: 578440 578876
  Show dependency treegraph
 
Reported: 2010-06-17 07:26 PDT by Philip Chee
Modified: 2010-08-11 15:16 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 (17.31 KB, patch)
2010-06-17 07:32 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.1 inc menupopups. (34.24 KB, patch)
2010-06-28 09:00 PDT, Philip Chee
neil: review+
iann_bugzilla: feedback+
stefanh: feedback+
Details | Diff | Splinter Review
Patch v1.1a fix bit rot plus utilityOverlay.xul f=IanN f=stefanh r=Neil (37.70 KB, patch)
2010-07-14 09:11 PDT, Philip Chee
philip.chee: review+
philip.chee: feedback+
Details | Diff | Splinter Review

Description Philip Chee 2010-06-17 07:26:14 PDT
Firefox Bug 535974 moved remaining stray tooltips and panels into "mainPopupSet". We should do the same.
Comment 1 Philip Chee 2010-06-17 07:32:13 PDT
Created attachment 451947 [details] [diff] [review]
Patch v1.0

First cut.
Comment 2 neil@parkwaycc.co.uk 2010-06-17 08:00:33 PDT
Comment on attachment 451947 [details] [diff] [review]
Patch v1.0

>+  <popupset id="bookmarksPopupset">
>+    <tooltip  id="btTooltip"/>
>+    <template id="bookmarksMenuTemplate"/>
Templates aren't popups. (Also not sure why btTooltip needs its own popupset.)

>+  </popupset>
> 
> <popup id="msgComposeContext" onpopupshowing="openEditorContextMenu(this);">
Did you deliberately avoid this one?
Comment 3 Robert Kaiser 2010-06-17 09:10:07 PDT
How much does this clash with my places bookmarks work in bug 498596 which changes some popups on its own?
Comment 4 Philip Chee 2010-06-17 10:14:31 PDT
> How much does this clash with my places bookmarks work in bug 498596 which
> changes some popups on its own?

Ah I see you move the bookmarks popups into mainPopupSet. I could leave the bookmarks popups alone but you'll still need to rebase or merge.
Comment 5 Ian Neal 2010-06-17 11:38:33 PDT
Shouldn't we be looking to change popup to menupopup? See bug 571567
Comment 6 Philip Chee 2010-06-28 09:00:21 PDT
Created attachment 454534 [details] [diff] [review]
Patch v1.1 inc menupopups.

> neil@parkwaycc.co.uk      2010-06-17 08:00:33 PDT
> 
> (From update of attachment 451947 [details] [diff] [review])
>>+  <popupset id="bookmarksPopupset">
>>+    <tooltip  id="btTooltip"/>
>>+    <template id="bookmarksMenuTemplate"/>
>Templates aren't popups. (Also not sure why btTooltip needs its own popupset.)
Fixed. and Fixed.

>>+  </popupset>
>> 
>> <popup id="msgComposeContext" onpopupshowing="openEditorContextMenu(this);">
> Did you deliberately avoid this one?
Fixed.

> Robert Kaiser 2010-06-17 09:10:07 PDT
> 
> How much does this clash with my places bookmarks work in bug 498596 which
> changes some popups on its own?

Sorry but it seems no matter what I try I think you still need to rebase.

> Ian Neal      2010-06-17 11:38:33 PDT
> 
> Shouldn't we be looking to change popup to menupopup? See bug 571567
I've converted popup->menupopup for those popups I've found and moved inside popupsets. I may not have caught all occurrences but that can be done in a following patch.
Comment 7 neil@parkwaycc.co.uk 2010-07-02 05:18:38 PDT
(In reply to comment #5)
> Shouldn't we be looking to change popup to menupopup? See bug 571567
stefanh says that Mac checkbox menuitems only work in menupopups, not popups.
Comment 8 Ian Neal 2010-07-12 15:18:24 PDT
Comment on attachment 454534 [details] [diff] [review]
Patch v1.1 inc menupopups.

Not spotted any issues using this patch, may be worth checking with stefanh though.
Comment 9 Stefan [:stefanh] 2010-07-13 11:54:30 PDT
Comment on attachment 454534 [details] [diff] [review]
Patch v1.1 inc menupopups.

Seems slightly bitrottened:
Hunk #3 FAILED at 928.
1 out of 3 hunks FAILED -- saving rejects to file suite/mailnews/mailWindowOverlay.xul.rej
Comment 10 Stefan [:stefanh] 2010-07-13 12:27:20 PDT
Comment on attachment 454534 [details] [diff] [review]
Patch v1.1 inc menupopups.

But the patch seems to work fine.
Comment 11 Stefan [:stefanh] 2010-07-13 13:06:23 PDT
(In reply to comment #6)
> I've converted popup->menupopup for those popups I've found and moved inside
> popupsets. I may not have caught all occurrences but that can be done in a
> following patch.

You missed like 10-12 <popup>'s
Comment 12 Philip Chee 2010-07-14 09:11:41 PDT
Created attachment 457312 [details] [diff] [review]
Patch v1.1a fix bit rot plus utilityOverlay.xul f=IanN f=stefanh r=Neil

Carrying forward r=Neil
Fix bit-rot plus don't know why utilityOverlay.xul wasn't in the previous patch as it was in my local tree.
Comment 13 Philip Chee 2010-07-14 10:33:59 PDT
Comment on attachment 457312 [details] [diff] [review]
Patch v1.1a fix bit rot plus utilityOverlay.xul f=IanN f=stefanh r=Neil

> Fix bit-rot plus don't know why utilityOverlay.xul wasn't in the previous patch
> as it was in my local tree.
rs=Neil over IRC
Comment 14 Justin Wood (:Callek) 2010-07-14 19:09:11 PDT
(In reply to comment #13)
> Comment on attachment 457312 [details] [diff] [review]
> Patch v1.1a fix bit rot plus utilityOverlay.xul f=IanN f=stefanh r=Neil
> 
> > Fix bit-rot plus don't know why utilityOverlay.xul wasn't in the previous patch
> > as it was in my local tree.
> rs=Neil over IRC

a+=Callek if you want to land before we reopen
Comment 15 Philip Chee 2010-07-15 04:03:12 PDT
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/04473c202657
Comment 16 Jens Hatlak (:InvisibleSmiley) 2010-08-11 15:16:35 PDT
Could this have caused bug 586408? (I just want to direct some knowledgeable people over there to find a fix, not reopen this one.)

Note You need to log in before you can comment on or make changes to this bug.