Closed Bug 572682 Opened 9 years ago Closed 9 years ago

Move popups, panels, and tooltips into popupsets (e.g. mainPopupset)

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a3

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 2 obsolete files)

Firefox Bug 535974 moved remaining stray tooltips and panels into "mainPopupSet". We should do the same.
Attached patch Patch v1.0 (obsolete) — Splinter Review
First cut.
Attachment #451947 - Flags: review?(neil)
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?
How much does this clash with my places bookmarks work in bug 498596 which changes some popups on its own?
> 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.
Shouldn't we be looking to change popup to menupopup? See bug 571567
Attached patch Patch v1.1 inc menupopups. (obsolete) — Splinter Review
> 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.
Attachment #451947 - Attachment is obsolete: true
Attachment #454534 - Flags: review?(neil)
Attachment #454534 - Flags: feedback?(iann_bugzilla)
Attachment #451947 - Flags: review?(neil)
Attachment #454534 - Flags: review?(neil) → review+
(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 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.
Attachment #454534 - Flags: feedback?(iann_bugzilla) → feedback+
Attachment #454534 - Flags: feedback?(stefanh)
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 on attachment 454534 [details] [diff] [review]
Patch v1.1 inc menupopups.

But the patch seems to work fine.
Attachment #454534 - Flags: feedback?(stefanh) → feedback+
Blocks: 578440
(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
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.
Attachment #454534 - Attachment is obsolete: true
Attachment #457312 - Flags: review+
Attachment #457312 - Flags: feedback+
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
(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
Blocks: 578876
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/04473c202657
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3
Could this have caused bug 586408? (I just want to direct some knowledgeable people over there to find a fix, not reopen this one.)
You need to log in before you can comment on or make changes to this bug.