897060 - Dropdown menus in web content don't work in e10s builds

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We crash because the menu creates a new window that doesn't have a proper connection to the compositor.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Milan, can someone take a look at this and see if there's an easy fix?  We're trying to get e10s dogfoodable in a few weeks time.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Bill - all platforms?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Bill - all platforms?

Ideally, yes. As far as I can tell there shouldn't be much platform-specific stuff going on here though. I talked to Matt Woodrow about this a week ago, but I don't think I'll be able to do it myself. The basic problem is that we're calling nsView::CreateWidgetForPopup in the content process. It needs to ask the main process to open a popup window, and I'm guessing it needs to create its own PuppetWidget that is hooked up to the popup. And we need to make sure that the ClientLayerManager for the puppet widget can publish layers that will be drawn by the popup.

Comment 4

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

Timothy, you could probably fix this? Unassign yourself if not :-)

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Somebody in the #e10s channel can explain how to configure a browser to test this. Basically you just need to turn on OMTC and enable the browser.tabs.remote pref.

Comment 6

[Matt Woodrow (:mattwoodrow)]

CC'ing dzbarsky who has been doing work recently on the IPDL protocols between parent/child, and might be able to help fill in some of the gaps in my knowledge.

I'll dump how I thought this could work in case it helps someone get started.

When we try open a popup in the child process, we send a notification (though PBrowser? PContent?) to the parent process to setup some things there.

In the child process:

We already have a singleton CompositorChild instance that provides us with a link to the compositor thread of the main process. Each tab currently being handled by this process (there can be multiple if you have multiple windows) is using this to send layer trees to the parent process which get stored in the sIndirectLayerTrees array.

We create a PuppetWidget, a ClientLayerManager (forwarding through the existing sCompositorChild), and get allocated an id number (which indexes into the sIndirectLayerTrees array).

Then we proceed as normal, building layers for the popup and forwarding them to the compositor where they will sit in sIndirectLayerTrees (this part should 'just work').

In the parent process:

Receive the message from the child (which needs to include the size and id number for the popup content).

Allocate a real native popup widget for this, a ClientLayerManager (along with a new CompositorChild connection - these are per-widget in the parent process, not a singleton).

Inside the layer manager, we just need a single layer, a RefLayer that references the id number for the content.

And then things should work. Maybe.

Comment 7

[David Zbarsky (:dzbarsky)]

You would want the notification to live on PContent so that it works with nested processes.  I guess the main process needs to know the size to allocate the widget?  If that's the case, this plan sounds reasonable.

(CompositorChild is in both the child process and the parent process?  That probably explains why CompositorParent never made sense to me...)

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> Timothy, you could probably fix this? Unassign yourself if not :-)

Followed up with Timothy, need to finish up another bug before he could be available for this, so it's better to set expectations here and leave the bug unassigned.

Comment 9

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

When we don't trust the content process we won't want it to be opening popups and controlling their size and position. So maybe the best thing to do here would be to delegate opening the popup to the parent process at a higher level. Or at least we'll need to sanitize the popup's geometry.

Bill, when you talk about "dropdown menus" you're talking about combobox dropdowns right? (i.e. <select size=1> in HTML?)

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> When we don't trust the content process we won't want it to be opening
> popups and controlling their size and position. So maybe the best thing to
> do here would be to delegate opening the popup to the parent process at a
> higher level. Or at least we'll need to sanitize the popup's geometry.

Yeah, and we'd probably want to rate-limit them too. I'm not sure what you mean by delegating to a higher level.

> Bill, when you talk about "dropdown menus" you're talking about combobox
> dropdowns right? (i.e. <select size=1> in HTML?)

Yes, exactly. Everyone seems to have a different name for these: popup, dropdown, combobox, select :-). Unfortunately, bugzilla uses tons of these things, so it's hard to try out e10s builds for any length of time if they're unimplemented.

Comment 11

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

I think the way to go here is to mimic the way Fennec and B2G work and move away from the page itself rendering the dropdown. Instead, the page will send the option list to chrome code in the master process which will render the dropdown, handle events in it, and send any selection change back to the content process. I think this is ultimately going to be more secure and less complex than letting the content process create popup widgets. Also, I think a hacky version can be put together quite quickly.

