873923 - Make anchored popups work with e10s

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

There are a number of places that open an anchored panel where the anchor is in a content document.  This is problematic in an e10s world where that document is in another process, and the CPOW wrappers don't allow a wrapper for the anchor element to be passed into native code.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

The one part of this that I felt would be tricky is how the panel "tracks" the anchor as it scrolls - I'm sure I saw something like that happen in the past, but now I can't see it happen at all.

To test, I added the following on a page:

<form onsubmit="return false;">
    <fieldset>
    Email Address: <input id="email" type="email" required>
    </fieldset>
</form>

and enough "padding" such that the document is larger than the viewport.  Then, press "enter" while the field is empty - an anchored panel opens indicating the field is invalid.  Then use the mouse-wheel to scroll the document - the panel remains in the same place as the anchor moves underneath it.

The good news is that this makes it quite a bit simpler to fix this bug - we probably just need something like an nsIPopupAnchor interface with a few details about the anchor position etc.

Gavin and I were discussing this, so adding him to CC - Gavin or Josh - are you aware of any scenarios in which the panel does automatically track the anchor as it moves?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Hmm, I guess I misremembered what the expected behavior was for these cases. It seems like we should be hiding the form validation popup when you scroll the page under it, similar to how we hide autocomplete suggestion popups.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: Use nsIPopupAnchor

This is still a WIP, but the earlier I get feedback the less time I waste!

This patch introduces a new nsIPopupAnchor interface with some simple attributes necessary so nsMenuPopupFrame etc can all work without a reference to the anchor element  It all works, but hasn't changed much of the "public" interface into the popups - ie, openPopup etc still wants an nsIElement and convert it to an nsIPopupAnchor before passing it into the popup-frame.  It only comes with one nsIPopupAnchor implementation, which is implemented in C++ and itself holds a reference to the anchor's nsIContent - so this patch is only laying the foundation for the future e10s work and doesn't yet allow the creation of an anchored popup without a reference to an element.  See the following patch for an update on this.

It's slightly messy in a couple of areas, particularly in SetPopupPosition, where the code looks for the tag, sizedtopopup attribute and fetches AppUnitsPerDevPixel() - it might be possible to clean that up a bit, but I felt the most important thing was to ensure the semantics match exactly before attempting any optimization.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: Javascript implementation of nsIPopupAnchor

This patch is also a WIP, but far less finished.  The approach taken here might not fly - see below.

This patch is trying to expose nsIPopupAnchor directly to the popup interfaces - eg, have nsIPopupBoxObject.openPopup take an nsIPopupAnchor directly.  The popup bindings in popup.xml would still take an element, but convert it to a javascript implementation of nsIPopupAnchor before passing it on to nsIPopupBoxObject which, in theory, would not change the public-facing interfaces.  The intention was then to have popup.xml accept *either* an element or an nsIPopupAnchor, so in an e10s world we can pass the nsIPopupAnchor directly and everyone is happy.

But they aren't...

