558072 - Allow means of retrieving likely position and size where popup will appear in popupshowing event

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Attachment: patch

An new event, popupplaced should be fired after a popup has been positioned and sized on screen, but before it is made visible. This allows tweaking of the appearance of the panel before it is shown.

The attached patch does this by delaying making the view/widget visible and firing the popupshown event until after the popupplaced event.

Comment 1

[Markus Stange [:mstange]]

Isn't the real problem that correct coordinates aren't available during the popupshowing event? If we could fix this in a way where flushing layout during the popupshowing event would result in AdjustView being called but the view not being shown yet, would that be better?
Also, isn't sending events during layout dangerous?

Comment 2

[Neil Deakin]

(In reply to comment #1)
> Isn't the real problem that correct coordinates aren't available during the
> popupshowing event? If we could fix this in a way where flushing layout during
> the popupshowing event would result in AdjustView being called but the view not
> being shown yet, would that be better?

I recall trying that but it didn't work. I'll investigate some more.


> Also, isn't sending events during layout dangerous?

The patch doesn't do that. It fires when popupshown was fired.

Comment 3

[Markus Stange [:mstange]]

(In reply to comment #2)
> (In reply to comment #1)
> > Isn't the real problem that correct coordinates aren't available during the
> > popupshowing event? If we could fix this in a way where flushing layout during
> > the popupshowing event would result in AdjustView being called but the view not
> > being shown yet, would that be better?
> 
> I recall trying that but it didn't work. I'll investigate some more.

I wonder if it's because we're not calling FrameNeedsReflow early enough.

> > Also, isn't sending events during layout dangerous?
> 
> The patch doesn't do that. It fires when popupshown was fired.

Oops, sorry, misread.

Comment 4

[Neil Deakin]

I tested this more and this seems to work ok if nsPopupSetFrame::DoLayout is changed to position when the popup is in the 'showing' state as well as one of the 'open' states, and a type attribute is set on the panel. This wouldn't work for general menus but that may not matter too much.

However, it also needs a call to FrameNeedsReflow to happen at some point as well, as indicated above. I don't really want to force an extra reflow to occur though unless actually desired, but an attribute could be used to control this.

Alternatively, a rectangle property on the popupshowing event or the popup which reflows and returns the expected location.

Comment 5

[Markus Stange [:mstange]]

(In reply to comment #4)
> I tested this more and this seems to work ok if nsPopupSetFrame::DoLayout is
> changed to position when the popup is in the 'showing' state as well as one of
> the 'open' states, and a type attribute is set on the panel.

How does the type attribute come into play here? I don't understand.

> However, it also needs a call to FrameNeedsReflow to happen at some point as
> well, as indicated above. I don't really want to force an extra reflow to occur
> though

If we add this reflow we can get rid of the other one, no?

Comment 6

[Neil Deakin]

(In reply to comment #5)
> How does the type attribute come into play here? I don't understand.

The frames for the children of popups are only generated once the popup is first shown. Setting the type attribute has the side effect of disabling this optimization.

> If we add this reflow we can get rid of the other one, no?

Possibly. Something has to trigger the view being placed and shown though. For many popups, the popup needs to reflow after the popupshowing event anyway to account for the content that has been added to it.

I plan on experimenting more, but this may end up being the flaw with this implementation, as one of the main planned uses of this feature is to better support the arrow popups that append their content during the popupshowing event yet need to know where the final position is going to be in order to update the arrow location.

Comment 7

[Markus Stange [:mstange]]

(In reply to comment #6)
> one of the main planned uses of this feature is to better
> support the arrow popups that append their content during the popupshowing
> event yet need to know where the final position is going to be in order to
> update the arrow location.

The use case for which I was looking at this code originally is very similar - I wanted to make menulists anchor their popups at the selected item during the popupshowing event, and for that I needed the popup's contents to be reflown at the right position (or at least being marked as needing reflow so that they'll reflow when I ask for layout information).

Comment 8

[Neil Deakin]

I see several possible ways to do this:

- use the separate popupplaced event as implemented in this patch
- provide the option to reflow and position the popup before the popupshowing event. This is simpler to use but has the disadvantage that if one wants to add some content to the popup, the coordinates aren't useful as they would change
- provide a method on popups called guessPosition() which reflows and positions the popup (although it isn't made visible) and returns the popup rectangle. This could be called multiple times, but there will be cases where the layout needs to be done asynchronously which won't work.

Comment 9

[Neil Deakin]

Although if one appends content during popupshowing, it should cause another reflow and the flush that occurs before getting coordinates should get updated values.

I think I'll try option 2. A placed="true" attribute would indicate that the popup will generate its lazy children and mark for reflow before the popupshowing event. Popups without this will work the same as currently.

Comment 10

[Markus Stange [:mstange]]

Sounds good.
So the purpose of the placed="true" attribute is to avoid a double reflow for those popups that don't need it? And the reason for the unconditional second reflow is that we need some way to trigger showing the popup?

Comment 11

[Neil Deakin]

That's correct. But I could use some other way to trigger showing the popup, such as just a callback. For instance, if the popup wasn't dirty, just use a simple callback, or just show the popup synchronously.

Comment 12

[Markus Stange [:mstange]]

I think that would be perfect. I don't really like adding an attribute for this.

Comment 13

[Neil Deakin]

Attachment: another approach

This patch doesn't add any attributes or different handling for certain popups. Instead, it moves the lazy generation to before the popupshowing event.

However, it has the issue that some callers (such as the tooltip opening code) rely on getting to the point where it is known that the popup will open synchronously, but with this patch, this can be asynchronous. This prevents tooltips from working for example.

Comment 14

[Neil Deakin]

Attachment: updated patch

Comment 15

[Neil Deakin]

This latest patch just makes the lazy generation synchronous so it fixes the issues with tooltips.

Comment 16

[Neil Deakin]

Attachment: patch, plus fix a test

The contextmenu test fails sometimes as the popup is now opened more synchronously and the page hasn't finished loading yet (images or child frames I assume?)

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 443929 [details] [diff] [review]
patch, plus fix a test

This works great!

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 443929 [details] [diff] [review]
patch, plus fix a test

>diff --git a/layout/xul/base/src/nsXULPopupManager.cpp b/layout/xul/base/src/nsXULPopupManager.cpp

>@@ -1001,66 +1004,57 @@ nsXULPopupManager::FirePopupShowingEvent

>-  // it is common to append content to the menu during the popupshowing event.
>-  // Flush the notifications so that the frames are up to date before showing
>-  // the popup, otherwise the new frames will reflow after the popup appears,
>-  // causing the popup to flicker. Frame code always calls this asynchronously,
>-  // so this should be safe.
>-  nsIDocument *document = aPopup->GetCurrentDoc();
>-  if (document)
>-    document->FlushPendingNotifications(Flush_Layout);
>-

I don't think you should remove this. Consider a button type="menu" with a menupopup which has hidden="true". If there hasn't been a flush since hidden="true" has been set, the nsMenuPopupFrame for the popup will still exist, so this function will proceed to call nsMenuPopupFrame::ShowPopup() which will call nsMenuFrame::PopupOpened() which will set open="true" on the button. However, since the popup will never actually open, it won't close either, so the open attribute will never be unset.

SetClickAndHoldHandlers() in browser.js hits this case.  If you click-and-hold on the back button to open the menu, close the menu and then click on the back button normally, sometimes you'll get stuck in open="true" mode with this patch.

SetClickAndHoldHandlers() should probably do something more sensible like canceling the popupshowing event, instead of setting hidden = true in the mousedown handler...

Comment 19

[Neil Deakin]

(In reply to comment #18)

> SetClickAndHoldHandlers() should probably do something more sensible like
> canceling the popupshowing event, instead of setting hidden = true in the
> mousedown handler...

Please file a bug on fixing that. Setting hidden on a popup isn't the proper way to prevent it from opening.

Comment 20

[Neil Deakin]

Note that we perhaps should cleanup the parent menu (remove the open attribute, menuactive, etc) when the popup frame is destroyed anyway.

Comment 21

[Neil Deakin]

Markus, now that bug 572823 is fixed, do you think this would still be a problem?

Comment 22

[Markus Stange [:mstange]]

Probably not.

Comment 23

[Neil Deakin]

Attachment: move the lazy generation to before the popupshowing event, and allow coordinates to be retrieved during the popupshowing event

Calling getBoundingClientRect during a popupshowing event will flush and end up laying out and positioning (using SetPopupPosition) to get the likely coordinates. If someone appends content afterwards, the layout will just need to occur again.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsWeakFrame weakFrame(aFrame);

Instead of using nsWeakFrame here, I think we should still be passing in the content (to which we can hold strong refs) to FirePopupShowingEvent and just re-get the primary frame as necessary. That way, the code can still work if there are reframes, although of course if the content ends up with no frame at all, we will still need to bail out. (And we should have a test for script that makes the popup display:none during popupshowing.)

Comment 25

[Neil Deakin]

Attachment: update and add test which hides popup during popupshowing

One question that came up in another bug is that the menu/popup code uses patterns like the following to convert an nsIFrame into a specific frame type:

if (frame && frame->GetType() == nsGkAtoms::menuPopupFrame) {
   nsMenuPopupFrame* popupFrame = static_cast<nsMenuPopupFrame *>(frame);
   ...
} 

However, now do_QueryFrame has become available. Is do_QueryFrame preferred or faster?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 460405 [details] [diff] [review]
update and add test which hides popup during popupshowing

do_QueryFrame is preferred. It's not faster (or slower), but it's safer and shorter. So yeah, it would be nice to fix those.

Comment 27

[Neil Deakin]

I filed bug 582719 on the do_QueryFrame change.

Comment 28

[Neil Deakin]

Attachment: updated patch

Found a minor layout issue with the geolocation popup with the previous patch. This one just changes the flag passed to FlushPendingNotifications from Flush_Frames to Flush_Layout from within nsXULPopupManager::GetFrameOfTypeForContent.

This should prevent any problems since layout will always be up-to-date before a popup is shown.

Comment 29

[Neil Deakin]

Attachment: updated patch

This patch updates for 383930 and fixes a test that was affected by the recent unit and scaling changes.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/e560e0fe90d1

Comment 31

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336