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

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

Firefox Bug 535974 moved remaining stray tooltips and panels into "mainPopupSet". We should do the same.

Comment 1

[Philip Chee]

Attachment: Patch v1.0

First cut.

Comment 2

[[e-mail]]

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]

How much does this clash with my places bookmarks work in bug 498596 which changes some popups on its own?

Comment 4

[Philip Chee]

> 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]

Shouldn't we be looking to change popup to menupopup? See bug 571567

Comment 6

[Philip Chee]

Attachment: Patch v1.1 inc menupopups.

> [e-mail]      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

[[e-mail]]

(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]

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]]

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]]

Comment on attachment 454534 [details] [diff] [review]
Patch v1.1 inc menupopups.

But the patch seems to work fine.

Comment 11

[Stefan [:stefanh]]

(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]

Attachment: 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]

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)]

(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]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/04473c202657

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Could this have caused bug 586408? (I just want to direct some knowledgeable people over there to find a fix, not reopen this one.)