If we get this working well for e10s we can also use it for non-e10s desktop builds and get rid of a bunch of somewhat ugly layout code, so that's a bonus.

The B2G code for its combobox UI is in apps/system/js/value_selector/value_selector.js. This relies on content-process chrome scripts (what's the correct name for those?) that listen for <select> elements being focused and using MessageManager to activate and populate the UI. We probably want something slightly different for desktop, e.g. we don't want to pop up the UI just because a <select> was focused.

So, my proposal is:
-- modify nsComboboxControlFrame so it never tries to show or hide a dropdown directly. Instead it dispatches show/hidedropdown events that are handled by content-process chrome scripts.
-- each event references a <select> element. Content-process chrome script extracts the option list, current selection, dropdown anchor point and other metadata (eventually including some computed style data like font styles, text colors and background colors) and hands it to the chrome process using MessageManager.
-- Master process chrome script constructs its own dropdown from the message (probably using a XUL popup) and renders it
-- When a value is selected or the popup is dismissed, send a message to the content-process chrome script to update the <select> element.

Comment 12

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> I think the way to go here is to mimic the way Fennec and B2G work and move
> away from the page itself rendering the dropdown. Instead, the page will
> send the option list to chrome code in the master process which will render
> the dropdown, handle events in it, and send any selection change back to the
> content process. I think this is ultimately going to be more secure and less
> complex than letting the content process create popup widgets. Also, I think
> a hacky version can be put together quite quickly.

Won't that break styling <option>s?

Comment 13

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

Attachment: testcase for styled <option>s

Other browsers (Chrome anyway) have minimal support for <option> styling. Compare their rendering of the combobox dropdowns to ours. As far as I can tell, they support a background color for the dropwdown itself, and text colors and background colors for the options, and that's it. On Linux anyway. OTOH we support pretty much all of CSS (although background images aren't inherited into the dropdown for some reason).

Given this, I don't think there's a real need to support sophisticated option styling (and I haven't seen it used in the wild either).

Nevertheless we could support quite a lot of option styling by pulling computed style off the option elements, adding it to the option list, and reapplying it in the master process's dropdown. We have to be careful not to do things like resource loads that might expose attack surface.

Comment 14

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

Also, although we render styled options in the dropdown, we don't render option styles in the current-option display box.

Comment 15

[(no longer active)]

If we don't care about styling option elements, we can hopefully move to use the OS widget for drop-downs some day. :)

(Note that I was not advocating for this one way or another, I just wanted to point this out.)

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In case anyone has doubts: we definitely don't care about *any* styling for a v1 of fixing this)

Comment 17

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

I have a super-hacky version of this working.

Comment 18

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

The approach in comment #11 has the downside that it requires all e10s-using Gecko apps to supply their own combobox support code. That's probably not good. However, I'll keep going with this approach as a stop-gap.

Comment 19

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

Attachment: hack patch

This works OK on most comboboxes. When there are a lot of options so that scrolling is needed, things go wrong and I'm not sure why. Needs work.

To get something landable we'll need some help from someone familiar with front-end code to whip my browser.js into shape.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Attachment: dropdown-refactor

Ok, so here's a refactor of roc's patch which moves some of the front-end code around.

The main change in the refactor is that I moved most of the code from browser to toolkit, as I think the front-end code shouldn't need to do any work for dropdowns to work.

It works similarly now to how autocomplete works: the front-end just needs to provide the popup element which will be used to display it, and it can optionally do its own styling as necessary. I think that ultimately it might even be implemented as a binding extending the autocomplete-result-popup, but we don't need to spend time with that in this bug (this implementation just puts it in the right direction)

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 790009 [details] [diff] [review]
dropdown-refactor

Roc, see my change in nsListControlFrame.cpp. I'm testing on Mac and here the code never reached the mozshowdropdown dispatch, so I' un-nested it a bit and it worked.

I also conditioned this new code for now to only run with browser.tabs.remote=true (instead of the ToolkitHasNativePopup check), in order to speed up the process here by not affecting the existing handling done by fennec/b2g.

Let me know if you think those changes are ok

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Attachment: dropdown-refactor

forgot to `hg add` a file

Comment 23

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

Thanks! Did you track down the "too many options" problem?

Comment 24

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

Comment on attachment 790015 [details] [diff] [review]
dropdown-refactor

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

Looks good other than that.

::: layout/forms/nsListControlFrame.cpp
@@ +1819,5 @@
> +      nsContentUtils::DispatchChromeEvent(mContent->OwnerDoc(), mContent,
> +                                          NS_LITERAL_STRING("mozshowdropdown"), true,
> +                                          false);
> +      return NS_OK;
> +    }