[There's some hand-waving here] The problem with this approach seems to be the security model of XBL.  When openPopup is called directly from chrome code it all works fine.  However, if the popup is opened from either a file:/// XUL URL, or by mochitest-plain (eg, the toolkit/content/tests/widgets/test_popupanchor.xul test), popup.xml can't use Components.utils to import a javascript implementation of nsIPopupAnchor (Error: Permission denied to access property 'utils') - thus, it can't convert an element to this interface.

What this might mean is that nsIPopupBoxObject will need to take a generic nsISupports as the popup anchor, then attempt to QI it for either the element interface or nsIPopupAnchor.  The e10s chrome code will then need to pass the nsIPopupAnchor into popup.xml, whereas most other code will still just pass the element.  This will be slightly messier, but I figured I'd get some feedback on the general approach before burning more time on this...

Comment 5

[[e-mail]]

Comment on attachment 759620 [details] [diff] [review]
Use nsIPopupAnchor

This seems to be over-engineered to me. I'm not sure how (when it does do, see below) the popup tracks the anchor, but that might have to be changed or abandoned for e10s, in which case it would seem to me that all you're really interested in is a way of opening the popup anchored to a screen rectangle (openPopup uses the element's rectangle, while openPopupAtScreen uses a point, not a rectangle).

>+  readonly attribute DOMString id;
I don't think you need to pass in the id. You gave an example of a popup which could be anchored to more than one element, but in that case the anchor was always in the same chrome document so no e10s concerns apply.

>+  readonly attribute DOMString tag;
I'm not sure why we're still checking the tag; I thought XBL form controls were removed ages ago.

>+  readonly attribute DOMString sizeToPopup;
>+  readonly attribute nsIDOMClientRect rect;
These are used for menulists and chrome autocomplete, which again is e10s-free.

(In reply to Mark Hammond from comment #1)
> The one part of this that I felt would be tricky is how the panel "tracks"
> the anchor as it scrolls - I'm sure I saw something like that happen in the
> past, but now I can't see it happen at all.
Which OS? On Linux I couldn't get the page to scroll when the panel was open. On Windows, the panel sometimes followed the scrolling, but sometimes it "forgot".

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to [e-mail] from comment #5)
> Comment on attachment 759620 [details] [diff] [review]
> Use nsIPopupAnchor
> 
> This seems to be over-engineered to me. I'm not sure how (when it does do,
> see below) the popup tracks the anchor, but that might have to be changed or
> abandoned for e10s, in which case it would seem to me that all you're really
> interested in is a way of opening the popup anchored to a screen rectangle
> (openPopup uses the element's rectangle, while openPopupAtScreen uses a
> point, not a rectangle).

I sympathize with this view (and 1/2 agree with it - I'm torn :) - but it does beg the question: if we decide we can live without this behaviour in an e10s world, then do we need it for anchored popups at all?  However, it's not quite as simple as a position - there is the flipping and application of other constraints that we'd need to implement too.  I'm flagging enn for moreinfo - enn, welcome back!  And can you please offer your general thoughts on panels in an e10s world?

For completeness, I'll address some of your other comments:
> 
> >+  readonly attribute DOMString id;
> I don't think you need to pass in the id. You gave an example of a popup
> which could be anchored to more than one element, but in that case the
> anchor was always in the same chrome document so no e10s concerns apply.

There is some code that gets the ID of the panel's anchor element (which was included in the patch).  However, it might be possible to refactor some of that code to make it unnecessary.

> >+  readonly attribute DOMString tag;
> I'm not sure why we're still checking the tag; I thought XBL form controls
> were removed ages ago.

There is code that checks if the tag is "select" IIRC.  As I mentioned, I wasn't trying to "fix" any panel code, just adjust it so it can work without a direct reference to the anchor in all contexts.

> >+  readonly attribute DOMString sizeToPopup;
> >+  readonly attribute nsIDOMClientRect rect;
> These are used for menulists and chrome autocomplete, which again is
> e10s-free.

Right - but the approach in this patch was to allow *all* uses of anchors to use this new interface rather than creating a parallel implementation for use in multi-process scenarios.  I understand you think this is unwise, but am just putting this patch in context.

> (In reply to Mark Hammond from comment #1)
> > The one part of this that I felt would be tricky is how the panel "tracks"
> > the anchor as it scrolls - I'm sure I saw something like that happen in the
> > past, but now I can't see it happen at all.
> Which OS? On Linux I couldn't get the page to scroll when the panel was
> open. On Windows, the panel sometimes followed the scrolling, but sometimes
> it "forgot".

On Windows.  One example I can reliably reproduce is that a HTML5 form validation panel remains in the same place even when the content window hosting the anchor is scrolled with the mouse-wheel.

Comment 7

[Neil Deakin]

What code would be calling openPopup with an nsIPopupAnchor? And what code would be creating the nsIPopupAnchor implementation?

> This is problematic in an e10s world where that document is in another process, and the CPOW wrappers don't allow a wrapper for the anchor element to be passed into native code.

Context menus and tooltips don't get opened via script, so nothing gets 'passed into native code'. Is there a specific example we can use that won't work with different processes?

I didn't look at the patch in detail though, but some things stood out:

> nsCOMPtr<nsIPopupAnchor> NS_ContentToPopupAnchor(nsIContent *aContent)
> +{
> +  NS_ASSERTION(aContent, "can't create an nsPopupAnchor without an nsIContent");

The anchor may be null; in this case the popup is anchored to the document.

+  readonly attribute nsIDOMClientRect screenRectInAppUnits;
+  readonly attribute nsIDOMClientRect rect;
+  readonly attribute long appUnitsPerDevPixel;

Don't like using appunits here. How would the implementer know these values?

-      <property name="anchorNode" readonly="true"
-                onget="return this.popupBoxObject.anchorNode"/>
+      <property name="popupAnchor" readonly="true"
+                onget="return this.popupBoxObject.popupAnchor"/>

Better would be to just leave the name as is.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to Neil Deakin from comment #7)
> What code would be calling openPopup with an nsIPopupAnchor? And what code
> would be creating the nsIPopupAnchor implementation?

The initial case I've hit is the HTML5 form validation panel:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#725

and I *think* autocomplete (and tooltips relating to HTML5 form elements?) might have the same basic requirement - a panel opened in chrome anchored to a content element.

So what would probably happen in an e10s world is that a "frame script" in the content process would have the form observer, which would send a message back to the chrome process with details about the panel to open (including the anchor).  This chrome message handler would create the nsIPopupAnchor and pass it to the popup.

> > This is problematic in an e10s world where that document is in another process, and the CPOW wrappers don't allow a wrapper for the anchor element to be passed into native code.
> 
> Context menus and tooltips don't get opened via script, so nothing gets
> 'passed into native code'. Is there a specific example we can use that won't
> work with different processes?

In the example above, the chrome process can actually get a CPOW proxy to the element in the child process, but it can't be passed into openPopup.

> I didn't look at the patch in detail though, but some things stood out:

Thanks - I'll address these issues if it turns out we do actually want to take this approach.

Comment 12

[Neil Deakin]

Attachment: Open popup at a rectangle

This patch implements an openPopupAtScreenRect method which opens a popup anchored at a given screen rectangle.

Comment 13

[George Wright (:gw280) (needinfo me!)]

Neil, does this patch need to be reviewed, or are you working on an updated one?

Comment 14

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-873923-Add-openPopupAtScreenRect.patch

I rebased the patch on latest m-c, so figured I'd upload it here in case you want to flag it for review. It works well for me, at least.

Comment 15

[Neil Deakin]

Attachment: Fix a compile error

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8605271 [details] [diff] [review]
Fix a compile error

Looks good. Although I don't think I can review webidl or toolkit changes.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8605271 [details] [diff] [review]
Fix a compile error

Review of attachment 8605271 [details] [diff] [review]:
-----------------------------------------------------------------

The popup stuff looks good to me, but it looks like some tests for the new public method would be worthwhile - if there's no time for that in this bug, can we get a new bug on file for tests?

Comment 18

[Neil Deakin]

Attachment: Updated patch

This fixes a minor issue when using noautohide popups causing them to jump around on screen when the window is moved.

Comment 19

[Neil Deakin]

Attachment: Test for openPopupAtScreenRect

Comment 20

[Neil Deakin]

Comment on attachment 8605884 [details] [diff] [review]
Updated patch

For dom/webidl change.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8605884 [details] [diff] [review]
Updated patch

Review of attachment 8605884 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I won't have time to review this in the near future.

Comment 22

[Neil Deakin]

Comment on attachment 8605884 [details] [diff] [review]
Updated patch

For dom/webidl/PopupBoxObject.webidl change

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8605884 [details] [diff] [review]
Updated patch

Review of attachment 8605884 [details] [diff] [review]:
-----------------------------------------------------------------

The DOM bits look fine.  I didn't review the actual popup manager and frame changes.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3034d392919d
https://hg.mozilla.org/integration/mozilla-inbound/rev/810c13b0ac02

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3034d392919d
https://hg.mozilla.org/mozilla-central/rev/810c13b0ac02

Comment 26

[Firefox Bug Husbandry Bot]

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