This needs to be inside the "if (mComboboxFrame)" check.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Attachment: Display select dropdowns in the parent process

Neil, this is an implementation of the basic approach described in comment 11: when a popup in the content process is meant to be displayed, there's a content script listening for an event (in which the <select> is the target), and then all the information related to this dropdown is then relayed to the parent through the message manager, which builds the menuitems and displays the popup.

I used an approach similar to the autocompletepopup attribute: the front-end provides the popup element (and styling), and the toolkit code takes care of populating and displaying it.

Comment 26

[:Felipe Gomes (needinfo for replies!)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> Thanks! Did you track down the "too many options" problem?

Yeah.. yes and no, really. I was digging into it and I think it had something to do with the nested <vbox> for option groups (at least for the testcase I was using), but I don't know exactly what was happening.

But then I switched from using a panel to a menupopup which was not affected by the prob. And we also get the nice styling (shadows etc) for free.

I also included the fix for the coordinates when subframes are involved (metro/b2g had code to deal with this so I just grabbed it)

Only thing missing is the color/highlight styling prob which manifest itself a bit different with the menupopup.. We'll need to detect from the computedStyle if it was the default color or custom-set because the default <option> color won't necessarily match the default menutext color. But, of course, it's a task for another bug

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Try builds if anyone wants to test/use it:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fea4c97da3e2/
from
https://tbpl.mozilla.org/?tree=Try&rev=fea4c97da3e2

Comment 28

[:Felipe Gomes (needinfo for replies!)]

Attachment: Display select dropdowns in the parent process

Fix two bugs found by Bill. There's now an extra check for "XRE_GetProcessType() == GeckoProcessType_Content" (in addition to check the browser.tabs.remote pref), because even with the pref set to true, there are pages that we might open as non-remote and we don't want to disable the standard dropdown there

Comment 29

[Neil Deakin]

Comment on attachment 792434 [details] [diff] [review]
Display select dropdowns in the parent process

+#SelectDropdown .option.ingroup {
+  -moz-margin-start: 2em;
+}

I don't know much about optgroup but can they be nested?

+    <!-- for select dropdowns -->
+    <menupopup id="SelectDropdown" hidden="true"/>
+

This will have different rollup behaviour than selects currently do. See nsIRollupListener. You get closer by adding rolluponmousewheel="true". But the behaviour of nsIRollupListener::ShouldConsumeOnMouseWheelEvent is different.


@@ -1020,17 +1023,18 @@
       <tabbrowser id="content" disablehistory="true"
                   flex="1" contenttooltip="aHTMLTooltip"
                   tabcontainer="tabbrowser-tabs"
                   contentcontextmenu="contentAreaContextMenu"
-                  autocompletepopup="PopupAutoComplete"/>
+                  autocompletepopup="PopupAutoComplete"
+                  dropdownpopup="SelectDropdown"/>

Might be clearer which popup this relates to if you used 'selectpopup' rather than 'dropdownpopup'.


+  uninit: function() {
+    this.global.removeMessageListener("Forms:SelectDropDownItem", this);
+    this.global.removeMessageListener("Forms:DismissedDropDown", this);
+    this.global.removeEventListener("pagehide", this);
+    this.element = null;
+    this.gobal = null;

this.global


diff --git a/toolkit/modules/SelectParentHelper.jsm b/toolkit/modules/SelectParentHelper.jsm
+  open: function(browser, popup, x, y) {
+    currentBrowser = browser;
+    this._registerListeners(popup);
+    popup.hidden = false;
+    popup.openPopup(currentBrowser, "overlap", x, y);

This is unlikely to work when the popup is near the edge of the screen. The popup will get flipped around currentBrowser
rather than on the opposite side of the <select> button/field. The <select> likely has code which determines the position and side.
I don't know if it's feasible to adapt that code, or if we need to add a method to show a popup at a rectangle.

You also would want to test when multiple monitors are present.


+  },
+
+  hide: function(popup) {
+    popup.hidePopup();
+    popup.hidden = true;

Is it worth it to set the popup hidden, destroying the frame, every time?


+    switch (event.type) {
+      case "click":
+        if (event.target.hasAttribute("selectionIndex")) {
+          currentBrowser.messageManager.sendAsyncMessage("Forms:SelectDropDownItem", {
+            selectionIndex: parseInt(event.target.getAttribute("selectionIndex"), 10)
+          });

menuitems use 'value' for the value, rather than 'selectedIndex'. You might consider also to just use menuitem.selectedIndex rather than convert between integers/strings, although that might not work as you're populating the popup before it opens.

+
+  _registerListeners: function(popup) {
+    popup.addEventListener("click", this);

menuitems are selected with the 'command' event, not the 'click' event.


+function populateChildren(element, options, selectedIndex, startIndex = 0, isGroup = false) {
...
+    item.style.backgroundColor = option.backgroundColor;
+    item.style.color = option.color;

This won't override the colours. You might be able to get away with setting -moz-appearance: none when a colour is set, otherwise you'd need to change nsNativeTheme::IsWidgetStyled.


+    if (option.children.length > 0) {
+      item.setAttribute("class", "optgroup");

The class was already set above, so maybe adjust this to not set the class twice.


I only skimmed remote-browser.xml and SelectContentHelper.jsm and didn't really review them.

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Attachment: Display select dropdowns in the parent process

(In reply to Neil Deakin from comment #29)
> +#SelectDropdown .option.ingroup {
> +  -moz-margin-start: 2em;
> +}
> 
> I don't know much about optgroup but can they be nested?

No, they don't nest

> 
> +    <!-- for select dropdowns -->
> +    <menupopup id="SelectDropdown" hidden="true"/>
> +
> 
> This will have different rollup behaviour than selects currently do. See
> nsIRollupListener. You get closer by adding rolluponmousewheel="true". But
> the behaviour of nsIRollupListener::ShouldConsumeOnMouseWheelEvent is
> different.

I added the rolluponmousewheel and will file a follow-up to tune other differences in behavior.

> -                  autocompletepopup="PopupAutoComplete"/>
> +                  autocompletepopup="PopupAutoComplete"
> +                  dropdownpopup="SelectDropdown"/>
> 
> Might be clearer which popup this relates to if you used 'selectpopup'
> rather than 'dropdownpopup'.

changed to 'selectpopup'


> 
> diff --git a/toolkit/modules/SelectParentHelper.jsm
> b/toolkit/modules/SelectParentHelper.jsm
> +  open: function(browser, popup, x, y) {
> +    currentBrowser = browser;
> +    this._registerListeners(popup);
> +    popup.hidden = false;
> +    popup.openPopup(currentBrowser, "overlap", x, y);
> 
> This is unlikely to work when the popup is near the edge of the screen. The
> popup will get flipped around currentBrowser
> rather than on the opposite side of the <select> button/field. The <select>
> likely has code which determines the position and side.
> I don't know if it's feasible to adapt that code, or if we need to add a
> method to show a popup at a rectangle.
> 
> You also would want to test when multiple monitors are present.

I tested on the screen edges and it's working quite well. I haven't tested with multiple monitors yet, but similarly I'll leave the position edge cases for a follow-up.

> 
> 
> +  },
> +
> +  hide: function(popup) {
> +    popup.hidePopup();
> +    popup.hidden = true;
> 
> Is it worth it to set the popup hidden, destroying the frame, every time?

No, I don't think so. Removed. I added it because the popup begins as hidden (following the convention from the others in browser.xul), but I think that is that way just for start-up perf, and after it's created there's no point in destroying it.

> 
> 
> +    switch (event.type) {
> +      case "click":
> +        if (event.target.hasAttribute("selectionIndex")) {
> +         
> currentBrowser.messageManager.sendAsyncMessage("Forms:SelectDropDownItem", {
> +            selectionIndex:
> parseInt(event.target.getAttribute("selectionIndex"), 10)
> +          });
> 
> menuitems use 'value' for the value, rather than 'selectedIndex'. You might
> consider also to just use menuitem.selectedIndex rather than convert between
> integers/strings, although that might not work as you're populating the
> popup before it opens.

I changed it to use 'value' and integers, worked fine. I can't use the selectedIndex (I assume you meant `menupopup`.selectedIndex?) because the optgroup heads are skipped in the index count, and they wouldn't be skipped as an item in the menu

> 

> +
> +  _registerListeners: function(popup) {
> +    popup.addEventListener("click", this);
> 
> menuitems are selected with the 'command' event, not the 'click' event.

ok

> 
> 
> +function populateChildren(element, options, selectedIndex, startIndex = 0,
> isGroup = false) {
> ...
> +    item.style.backgroundColor = option.backgroundColor;
> +    item.style.color = option.color;
> 
> This won't override the colours. You might be able to get away with setting
> -moz-appearance: none when a colour is set, otherwise you'd need to change
> nsNativeTheme::IsWidgetStyled.

The foreground color worked on my testing, but not background. Since the colors aren't even being sent right now (there's a follow-up to get all the supported styles properly from content), I removed this block in this patch.

Comment 31

[Neil Deakin]

Comment on attachment 792887 [details] [diff] [review]
Display select dropdowns in the parent process

       <method name="receiveMessage">
         <parameter name="aMessage"/>
         <body><![CDATA[
-          let json = aMessage.json;
+          let data = aMessage.data;

I'm not sure what this change is about, so I'll assume it's ok.


+XPCOMUtils.defineLazyModuleGetter(this, "Rect",
+                                  "resource://gre/modules/Geometry.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Point",
+                                  "resource://gre/modules/Geometry.jsm");

Since you use both types right away after loading SelectCpntentHelper.jsm, using a lazy module here doesn't seem necessary.


+    // step out of iframes and frames, offsetting scroll values
+    let view = aElement.ownerDocument.defaultView;
+    for (let frame = view; frame != this.global.content; frame = frame.parent) {
+      // adjust client coordinates' origin to be top left of iframe viewport
+      let rect = frame.frameElement.getBoundingClientRect();
+      let left = frame.getComputedStyle(frame.frameElement, "").borderLeftWidth;
+      let top = frame.getComputedStyle(frame.frameElement, "").borderTopWidth;
+      offset.add(rect.left + parseInt(left), rect.top + parseInt(top));
+    }
+
+    return new Rect(r.left + offset.x, r.top + offset.y, r.width, r.height);

You could also just do return r.translate(offset.x, offset.y); Also, maybe not bother with the Point type at all. But...


+  },
+
+  _getRectRelativeToOrigin: function() {
+    let rect = this._getBoundingContentRect(this.element);
+
+    return {
+      x: rect.left,
+      y: rect.top,
+      w: rect.width,
+      h: rect.height
+    }
+  },

This function doesn't seem necessary. You don't use the width, and receiveMessage wants the bottom rather than the height anyway.
_getBoundingContentRect could just instead return an object containing the properties receiveMessage wants.

+  _buildOptionList: function() {
+    return buildOptionListForChildren(this.element);
+  },

Why is this wrapper function needed?


+function buildOptionListForChildren(node) {
...
+        // color does not override color: menutext in the parent.
+        // backgroundColor: computedStyle.backgroundColor,
+        // color: computedStyle.color,
+        children: child.tagName == 'OPTGROUP' ? buildOptionListForChildren(child) : []

I asked about nested optgroups because this code handles them as if they were supported.


> I tested on the screen edges and it's working quite well. I haven't tested with multiple monitors yet, but similarly I'll leave the position edge cases for a follow-up.

OK, maybe you're going to ensure the positioning is correct in a separate bug? When I test it the popup appears over top of the <select> rather than anchored along its edge, but it also appears incorrectly even when opened in the middle of the screen as well. The popup appears offset to the right and the width is incorrect.

Comment 32

[:Felipe Gomes (needinfo for replies!)]

(In reply to Neil Deakin from comment #31)
> Comment on attachment 792887 [details] [diff] [review]
> Display select dropdowns in the parent process
> 
>        <method name="receiveMessage">
>          <parameter name="aMessage"/>
>          <body><![CDATA[
> -          let json = aMessage.json;
> +          let data = aMessage.data;
> 
> I'm not sure what this change is about, so I'll assume it's ok.

yeah, they mean the same thing, but json is deprecated so I'm updating it while I'm in this function. 

> 
> 
> +function buildOptionListForChildren(node) {
> ...
> +        // color does not override color: menutext in the parent.
> +        // backgroundColor: computedStyle.backgroundColor,
> +        // color: computedStyle.color,
> +        children: child.tagName == 'OPTGROUP' ?
> buildOptionListForChildren(child) : []
> 
> I asked about nested optgroups because this code handles them as if they
> were supported.

I'll verify but I think the parser flattens the dom structure for nested optgroups in the source.
> 
> 
> > I tested on the screen edges and it's working quite well. I haven't tested with multiple monitors yet, but similarly I'll leave the position edge cases for a follow-up.
> 
> OK, maybe you're going to ensure the positioning is correct in a separate
> bug? When I test it the popup appears over top of the <select> rather than
> anchored along its edge, but it also appears incorrectly even when opened in
> the middle of the screen as well. The popup appears offset to the right and
> the width is incorrect.

Is that by a small amount? If so this is a separate issue that billm is working on: the rendered content is shifted by the scrollbar width. You can see that the actual click bounds for the select are in the right position and the popup shows underneath it.

Comment 33

[:Felipe Gomes (needinfo for replies!)]

Attachment: Display select dropdowns in the parent process

This patch implements all the feedback. I got rid of the Rect/Point modules and just wrote the straight calculations. I also removed the computedStyle calls which are not used for now.

Only thing I didn't change is the buildOptionList wrapper, as I like the obj's method to be about the this.element, and let the outside helper function handle the recursion with a different node.

I checked the nesting for optgroups and the DOM is nested but we render it as flat (from what I found this is part of the spec, or at least the de-facto accepted behavior). So the patch does the right thing (it sends the data as nested but the css renders it as flat)

The width can't easily be passed to openPopup without access to the anchor element, so I'll handle it in a follow up (I think it might not even be necessary as the menupopup calculates a reasonable width depending on the content, but i'll open a bug)

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 794317 [details] [diff] [review]
Display select dropdowns in the parent process

>+#SelectDropdown {

I'd prefer to adding "content" to this id.

>+#SelectDropdown .optgroup {

Please use an unambiguous class name (like contentSelectDropdown-optgroup) and drop #SelectDropdown from this selector.

>+#SelectDropdown .option.ingroup {

Ditto.

Also, this is the only place where the "option" class is used. And since AIUI "ingroup" implies "option", the latter seems redundant.

Comment 35

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We're pretty desperate to get this checked in. If somebody feels qualified to review this over the weekend, it would be much appreciated!

Comment 36

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 794317 [details] [diff] [review]
Display select dropdowns in the parent process

Trying the luck with Tim

Comment 37

[Neil Deakin]

Comment on attachment 794317 [details] [diff] [review]
Display select dropdowns in the parent process

> > OK, maybe you're going to ensure the positioning is correct in a separate
> > bug? When I test it the popup appears over top of the <select> rather than
> > anchored along its edge, but it also appears incorrectly even when opened in
> > the middle of the screen as well. The popup appears offset to the right and
> > the width is incorrect.
> 
> Is that by a small amount?

The popup is only offset by a small amount horizontally. The width is completely incorrect, at least according to the current behaviour where the width is the same as the <select> itself. Your code also assumes that the popup will always appear below the <select> which is not the case. When the <select> is near the bottom of the screen, the popup should appear upwards and be anchored along the top edge of the <select>.

But the rest of the patch looks ok, if you file a followup to improve the position/size.

Comment 38

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 794317 [details] [diff] [review]
Display select dropdowns in the parent process

Thanks Neil, I'll file the follow-ups. Landed with the suggestions by Dão.

https://hg.mozilla.org/integration/fx-team/rev/56c09de8d0f6

Comment 39

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/mozilla-central/rev/56c09de8d0f6

Comment 40

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Some of these new files don't seem to have a license header. Please make sure to fix that.

Comment 41

[:Felipe Gomes (needinfo for replies!)]

Thanks for catching that. Fixed the missing headers with:
https://hg.mozilla.org/integration/fx-team/rev/cbe79a948a4d

Follow-ups discussed here were filed as bug 910022 and bug 910023.

Comment 42

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks!

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cbe79a948a4d