1421229 - (content-select) [META] Render dropdown menu of <select> in content process

Status: RESOLVED WONTFIX

(Core :: Layout: Form Controls, enhancement, P1)

Description

[Tommy Kuo (away forever...)]

In our current design, we pass the all information of <select> to the parent process to show the dropdown menu. And it caused several issues in Bug 1154677.

For example, Bug 1118086 and Bug 1419800 indicated the performance issue when the dropdown menu has many options. Furthermore, there are also lots of issues related to styling, because of the current design that made Firefox lose the power to style options with more CSS properties.

In Bug 1118086 Comment 87, mats proposed that we should render the dropdown menu in the content process. That will resolve the performance issues, the styling issues. And I think we can also resolve the massive event handling because we have more controls on the dropdown menu.

So, I tend to implement the architecture of rendering the dropdown menu in the content process.

Comment 1

[Tommy Kuo (away forever...)]

Attachment: content-dropdown.mp4

This video shows that the how quick to render the dropdown menu with 10000 options and the power of more styling when we render the dropdown menu in the content process.

The current patch is very hacky, but I think we proved that this way works. My working repo is here: https://github.com/kuoe0/gecko-dev/tree/content-process-dropdown-menu

Comment 2

[Tommy Kuo (away forever...)]

Hi mats, there is my plan to implement this. Could you take a look at this and give me some feedback?

My basic idea is changing nsListControlFrame (the dropdown frame) to an absolute top-level frame so that we can leverage the existing codepath to render the dropdown menu on the top of other elements in the content process. To make it be an absolute top-level frame, I'll add the following properties in the UA stylesheet to do that.

```
-moz-top-layer: top;
position: absolute;
```

To make the dropdown frame work as an absolute frame, we have to allow the dropdown frame to resolve the style context from nsComboboxControlFrame (the combobox frame) and enable nsCanvasFrame to build the display list of the dropdown frames in its absolute list.

I'll update some code in ServoRestyleManager and nsFrame to make sure that the parent frame that we get for the dropdown frame is the combobox frame. So, we can find the correct parent style context for the dropdown frame.

And, we seem not to build the display list of the frames in the absolute list in nsCanvasFrame. I'll add the code to call the `BuildDisplayListForChild` to build the dropdown frame in nsCanvasFrame.

In addition, we don't reflow the dropdown frame when nsAbsoluteContainingBlock is doing reflow. Because when the combobox frame is reflowing, it needs the max height of options as its height. And also, we need some information of the combobox frame, such as width, to reflow the dropdown frame. So, we reflow the dropdown frame when the combobox frame is reflowing. And to prevent from reflowing the dropdown frame twice, we have to skip the reflow of the dropdown frame in nsAbsoluteContainingBlock.

Thanks!

Comment 3

[Mats Palmgren (inactive)]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #2)
> To make the dropdown frame work as an absolute frame, we have to allow the
> dropdown frame to resolve the style context from nsComboboxControlFrame (the
> combobox frame)

I don't understand what you mean by that.  I don't see why we need to change
anything about how the style context is created for the ListControlFrame.

> and enable nsCanvasFrame to build the display list of the
> dropdown frames in its absolute list.

That seems weird to me.  I think we should try to make nsComboboxControlFrame
deal with abs.pos. child frames instead and it should build the display list
normally.  I think we need to force nsComboboxControlFrame to always be an
abs.pos. containing block in the frame constructor.  (you could add
"position:relative" in forms.css temporarily to experiment with that).

> I'll update some code in ServoRestyleManager and nsFrame to make sure that
> the parent frame that we get for the dropdown frame is the combobox frame.
> So, we can find the correct parent style context for the dropdown frame.

I think we still want the ListControlFrame to be a child frame of
the nsComboboxControlFrame.

> And, we seem not to build the display list of the frames in the absolute
> list in nsCanvasFrame. I'll add the code to call the
> `BuildDisplayListForChild` to build the dropdown frame in nsCanvasFrame.

That's odd.  data:text/html,<div style="position:absolute">x
puts the <div> frame on the AbsoluteList of the nsCanvasFrame,
and it renders fine.  Anyway, I don't think we should let the dropdown
frame escape out of nsComboboxControlFrame.  It's an anonymous frame
which nsComboboxControlFrame owns.

Otherwise, I don't think we can control which frame ends up being the parent.
e.g. <div style="position:relative"><select> would make the <div> the parent,
right?

I'm not sure who builds the display list items for the dropdown currently,
I'd guess it's built as a child of nsComboboxControlFrame?  We should 
probably continue to do so.

> In addition, we don't reflow the dropdown frame when
> nsAbsoluteContainingBlock is doing reflow. Because when the combobox frame
> is reflowing, it needs the max height of options as its height. And also, we
> need some information of the combobox frame, such as width, to reflow the
> dropdown frame. So, we reflow the dropdown frame when the combobox frame is
> reflowing. And to prevent from reflowing the dropdown frame twice, we have
> to skip the reflow of the dropdown frame in nsAbsoluteContainingBlock.

I think we should try to keep most of the code that's positioning / reflowing
the dropdown in nsComboboxControlFrame as is.

Comment 4

[Tommy Kuo (away forever...)]

> I think we need to force nsComboboxControlFrame to always be an
abs.pos. containing block in the frame constructor. 

I don't understand this. What does "force nsComboboxControlFrame to always be an abs.pos. containing block" mean?

> "data:text/html,<div style="position:absolute">x" puts the <div> frame on the AbsoluteList of the nsCanvasFrame, and it renders fine.

I try to use the debugger to trace this, and I found that the absolute <div> would be put into the principle list, not the absolute list. I still don't know why.

---

BTW, I think I don't describe my design clearly in the last comment.

In my plan, I just change the dropdown frame to an absolute frame. Changing it to an absolute frame prevents the dropdown frame from being clipped by it parent frame. And changing it to a top-level frame also allows the dropdown frame would be put in the AbsoluteList of the top canvas frame. That would ensure that the dropdown frame will be on the top of all elements. Because I think <select> is a user input, it should not be hidden by any element.

There is the frame tree of "data:text/html,<select><option>1<option>2" with my design.

```
Canvas(html)
└─Block(html)
  └─Block(body)
    └─ComboboxControl(select)
      └─Placeholder(select) outOfFlowFrame=ListControl(select)
        ├─ComboboxDisplay(select)
        └─HTMLButtonControl(button)
AbsoluteList
└─ListControl(select)
  └─Block(select)
    ├─Block(option)
    └─Block(option)
```

I think the dropdown frame is still the child of the combobox frame in this structure. However, because the dropdown frame is an absolute (out-of-flow) frame, it would be put into the AbsoluteList. And we put a placeholder frame as the child of the combobox frame and point to the real dropdown fram. So, when some function, such as `GetCorrectedParent()`, wants to look up the parent style context of the dropdown frame, we have to call `GetPlaceholderFrame()` to reach the combobox frame, or we will find a wrong parent style context for the dropdown frame.

In this design, I still make the combobox frame deal the reflow of the dropdown frame. Every time we want to show the dropdown frame, we'll call `nsComboboxControlFrame::AbsolutelyPositionDropDown()` to deal the position and resize stuff. We also do this in the original design.

And I still deal the reflow by calling `nsComboboxControlFrame::ReflowDropdown()` when the combobox frame is reflowing. And that's why I have to skip the reflow of the dropdown frame to prevent the dropdown frame from reflowing twice when the AbsoluteList is reflowing.

If there is anything unclear, please let me know.

Comment 5

[Tommy Kuo (away forever...)]

The frame tree in comment 4 is wrong, and the correct one is following:

```
Canvas(html)
└─Block(html)
  └─Block(body)
    └─ComboboxControl(select)
      ├─Placeholder(select) outOfFlowFrame=ListControl(select)
      ├─ComboboxDisplay(select)
      └─HTMLButtonControl(button)
AbsoluteList
└─ListControl(select)
  └─Block(select)
    ├─Block(option)
    └─Block(option)
```

Comment 6

[Mats Palmgren (inactive)]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 (away 12/19 ~ 1/3) from comment #4)
> > I think we need to force nsComboboxControlFrame to always be an
> abs.pos. containing block in the frame constructor. 
> 
> I don't understand this. What does "force nsComboboxControlFrame to always
> be an abs.pos. containing block" mean?

Let's not worry about the details for now.  I'm mostly interested in
getting the frame tree right.

> > "data:text/html,<div style="position:absolute">x" puts the <div> frame on
> > the AbsoluteList of the nsCanvasFrame, and it renders fine.
> 
> I try to use the debugger to trace this, and I found that the absolute <div>
> would be put into the principle list, not the absolute list. I still don't
> know why.

It puts in the <div> frame in the AbsoluteList of the Canvas frame.
If you see something else then you have a broken build.

FYI, you can easily dump the frame tree if you start a debug build
from the command line with "firefox -layoutdebug ..."

> In my plan, I just change the dropdown frame to an absolute frame. Changing
> it to an absolute frame prevents the dropdown frame from being clipped by it
> parent frame.

No it doesn't prevent it from being clipped in general.  e.g.:
<div style="position:relative; overflow:hidden">
  <div style="position:absolute; top:200px;">Hello</div>
</div>

> And changing it to a top-level frame also allows the dropdown
> frame would be put in the AbsoluteList of the top canvas frame. That would
> ensure that the dropdown frame will be on the top of all elements.

OK, so "-moz-top-layer:top" does that?  Sorry, I was unfamiliar with what
that property does.  I'm guessing this is the reason it's not clipped then.

> Because I
> think <select> is a user input, it should not be hidden by any element.

Yes, we don't want the dropdown to be clipped by any ascendant boxes on
the page (only by the viewport).

> I think the dropdown frame is still the child of the combobox frame in this
> structure. However, because the dropdown frame is an absolute (out-of-flow)
> frame, it would be put into the AbsoluteList. And we put a placeholder frame
> as the child of the combobox frame and point to the real dropdown fram.

OK, I guess this is how a "-moz-top-layer:top" abs.pos. frame tree works
so it seems like it should be supported already.

> So,
> when some function, such as `GetCorrectedParent()`, wants to look up the
> parent style context of the dropdown frame, we have to call
> `GetPlaceholderFrame()` to reach the combobox frame, or we will find a wrong
> parent style context for the dropdown frame.

I don't quite understand this bit, why is the style context parent wrong
in the first place?  It works correctly for a non-top-layer abs.pos., e.g.: 
<div style="color:blue">
  <div style="position:absolute">Hello</div>
</div>
(this also putsmakes the abs.pos. frame a child of Canvas, but it's still blue)

so why do we need tweaks for "-moz-top-layer:top"?

> In this design, I still make the combobox frame deal the reflow of the
> dropdown frame. Every time we want to show the dropdown frame, we'll call
> `nsComboboxControlFrame::AbsolutelyPositionDropDown()` to deal the position
> and resize stuff. We also do this in the original design.

OK, good.

> And I still deal the reflow by calling
> `nsComboboxControlFrame::ReflowDropdown()` when the combobox frame is
> reflowing. And that's why I have to skip the reflow of the dropdown frame to
> prevent the dropdown frame from reflowing twice when the AbsoluteList is
> reflowing.

OK, that makes sense.

Comment 7

[Mats Palmgren (inactive)]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 (away 12/19 ~ 1/3) from comment #5)
OK, that frame tree is fine if that's what "-moz-top-layer:top" produces.
I'm still a bit worried that you say the style context parent isn't the correct
one.  Can you run "firefox -layoutdebug ..." and dump the frame tree from there?
It prints the style context pointers ("sc=...").

Comment 8

[Mats Palmgren (inactive)]

Xidorn, since you implemented -moz-top-layer, can you comment if using it
for the <select> dropdown seems reasonable and if you foresee any issues?

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Mats Palmgren (:mats) from comment #8)
> Xidorn, since you implemented -moz-top-layer, can you comment if using it
> for the <select> dropdown seems reasonable and if you foresee any issues?

The only problem comes to me immediately is that, we would not be able to render dropdown outside the viewport, but it seems to me that we are already not able to have dropdown outside viewport, which feels unfortunate :/ (Chrome can render dropdown outside viewport, fwiw)

Other than that, I don't see any fundamental problem. You may need to calculate the position carefully, since top layer element is always absolutely- or fixed-positioned, and it escapes from any transform thing, but I guess we already need to consider that?

You would need to add code into ViewportFrame::BuildDisplayListForTopLayer if you are not going to reuse canvas custom content mechanism.

Comment 10

[Tommy Kuo (away forever...)]

(In reply to Mats Palmgren (:mats) from comment #7)
> (In reply to Tommy Kuo [:kuoe0] at UTC+8 (away 12/19 ~ 1/3) from comment #5)
> OK, that frame tree is fine if that's what "-moz-top-layer:top" produces.
> I'm still a bit worried that you say the style context parent isn't the
> correct
> one.  Can you run "firefox -layoutdebug ..." and dump the frame tree from
> there?
> It prints the style context pointers ("sc=...").

I'm home now, and I'll attach the frame tree here tomorrow. In my patch, I add some code at [1] and [2] to make sure we can get the correct style context through the placeholder frame.

[1]: https://github.com/kuoe0/gecko-dev/commit/c769691ae629764d05fad79289fbedd09b7d9cce#diff-0f1d9e4ee2797489dc5f4c5b782a66f2
[2]: https://github.com/kuoe0/gecko-dev/commit/c769691ae629764d05fad79289fbedd09b7d9cce#diff-2655cf96f6ad23c8146c9254801027c7

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #9)
> Other than that, I don't see any fundamental problem. You may need to
> calculate the position carefully, since top layer element is always
> absolutely- or fixed-positioned, and it escapes from any transform thing,
> but I guess we already need to consider that?

Yes, I will consider this when calculating the position of the dropdown frame.

> You would need to add code into ViewportFrame::BuildDisplayListForTopLayer
> if you are not going to reuse canvas custom content mechanism.

I think the dropdown frame is just an anonymous frame, not an anonymous content. Maybe it would be difficult to reuse the mechanism that you mentioned. Currently, I add some code at [3] to make the dropdown frame would be painted.

[3]: https://github.com/kuoe0/gecko-dev/commit/c769691ae629764d05fad79289fbedd09b7d9cce#diff-bf9a8d60ebb965fb103039d38d443f04R608

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Had a brief look at that code, I think you need to build display list of the dropdown in ViewportFrame::BuildDisplayListForTopLayer rather than nsCanvasFrame::BuildDisplayList, otherwise the dropdown may be covered by at least top layer elements, e.g. fullscreen element or in the future, model <dialog>.

If you are going to use top layer, you should add a test for its interaction with fullscreen (i.e. whether a dropdown inside fullscreen would work as expected).

Comment 12

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #9)
> The only problem comes to me immediately is that, we would not be able to
> render dropdown outside the viewport, but it seems to me that we are already
> not able to have dropdown outside viewport, which feels unfortunate :/

Right, we're aware of that, but I think that if we "flip it inwards",
like we do when the dropdown would overlap the bottom screen edge,
or display fewer <option>s but with a scrollbar, then it should work
fine in most cases.  We would only clip it in extremely small windows
where it doesn't fit in either direction.  Regressing that edge case
seems worth it to fix all the current regressions we have.

> (Chrome can render dropdown outside viewport, fwiw)

Edge can't though, which makes me somewhat comfortable that it's
not a problem in practice.  (It's probably worth investigating
the exact heuristics they use in different scenarios...)

Comment 13

[Mats Palmgren (inactive)]

Making it impossible to display the dropdown outside the viewport
is also a nice improvement from a security point of view...

Comment 14

[Mats Palmgren (inactive)]

Tommy, please note that nsListControlFrame has a dual use - it's used
both for the dropdown in a combobox, but also as the primary frame for
<select size=N> where N > 1 (and such an element can of course have
position:absolute set on it by the author).

Comment 15

[Tommy Kuo (away forever...)]

(In reply to Mats Palmgren (:mats) from comment #14)
> Tommy, please note that nsListControlFrame has a dual use - it's used
> both for the dropdown in a combobox, but also as the primary frame for
> <select size=N> where N > 1 (and such an element can of course have
> position:absolute set on it by the author).

Yes, I know that. I have the test case in my local test cases.

(In reply to Mats Palmgren (:mats) from comment #6)
> > > "data:text/html,<div style="position:absolute">x" puts the <div> frame on
> > > the AbsoluteList of the nsCanvasFrame, and it renders fine.
> > 
> > I try to use the debugger to trace this, and I found that the absolute <div>
> > would be put into the principle list, not the absolute list. I still don't
> > know why.
> 
> It puts in the <div> frame in the AbsoluteList of the Canvas frame.
> If you see something else then you have a broken build.
> 
> FYI, you can easily dump the frame tree if you start a debug build
> from the command line with "firefox -layoutdebug ..."


BTW, in this case, I also saw the <div> in AbsoluteList when I dump the frame tree from -layoutdebug. However, I only saw we get the principle list in the code[1], and I'm not sure how do we build the display list of the frame in the AbsoluteList. And I try to use gdb to trace the code stack, I think the frame is built when we are building the html block frame.

[1]: https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/layout/generic/nsCanvasFrame.cpp#603-606

---

The code stack from lldb is following. (I used <select> frame for setting breakpoint easier, and the build is without my patch)

- 0x00000001149487c0 is <html>
- 0x0000000114948eb8 is <body>

* thread #1, queue = 'com.apple.main-thread', stop reason = step over
  * frame #0: 0x000000010473c35f XUL`nsComboboxControlFrame::BuildDisplayList(this=0x0000000114949770, aBuilder=0x00007fff5fbf9648, aLists=0x00007fff5fbf4b90) at nsComboboxControlFrame.cpp:1557 [opt]
    frame #1: 0x0000000104611920 XUL`nsIFrame::BuildDisplayListForChild(this=<unavailable>, aBuilder=<unavailable>, aChild=0x0000000114949770, aLists=0x00007fff5fbf4d18, aFlags=0) at nsFrame.cpp:3716 [opt]
    frame #2: 0x000000010463adeb XUL`DisplayLine(aBuilder=<unavailable>, aLineArea=<unavailable>, aLine=<unavailable>, aDepth=<unavailable>, aDrawnLines=<unavailable>, aLists=0x00007fff5fbf4ef8, aFrame=<unavailable>, aTextOverflow=<unavailable>, aLineNumberForTextOverflow=<unavailable>) at nsBlockFrame.cpp:6672 [opt]
    frame #3: 0x000000010463a151 XUL`nsBlockFrame::BuildDisplayList(this=0x0000000114948eb8, aBuilder=0x00007fff5fbf9648, aLists=<unavailable>) at nsBlockFrame.cpp:6767 [opt]
    frame #4: 0x000000010460fe3f XUL`nsIFrame::BuildDisplayListForChild(this=<unavailable>, aBuilder=<unavailable>, aChild=0x0000000114948eb8, aLists=0x00007fff5fbf5648, aFlags=<unavailable>) at nsFrame.cpp:3455 [opt]
    frame #5: 0x000000010463adeb XUL`DisplayLine(aBuilder=<unavailable>, aLineArea=<unavailable>, aLine=<unavailable>, aDepth=<unavailable>, aDrawnLines=<unavailable>, aLists=0x00007fff5fbf5828, aFrame=<unavailable>, aTextOverflow=<unavailable>, aLineNumberForTextOverflow=<unavailable>) at nsBlockFrame.cpp:6672 [opt]
    frame #6: 0x000000010463a151 XUL`nsBlockFrame::BuildDisplayList(this=0x00000001149487c0, aBuilder=0x00007fff5fbf9648, aLists=<unavailable>) at nsBlockFrame.cpp:6767 [opt]
    frame #7: 0x000000010460fe3f XUL`nsIFrame::BuildDisplayListForChild(this=<unavailable>, aBuilder=<unavailable>, aChild=0x00000001149487c0, aLists=0x00007fff5fbf67c0, aFlags=<unavailable>) at nsFrame.cpp:3455 [opt]
    frame #8: 0x0000000104648274 XUL`nsCanvasFrame::BuildDisplayList(this=0x00000001141fa308, aBuilder=0x00007fff5fbf9648, aLists=0x00007fff5fbf67c0) at nsCanvasFrame.cpp:605 [opt]

Comment 16

[Tommy Kuo (away forever...)]

Attachment: frame tree

The frame tree of "data:text/html,<select><option>1<option>2" with my patch is here.

Comment 17

[Tommy Kuo (away forever...)]

(In reply to Mats Palmgren (:mats) from comment #6)
> > So,
> > when some function, such as `GetCorrectedParent()`, wants to look up the
> > parent style context of the dropdown frame, we have to call
> > `GetPlaceholderFrame()` to reach the combobox frame, or we will find a wrong
> > parent style context for the dropdown frame.
> 
> I don't quite understand this bit, why is the style context parent wrong
> in the first place?  It works correctly for a non-top-layer abs.pos., e.g.: 
> <div style="color:blue">
>   <div style="position:absolute">Hello</div>
> </div>
> (this also putsmakes the abs.pos. frame a child of Canvas, but it's still
> blue)
> 
> so why do we need tweaks for "-moz-top-layer:top"?

This problem is not produced by "-moz-top-layer:top". It also happens when I just change the dropdown frame to an absolute frame. I'm not sure that whether it is related to the dropdown frame that is an anonymous frame or not. I'll take a look at this more and the test case that you provided to trace how it gets the correct frame when calling `CorrectStyleParentFrame()`.

Well, just a reminder, the frame tree that is produced by my patch is attached in comment 16.

Comment 18

[Tommy Kuo (away forever...)]

(In reply to Mats Palmgren (:mats) from comment #6)
> I don't quite understand this bit, why is the style context parent wrong
> in the first place?  It works correctly for a non-top-layer abs.pos., e.g.: 
> <div style="color:blue">
>   <div style="position:absolute">Hello</div>
> </div>
> (this also putsmakes the abs.pos. frame a child of Canvas, but it's still
> blue)

I removed the code that I added to find parent style context and got the following crash log when loading "data:text/html,<select><option>1<option>2". It happened when we are building scroll frame, and we cannot get the correct parent style context to construct the anonymous content of the scroll frame. In the while-loop[1], it will keep looking up its parent until find the root and then the parent would be null. Therefore, I add code to make it can find the combobox frame by getting the placeholder frame of the dropdown frame.

[1]: https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/layout/generic/nsFrame.cpp#9803-9827

[25863, Main Thread] ###!!! ASSERTION: Should have found a parent before this: 'aProspectiveParent->IsCanvasFrame()', file /Users/kuoe0/Works/Mozilla/gecko-dev/layout/generic/nsFrame.cpp, line 9829
Assertion failure: styleParentFrame || inheritFrame->IsCanvasFrame(), at /Users/kuoe0/Works/Mozilla/gecko-dev/layout/base/nsCSSFrameConstructor.cpp:11015
#01: nsCSSFrameConstructor::AddFCItemsForAnonymousContent(nsFrameConstructorState&, nsContainerFrame*, nsTArray<nsIAnonymousContentCreator::ContentInfo>&, nsCSSFrameConstructor::FrameConstructionItemList&, unsigned int)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57cc648]
#02: nsCSSFrameConstructor::BeginBuildingScrollFrame(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, nsAtom*, bool, nsContainerFrame*&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57cb629]
#03: nsCSSFrameConstructor::BuildScrollFrame(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, nsContainerFrame*, nsContainerFrame*&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57cdb59]
#04: nsCSSFrameConstructor::InitializeSelectFrame(nsFrameConstructorState&, nsContainerFrame*, nsContainerFrame*, nsIContent*, nsContainerFrame*, nsStyleContext*, bool, PendingBinding*, nsFrameItems&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57cd6f3]
#05: nsCSSFrameConstructor::ConstructSelectFrame(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57ccf9c]
#06: nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57d047a]
#07: nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57d69bf]
#08: nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameItems&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57c620d]
#09: nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, nsCSSFrameConstructor::InsertionKind, TreeMatchContext*)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57dc81f]
#10: nsCSSFrameConstructor::CreateNeededFrames(nsIContent*, TreeMatchContext&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57d9b4a]
#11: nsCSSFrameConstructor::CreateNeededFrames(nsIContent*, TreeMatchContext&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57d9c91]
#12: nsCSSFrameConstructor::CreateNeededFrames()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57dcd00]
#13: mozilla::GeckoRestyleManager::ProcessPendingRestyles()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5745d53]
#14: mozilla::RestyleManager::ProcessPendingRestyles()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x576537d]
#15: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x576edbf]
#16: nsIPresShell::FlushPendingNotifications(mozilla::ChangesToFlush)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x57200d0]
#17: mozilla::PresShell::DoFlushPendingNotifications(mozilla::FlushType)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x576e64b]
#18: nsIPresShell::FlushPendingNotifications(mozilla::FlushType)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x258f3b7]
#19: nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2976095]
#20: nsDocLoader::DocLoaderIsEmpty(bool)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1bac438]
#21: nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1bad7af]
#22: non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1badc9a]
#23: mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x360911]
#24: nsDocument::DoUnblockOnload()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2979780]
#25: nsDocument::UnblockOnload(bool)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x297953d]
#26: nsDocument::DispatchContentLoadedEvents()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2966f60]
#27: decltype(*(fp).*fp0(Get<>(fp1).PassAsParameter())) mozilla::detail::RunnableMethodArguments<>::applyImpl<nsDocument, void (nsDocument::*)()>(nsDocument*, void (nsDocument::*)(), mozilla::Tuple<>&, mozilla::IndexSequence<>)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x29c803e]
#28: _ZN7mozilla6detail23RunnableMethodArgumentsIJEE5applyI10nsDocumentMS4_FvvEEEDTcl9applyImplfp_fp0_dtdefpT10mArgumentscvNS_13IndexSequenceIJEEE_EEEPT_T0_[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x29c7fad]
#29: mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x29c7dde]
#30: nsThread::ProcessNextEvent(bool, bool*)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1ec3d0]
#31: NS_ProcessPendingEvents(nsIThread*, unsigned int)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x20cb4c]
#32: nsBaseAppShell::NativeEventCallback()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x526b3ee]
#33: nsAppShell::ProcessGeckoEvents(void*)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5309421]
#34: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0xa7321]
#35: __CFRunLoopDoSources0[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x8821d]
#36: __CFRunLoopRun[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x87716]
#37: CFRunLoopRunSpecific[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x87114]
#38: RunCurrentEventLoopInMode[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x30ebc]
#39: ReceiveNextEventCommon[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x30bf9]
#40: _BlockUntilNextEventMatchingListInModeWithFilter[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x30b26]
#41: _DPSNextEvent[/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x46a54]
#42: -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x7c27ee]
#43: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:][/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5307e14]
#44: -[NSApplication run][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x3b3db]
#45: nsAppShell::Run()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x530a047]
#46: nsAppStartup::Run()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x85ef93b]
#47: XREMain::XRE_mainRun()[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8790764]
#48: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x8791c8c]
#49: XRE_main(int, char**, mozilla::BootstrapConfig const&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x879240c]
#50: mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/XUL +0x87a5b87]
#51: do_main(int, char**, char**)[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/firefox +0x2036]
#52: main[/Users/kuoe0/Works/Mozilla/obj-darwin-desktop-debug-noopt/dist/NightlyDebug.app/Contents/MacOS/firefox +0x1b2a]
/Users/kuoe0/Works/Mozilla/gecko-dev/mach run --layoutdebug  9.42s user 1.06s system 95% cpu 10.954 total

Comment 19

[Mats Palmgren (inactive)]

Can you attach a rollup patch of what you have so far on this bug please?

Comment 20

[James Teh [:Jamie]]

I haven't investigated how this is implemented, but I'm concerned it might have implications for accessibility. It'd be great if we can make sure this gets an a11y review before shipping. Thanks!

Comment 21

[Tommy Kuo (away forever...)]

Attachment: WIP.patch

My working repo is https://github.com/kuoe0/gecko-dev/tree/content-process-dropdown-menu. I merged all commits into this patch.

Comment 22

[Mats Palmgren (inactive)]

Thanks, I investigated the CorrectStyleParentFrame issue a bit and it looks
like an existing bug to me.  We've simply not had any abs.pos. anon boxes
until now.  I think we should fix that generally just in case we get more
of those, so I filed bug 1423761 on fixing that.
Could you try the patch there and let me know if it works for you?
(revert your changes to CorrectStyleParentFrame / UpdateStyleOfChildAnonBox first)

Comment 23

[James Teh [:Jamie]]

I took this patch for a spin and did some accessibility testing:

1. When you expand the combo box (e.g. with alt+downArrow), the selected option should get accessibility focus, but it doesn't. The select itself still reports as having accessibility focus.
2. Interestingly, when you use the arrow keys to select an option, the newly selected option *does* get accessibility focus.
3. When you collapse the combo box after selecting an option (e.g. with escape), the select itself should get accessibility focus, but it doesn't. Instead, the selected option still keeps accessibility focus.
4. The options all report as being in a set of only 1 option; e.g. a screen reader would report "1 of 1" for all options instead of, say, "1 of 4".

Comment 24

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #23)
> I took this patch for a spin and did some accessibility testing:
> 1. When you expand the combo box (e.g. with alt+downArrow), the selected
> option should get accessibility focus, but it doesn't.

There are two parts to this:
a) nsComboboxControlFrame::ShowPopup currently returns before it fires the eXULPopupShowing event. (I see this code is still being developed.) Accessibility needs that event to tell clients the combo box has expanded. I tried moving this up, but accessibility still doesn't seem to get it for some reason.
b) The focus event should be handled by nsListControlFrame::FireMenuItemActiveEvent, but that doesn't seem to work properly when the combo box expands. It does work when the selected item is changed, though.
I'm wondering whether the drop down still isn't fully visible or similar when these events fire.

> 3. When you collapse the combo box after selecting an option (e.g. with
> escape), the select itself should get accessibility focus, but it doesn't.

This is fixed by ensuring eXULPopupHiding gets fired in nsComboboxControlFrame::ShowPopup.

> 4. The options all report as being in a set of only 1 option; e.g. a screen
> reader would report "1 of 1" for all options instead of, say, "1 of 4".

When calculating this group position info, accessibility excludes items that are "invisible", which is true for most items when the combo box is collapsed. This information is cached, and I suspect the cache isn't being cleared when it expands (and the options become visible). However, I'm not quite sure how this cache invalidation is supposed to occur.

This needs further investigation, but should be a decent starting point.

Comment 25

[James Teh [:Jamie]]

Attachment: Fix accessibility group info for content select dropdown implementation.

In the new implementation, Accessible::NativeState for the options doesn't include the invisible state. (Perhaps they have a frame now when they didn't before?) In HTMLSelectOptionAccessible::NativeState, rather than just flipping (xor) the invisible state, absolutely ensure it gets removed. This allows group position info to be calculated correctly.

Comment 26

[Tommy Kuo (away forever...)]

(In reply to Mats Palmgren (:mats) from comment #22)
> Thanks, I investigated the CorrectStyleParentFrame issue a bit and it looks
> like an existing bug to me.  We've simply not had any abs.pos. anon boxes
> until now.  I think we should fix that generally just in case we get more
> of those, so I filed bug 1423761 on fixing that.
> Could you try the patch there and let me know if it works for you?
> (revert your changes to CorrectStyleParentFrame / UpdateStyleOfChildAnonBox
> first)

Hi mats, I tried with my patch and it worked well. Thanks to your fix.

Comment 27

[Tommy Kuo (away forever...)]

(In reply to James Teh [:Jamie])

Thank you, Jamie! I'll investigate more when implementing.

Comment 28

[Mats Palmgren (inactive)]

Another thing we need to look into is event capturing when the dropdown
is open.  A few places related to that:
https://searchfox.org/mozilla-central/search?q=COMBOBOX_ROLLUP_CONSUME_EVENT
https://searchfox.org/mozilla-central/search?q=CaptureRollupEvents

Comment 29

[Tommy Kuo (away forever...)]

(In reply to Mats Palmgren (:mats) from comment #28)
> Another thing we need to look into is event capturing when the dropdown
> is open.  A few places related to that:
> https://searchfox.org/mozilla-central/search?q=COMBOBOX_ROLLUP_CONSUME_EVENT
> https://searchfox.org/mozilla-central/search?q=CaptureRollupEvents

Thanks for the reminder. I didn't investigate the event handling a lot for the prototype, and I reckon that there will be more events that we need to handle in the new design when starting to implement.

Comment 30

[Tommy Kuo (away forever...)]

Is there any concern about this design? Or, should we involve someone in discussing this? If the architecture is basically okay, I think I can start implementing it.

Comment 31

[Tommy Kuo (away forever...)]

Attachment: dropdown-menu-can-not-be-rendered-outside-the-iframe-area.png

the screenshot shows the limitation that renders the dropdown menu outside the iframe

Comment 32

[Tommy Kuo (away forever...)]

cc Matt Woodrow

Hi Matt, this bug is what I'm implementing and the screenshot in comment 31 is the issue that I mentioned tonight.

Comment 33

[Tommy Kuo (away forever...)]

Just a quick update here. Currently, I already landed the changes to make the dropdown frame be a top-level absolute frame (bug 1425088) and to skip the widget code (bug 1428297) when content-select is enabled. To finish this feature, we still need to finish the following works at least.

- Drop-down and roll-up event handling (bug 1428701)
- Correct the position and reflow result (bug 1429691)
- Accessibility support
- Paint the dropdown frame outside iframes
- Test cases (good to have)

Don't hesitate to take this work if anyone wants to work on this.

Comment 34

[:Gijs (he/him)]

Jet, this is a qf:p1 that showed up on my fxperf (ie frontend) list as well. Is it possible for someone on the platform side to pick up the patches here and/or in deps (e.g. bug 1428701 looks pretty close, but not quite there yet).

Comment 35

[Jet Villegas (inactive)]

(In reply to :Gijs (back Tuesday April 3rd) from comment #34)
> Jet, this is a qf:p1 that showed up on my fxperf (ie frontend) list as well.
> Is it possible for someone on the platform side to pick up the patches here
> and/or in deps (e.g. bug 1428701 looks pretty close, but not quite there
> yet).

I'm working on getting this picked back up. Leaving the NI:me open until that occurs.

Comment 36

[Tantek Çelik]

re: needinfo, discussed/(re)prioritized at SFAllHands

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One issue that came up is if this will be a problem if the <select> is inside an <iframe> and we start putting iframes in separate content processes.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

ni?ing myself for comment 36

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Just a heads up that Emma is an intern being mentored by Neil who we've tasked to take this on. She's just getting ramped up now.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

How will this work in case <select> is in an iframe and iframe is in a different process than its parent page?

Comment 41

[Bobby Holley (:bholley)]

No need to track this separately for qf, since bug 1118086 is also tracked.

Comment 42

[Mats Palmgren (inactive)]

(In reply to Olli Pettay [:smaug] from comment #40)
> How will this work in case <select> is in an iframe and iframe is in a
> different process than its parent page?

<select> inside <iframe> is very rare, and even when that happens,
the dropdown not fitting inside it is probably rare too.
Also, <iframe> is scrollable by default, so in the rare event
that a <select> dropdown is clipped we could make it possible to
scroll it into view (currently, I think we close the dropdown on
scroll).  At least it should be detectable in most cases that
there are some options that are out-of-view.  And if all else
fails, you can still change its value using kbd commands.
(Granted, some users might not know how to do that.)

I'm still in favor of restricting the dropdown to the viewport
extents though, as Tommy was working on here.  This has none of
the security, performance and webcompat issues that rendering
out-of-process has.

Comment 43

[Mats Palmgren (inactive)]

Note also that the existing non-e10s code dynamically resizes
the dropdown to fit in the space between the <select> button
and the viewport edge, and flip it upwards if there's more
space there.  It's only when the viewport is about the size
of the <select> or smaller that it will start to clip.
This case should be extremely rare.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Mats Palmgren (:mats) from comment #42)
> <select> inside <iframe> is very rare
How do you know?

> I'm still in favor of restricting the dropdown to the viewport
> extents though, as Tommy was working on here.  This has none of
> the security, performance and webcompat issues that rendering
> out-of-process has.
Limiting top top level page's viewport or iframe's viewport?

Comment 45

[Mats Palmgren (inactive)]

(In reply to Olli Pettay [:smaug] from comment #44)
> (In reply to Mats Palmgren (:mats) from comment #42)
> > <select> inside <iframe> is very rare
> How do you know?

Oh, just a guess based on ~20 years of bug triage. :-)
I could be wrong of course.

> Limiting top top level page's viewport or iframe's viewport?

The iframe's viewport.

Comment 46

[Bobby Holley (:bholley)]

I think I agree with Mats that constraining <select> dropdown rendering to the content process is acceptable:
* We already prevent the dropdown from rendering over browser UI (including bottom-anchored panels like devtools).
* I've played around the current behavior for the above, and it seems pretty reasonable - especially given that it flips the list inward if there's more space, and can be easily scrolled with the mouse and keyboard.
* It unlikely that a site would have a very large <select> element that is critical to site functionality and is also stuffed into a tiny _and_ cross-origin iframe.
* The performance issues are very real (bug 1118086), and we don't have another near-term plan to address them.
* We pay a high complexity cost with the existing proxy-to-parent-process approach. That cost will get higher if we try to address the aforementioned performance issues in the current design, and even higher as we need to add more rendering functionality to dropdowns (see bug 1118086 comment 87). There are a lot of other places where that engineering effort would be better spent.

Peter, can you ack the above from the Product side?

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

I'm having great trouble to understand how we can render <select> in child process, in case <select> is in an OOP-iframe. Other browsers don't seem to do that, and I'm very worried that some website just uses iframes heavily and <select> in them, and then we'll need to change our implementation afterwards.

Right now we implement all e10s-<select> in JS + XBL/XUL, nothing like our non-e10s <select> which is way faster. We have very high overhead just in the e10s implementation.

Comment 48

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #47)
> I'm having great trouble to understand how we can render <select> in child
> process, in case <select> is in an OOP-iframe. Other browsers don't seem to
> do that, and I'm very worried that some website just uses iframes heavily
> and <select> in them, and then we'll need to change our implementation
> afterwards.

FWIW, Chrome allows the dropdown to render over browser chrome and past the viewport, and we don't. In general, I think the UX of what we have is decent enough that the risk you describe is small. But we can see what peter thinks.



Note: to test this, go to: http://software.hixie.ch/utilities/js/live-dom-viewer/

And put the following in the text area:

<!DOCTYPE html>
<iframe src="https://silviomoreto.github.io/bootstrap-select/examples/"></iframe>


You can use devtools to add a bunch of options to the condiments list and get a feel for the behavior of large dropdown menus.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

We don't allow select to render past browser chrome because of historical security reasons. Since in non-e10s we let web site to style <option> in way better than other browsers, it was possible to have for example fullscreen <option> where background looked like windows desktop, and the <option> captured all the mouse events. As far as I know, in e10s we don't support such styling - like no other browser support either.

And I don't know what that has to do with the iframe case. We can't let other child processes to render iframe's select. But we could let of course some other process (compositor?) to render iframe's select.

Comment 50

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #49)
> We don't allow select to render past browser chrome because of historical
> security reasons. Since in non-e10s we let web site to style <option> in way
> better than other browsers, it was possible to have for example fullscreen
> <option> where background looked like windows desktop, and the <option>
> captured all the mouse events. As far as I know, in e10s we don't support
> such styling - like no other browser support either.

Right. But the net effect is that we have this restriction that other browsers don't, and I think the UX of the restriction is probably good enough that we could apply it at iframe boundaries.

> And I don't know what that has to do with the iframe case. We can't let
> other child processes to render iframe's select. But we could let of course
> some other process (compositor?) to render iframe's select.

At the cost of potential complexity and performance issues, yes. The people working on this seem to believe that we won't get both "simple" and "fast" with the out-of-process approach. Do you disagree? If not, the question for Peter is whether we're willing to trade either/both of those in exchange for rendering dropdowns across OOP iframe boundaries.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #50)
> Right. But the net effect is that we have this restriction that other
> browsers don't, and I think the UX of the restriction is probably good
> enough that we could apply it at iframe boundaries.
iframes are often small. Way smaller than top level viewport

> 
> > And I don't know what that has to do with the iframe case. We can't let
> > other child processes to render iframe's select. But we could let of course
> > some other process (compositor?) to render iframe's select.
> 
> At the cost of potential complexity and performance issues, yes. The people
> working on this seem to believe that we won't get both "simple" and "fast"
> with the out-of-process approach. Do you disagree?
Other implementations seem to have solved this issue by rendering in non-child-process
(or at least the select popup is eventually shown in non-child-process).
Why would it be harder for us to have similar setup.

And it doesn't really make sense to compare to our current implementation, since it is in many ways very slow.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

I was profiling bug 1118086 a bit.

A1 On child side we seem to have O(nlogn) indexing on child side.
A2 JS calls go through Xray/CCW.
A3 JS creates a wrapper for every <option> element.
A4 JS creates  CSSStyleDeclaration object for each <option> element
A5 and for each CSSStyleDeclaration object a wrapper object is created
A6 Data sent to the parent side is in structured clone form
 
Those take by far most of the time in child process.
As far as I see, A2, A3, A4, A5 could be removed totally and 
A1 could be, if implemented in JS, basically O(1) and A6 could be way faster.
So, I'd expect, based on this profile, time spent in child process to drop >90% or so if implemented in C++.

But it is parent process which is even slower.
B1 There we create elements using JS,
B2 so wrapper is created for all the elements. 
B3 Every access to the element goes through the bindings layer
B4 js::NurseryWrapperAwareHashMap::lookup takes some time
B5 We add tooltip support for each item in the list 
B6 items are xul:menuitems so each of them have XBL binding attached
B7 slow explicit style/layout flush. This is taking lots of time, and then there is refreshdriver tick which reflows again.

Child side optimizations look simple (== implement in C++), and on parent side just implementing this in C++ would help quite a bit too, but having something similar to non-e10s' <select>'s popup would probably help quite a bit with the reflow. (or <xul:tree> like element, which can show millions of entries without slow reflows)

Comment 53

[Bobby Holley (:bholley)]

Mats, Enn: Thoughts?

Comment 54

[Mike Conley (:mconley) (:⚙️)]

There are a few reasons I can think of to avoid using the parent process to render the dropdown. I apologize if I'm repeating the obvious, but I figured it'd be good to have these here in one place:

1. We rely on some fairly ugly hacks to prevent the dropdown from overlapping the browser UI, and generally restrict it to the content area. By moving the rendering to the content process, we get that restriction "for free" by leaning on our layers system and compositor to enforce where the content process can cause things to render. Having spoken briefly with mattwoodrow, I suspect gfx would be reluctant to give sub-processes the ability to draw outside of their iframe rects, unless we can present a pretty compelling case.

2. By moving the rendering to the content process, the parent is generally freed up to do other things.

3. By having the content render the dropdown, in theory, we could support a much wider variety of styling rules for <select> and <option>. We lost a lot of these capabilities when we moved to rendering in the parent process. We ended up supporting a small subset of the old stylings by serializing them up to the parent for e10s, but it's fairly restrictive. Mostly background-color and color.

To continue taking advantage of (1) and (2), one I idea I had was to have subframes message the top-level frame with information on how to render their <select> dropdowns, and have the top-level process ultimately be responsible for rendering it. That keeps the parent essentially out of it, and keeps us restricted to the content area.

It's unclear to me how that strategy would affect (3). I suspect based on a short conversation with mats in IRC that this would still require us to restrict how the dropdown is styled - though I wonder if perhaps we can do more than we do today.

Comment 55

[Mats Palmgren (inactive)]

I still think that rendering the dropdown in the UI process
is a dead end; for security, performance and webcompat reasons.

Rendering iframe <select>s in the top-level content-process is
slightly better on two points - it can't cause UI jank, and
the dropdown is constrained to the content area so it can't
overlap the URL bar and other UI (assuming we get rid of the
widget as planned).  But it's still going to be slower, less
secure and support CSS badly compared to rendering it directly
in the iframe.

Rendering a <select> dropdown inside a small iframe can be
solved by only displaying the number of <option>s that fits
inside it (scrolling the rest).  The iframe being so small
that it doesn't even fit a single <option> is a hypothetical
case IMO - I've never seen anything like that on the web.

> It's unclear to me how that strategy would affect (3).

I think we should regard content and style from a child iframe
as potentially malicious to its parent, so I don't think it
would allow much of an improvement compared to the current
situation.

So I still think that rendering the dropdown in the iframe
is the best solution.  If drawing it outside the iframe bounds
is deemed required then we should solve that in the gfx layer
IMO, by doing the layout / display list in the iframe process
and then exporting the resulting pixels somehow...

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Rendering in top level child process is worse from security point of view than using parent process.
Parent process can always validate the data it is showing, but top level page stealing data from cross-origin iframe is hard to fix if top level renders it.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

It took perhaps a minute to find an example where <select> is inside an iframe which is small enough that it couldn't render the popup in any too good way.
https://www.tutorialspoint.com/html/html_select_tag.htm

Comment 58

[Mats Palmgren (inactive)]

(In reply to Olli Pettay [:smaug] from comment #56)
> Rendering in top level child process is worse from security point of view
> than using parent process.

I don't see why it would be worse.  The top-level process would also
validate the data in the same way we currently do.  Maybe we're not
thinking about the same solution though.

Comment 59

[Mats Palmgren (inactive)]

(In reply to Olli Pettay [:smaug] from comment #57)
> It took perhaps a minute to find an example where <select> is inside an
> iframe which is small enough that it couldn't render the popup in any too
> good way.
> https://www.tutorialspoint.com/html/html_select_tag.htm

Fair enough, but it's a HTML example site.  The <select> doesn't really
perform a function on the site itself.

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Mats Palmgren (:mats) (on vacation) from comment #58) 
> I don't see why it would be worse.  The top-level process would also
> validate the data in the same way we currently do.
But it would be the top level page stealing data from the iframe.

If parent process shows the popup, nothing tries to steal data
(but iframe may try to attack the parent process by pushing some unexpected data).

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Mats Palmgren (:mats) (on vacation) from comment #59)
> 
> Fair enough, but it's a HTML example site.  The <select> doesn't really
> perform a function on the site itself.
Sure, but we would still break that site if select's popup was within iframe's viewport.

Comment 62

[Mats Palmgren (inactive)]

Right, there will be some fallout like that, but I still think that
it's going to be very rare so it's a minor issue.
You should compare that to the regressions that we currently *already*
have: see the dependency list on bug 1154677, bug 1409613, bug 1409645.
It's quite clear to me that the current solution is *far worse* from
a web-compat point of view.  (Note that all those issues also affects
the main document, not just <select>s in iframes.)

That said, I'm all in favor in rendering it outside the iframe if it
can be implemented in the graphics layer somehow, so that the layout
and styling happens in the iframe-process and then we send the
resulting pixels somewhere else for rendering, like the compositor
process.  And if that's impossible to implement, then I still think
rendering it inside the iframe would be a better solution than we
have now.

Comment 63

[Mike Conley (:mconley) (:⚙️)]

I'm going to try to summarize the 3 positions as I understand them:


1. Render in the parent process (smaug)

PROS:
  * Parent process can draw anywhere, so frame boundaries are not an issue
  * This is what we currently do
  * We've identified some things that we can potentially improve performance-wise here.
  * The parent is trusted, and is unlikely to do anything malicious with the <select> dropdown information.

CONS:
  * See comment 54


2. Render in the subframe, and be restricted to that subframe (mats)

PROS:
  * Full styling abilities, offering us greater web compatibility. This would differentiate us from our competition.
  * No messaging overhead, beyond what's necessary to send the dropdown frame to the compositor

CONS:
  * Dropdowns would be restricted to the subframe. The subframe could scroll to offer more options, but this is very much a departure from what other browsers do. It's unclear how much users would be bitten by this.


3. Render in the subframe, not restricted to the subframe (mats)

PROS:
  * Full styling abilities, offering us greater web compatibility. This would differentiate us from our competition.
  * No messaging overhead, beyond what's necessary to send the dropdown frame to the compositor

CONS:
  * Since an iframe can request a way of drawing outside of its rect, a compromised subprocess could potentially draw over parent frames, opening up the possibility for spoofing.
  * Graphics is not really comfortable with this, given how our layers system works


4. Render in the top-level frame (mconley)

PROS:
  * Avoids jank in the parent process
  * Gets content-area draw restriction "for free"

CONS:
  * Without adequate checking, hostile subframe could fool the top-level frame into drawing something it shouldn't
  * Hostile parent frame now has access to the <select> information for a potentially cross-origin iframe. A compromised top-level frame could, in theory, read that information out of the process memory.
  * Doesn't really give us much more styling capability, unless we serialize it

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

I guess I would prefer 3, assuming top level child process doesn't get the data, only compositor, 
but I have no idea how easy or hard it would be to implement, and we'd need to be careful to not let iframe to spoof its parent document by painting something unexpected on top of it.
If we care mostly only about performance, then 1 should be enough, and it doesn't have security issues.

Comment 65

[Mike Conley (:mconley) (:⚙️)]

dveditz has offered to weigh in from the security point of view here.

Comment 66

[Daniel Veditz [:dveditz]]

Several mentions here of a subframe trying to overlay and spoof its parent, but a parent's selects can also overlay a frame and potentially spoof things inside that, too. Whether that's believable in practice depends on how much styling we allow. Of course a user can't really trust the content that a frame is where the parent site says it is, but this might be useful for some kind of clickjacking-like attack.

I'm confused by the pro/cons of #2 in comment 63. How could full styling be both "greater web compatibility" AND "differentiate us from our competition"? If other browsers don't support it it's hard to believe the web is using it. And then the con of potentially truncated selects compared to other browser sounds like a true web compat problem since we would differ from other browsers (though how common I have no idea).

The more styling we allow that makes them look unlike a select (translucency, images, etc) the more they could be used in a spoofing attack. The biggest worry is content overlaying chrome, of course, so I'm happy all the proposals keep that restriction. Next would be a parent overlaying a child frame since a parent can manipulate the child with knowledge and precision. I think all the proposals allow that so it seems unwise to relax the styling restrictions. Child frame's select overlaying the parent doesn't worry me quite as much since the child doesn't usually know its context in the parent and there's a lot less control even when it does.

#4 seems to have the most downsides. You're having to pass the data to another process like #1, but a process we don't trust as much. Still, in practice whatever data is going into the select came out of a file that a compromised parent can probably request as a resource to get the data into the process. CORB protects against that once we implement it (bug 1459357).

Comment 67

[Mike Conley (:mconley) (:⚙️)]

(In reply to Daniel Veditz [:dveditz] from comment #66)
> I'm confused by the pro/cons of #2 in comment 63. How could full styling be
> both "greater web compatibility" AND "differentiate us from our
> competition"? If other browsers don't support it it's hard to believe the
> web is using it. And then the con of potentially truncated selects compared
> to other browser sounds like a true web compat problem since we would differ
> from other browsers (though how common I have no idea).
> 

I believe the idea here is that <option> elements are not defined in the web spec as having any styling restrictions upon them. Browser vendors, including us, seem to have taken it upon themselves to independently choose a subset of styles to support (which, I think mats argues, violates the spec).

Comment 68

[Matt Woodrow (:mattwoodrow)]

(In reply to Daniel Veditz [:dveditz] from comment #66)
> Several mentions here of a subframe trying to overlay and spoof its parent,
> but a parent's selects can also overlay a frame and potentially spoof things
> inside that, too. Whether that's believable in practice depends on how much
> styling we allow. Of course a user can't really trust the content that a
> frame is where the parent site says it is, but this might be useful for some
> kind of clickjacking-like attack.

The parent frame can just also render normal webcontent on top of the <iframe>'s area, so I don't think there's much we can ever do to prevent that. I'd generally be more concerned about an iframe click jacking the parent.

> #4 seems to have the most downsides. You're having to pass the data to
> another process like #1, but a process we don't trust as much. Still, in
> practice whatever data is going into the select came out of a file that a
> compromised parent can probably request as a resource to get the data into
> the process. CORB protects against that once we implement it (bug 1459357).

The difference between 3 and 4, is that the output of 3 is a pixel buffer, containing maybe a rendered dropdown, but also potentially arbitrary content if the subframe is compromised. 4 only outputs a set of data (which can be verified) about what things should be put into a dropdown.

As you say, the more styling options we allow to be serialised, the larger the surface area we need to validate, but it's still in theory possible (and we can restrict the set to make this easier). If we allow sharing of pixel buffers then we have no way checking if they contain what we expect.

I much prefer 4 over 3 for this reason, unless we decide that the security risk of one content process click-jacking another isn't big enough to worry about here.

Comment 69

[Mike Conley (:mconley) (:⚙️)]

So we've had DOM, Layout, Graphics, Security and Performance all weigh in here. Based on what I've read, I think no matter what we do, there's a trade-off that someone in this group doesn't really like.

I, personally, weigh the security and user safety concerns as more important than the web compatibility concerns. I'm also sufficiently worried based on smaug's quick search of web content that there's likely iframes out there with <select> dropdowns that expect to be able to extend outside of their frames.

So I hope all of the necessary data to make the decision is captured here in this bug. I'm not certain, though, who has the authority to make a call based on the data. I'll see if I can find such a person.

Comment 70

[Mike Conley (:mconley) (:⚙️)]

Hey dbaron, heycam,

Your names were raised as the two who should probably jointly decide on how we should proceed here. This is a pretty long bug, but comment 40 and forward are probably all you need to read to get a sense of what's going on.

How should we proceed?

Comment 71

[Cameron McCormack (:heycam)]

Attachment: e10s vs non-e10s select rendering

Another consideration that I don't think has been mentioned so far is the difference in degree of nativeness of the e10s and non-e10s implementations.  We can put work into the non-e10s implementation to make it look more native if we need to.

Comment 72

[Cameron McCormack (:heycam)]

Although there is a tension between making our <select> look more native, especially with regards to sizing, and Web compatibility, since Chrome's <select> is smaller than ours.  David guesses that by fixing up and switching to our non-e10s implementation, we might resolve some Web compat bugs.


So I tend to agree with Mats, guessing that there won't be enough situations where the <iframe> is small enough to make the <select> useless, so my initial feeling is to go with option 2.  And I like the simplicity of this option.  And I will note that if we really wanted accurate data, we could add Telemetry to find out how many times <select> popups would be too small to be useful when limited to their <iframe> size.

But in comment 46, Bobby suggests that *cross-origin* <iframe>s that are too small for their <select>s will be even less common than same-origin ones.  So I wonder if there's another option we can go with:

5. Render in the furthest ancestor same content process as the iframe

This has all the same pros and cons as option 2 -- and particularly it doesn't need any remoting of the option data -- but should reduce the chance of breaking <select>-in-<iframe> content.  It is would be a little more complex than purely option 2, but not more than options 3 and 4, at a guess.

What do people think of this?

Comment 73

[Mike Conley (:mconley) (:⚙️)]

(In reply to Cameron McCormack (:heycam) from comment #72)
> 5. Render in the furthest ancestor same content process as the iframe
> 

I assume this means that if we have a frame tree structure like this:

        A1
       / \
      /   \
     B1   C1
    / \   / \
   /   \  D   A2
  B2   C2

Where frames starting with the same letter share the same process, then a <select> dropdown in A2 would be rendered by A1, a dropdown in B2 would be rendered in B1, and the dropdowns in C2 and D would be restricted to themselves?

If I understand correctly, then yes, I think I like it - thanks, heycam. Essentially, as I see it, we're attempting to minimize the sort of case that smaug brings up in comment 57.

Are there any objections to this proposal?

* Part of me suspects we're making a mountain out of a molehill, but I'm worried enough about breaking users' expectations of something as old and widely used as <select> that I think this discussion has been worth it.

Comment 74

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think heycam may have meant "furthest contiguous ancestor", i.e., revising your answer for A2 to be rendered by itself.  At least, that's what I was thinking when we talked about it -- and I think it's a lot more straightforward.  The idea is that there's no need to worry about crossing process boundaries.

Comment 75

[Cameron McCormack (:heycam)]

I neglected to think about the case where there are intervening cross-origin iframes between two same-process iframes.  But I agree with David that limiting this approach to the furthest continguous ancestor is simpler and cover enough of the cases we care about.

Comment 76

[James Teh [:Jamie]]

Comment on attachment 8935272 [details] [diff] [review]
Fix accessibility group info for content select dropdown implementation.

This patch was landed in bug 1453555.

Comment 77

[Mike Conley (:mconley) (:⚙️)]

(In reply to Cameron McCormack (:heycam) from comment #75)
> I neglected to think about the case where there are intervening cross-origin
> iframes between two same-process iframes.  But I agree with David that
> limiting this approach to the furthest continguous ancestor is simpler and
> cover enough of the cases we care about.

Okay, I'm into it. Thanks for clearing that up!

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

(I'm still having great trouble to understand why we'd want to implement <select> in so different way than other browsers, and by doing so, possibly cause compatibility issues.)

Comment 79

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Olli Pettay [:smaug] (vacation Jul 15->) from comment #78)
> (I'm still having great trouble to understand why we'd want to implement
> <select> in so different way than other browsers, and by doing so, possibly
> cause compatibility issues.)

Which differences in particular are you concerned about?  popup positioning, option sizing, and/or option stylability?

Right now, with e10s, our <options> are less stylable than Chrome's (e.g., don't support color or background), which developers complain about, and substantially larger on at least some platforms, which can cause compatibility issues.  Both of those are regressions from the e10s changes.

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

Sizing and positioning, especially sizing.

If our current <option>s are less styleable, that is something to fix. If other browsers have managed to do it by sending style information to some other process, we should be able to do the same.

Comment 81

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think our pre-e10s code was substantially more compatible with other browsers on sizing of <options> than our current code.

But it also seems like you're arguing that we should be rendering the options in the parent process.  Why?  (I worry a little that it's yet another thing we have to worry about security risks of transferring to the parent process.  And our current implementation of rendering in the parent has a bunch of disadvantages that aren't intrinsic to rendering in the parent, mostly related to its use of XUL <listbox> and both its styleability and heavy use of XBL.)

Comment 82

[Olli Pettay [:smaug][[e-mail]]]

I guess we're talking about different sizing. I'm talking about the size of the popup when <select> is inside (cross-origin) iframe, if rendering happens in child proces. If there isn't enough space to render the popup, it could be useless from user's point of view, but the page would be working fine in other browsers.

Comment 83

[Neil Deakin]

Attachment: Rollup patch with all content-select changes

Mats wanted to look at the assertions and test failures caused by this patch

Comment 84

[Mike Conley (:mconley) (:⚙️)]

Hey mats - will you have time to look at this soon?

Comment 85

[Jonathan Watt [:jwatt]]

Are we also planning to clip the context menu in an OOP iframe to the iframe's bounds?

Comment 86

[Mats Palmgren (inactive)]

A few initial comments and questions:

Is there a reason you introduce the :-moz-dropdown-list-parent pseudo?

I think the nsComboboxControlFrame::IsDisplayMode* functions
should be static (and probably leave them in nsLayoutUtils to 
avoid adding nsComboboxControlFrame.h #includes for those).
Can we make all the prefs StaticPrefs?

The new dependence on mozilla::dom::TabChild seems unfortunate.
Is there a reason we can't use the existing global RollupListener
for this? (gRollupListener)
(I think we should eventually replace the DropdownDisplayMode_Native
with this and then we don't have a TabChild (I think).  Do we have
a TabChild when running "firefox -layoutdebug"?)

We seem to use the wrong area inside <iframe>s when calculating
how many options will fit inside its viewport.

It appears we reflow the nsListControlFrame twice; first from
nsComboboxControlFrame::Reflow (via ReflowDropdown()), then
again when nsCanvasFrame reflows its abs.pos. children.
(I saw a few "ASSERTION: Out-of-flow frame got reflowed before
its placeholder" which probably comes from this (ReflowDropdown
is before nsBlockFrame::Reflow which reflows the placeholder).)

Comment 87

[Mats Palmgren (inactive)]

It appears we now allow mouse-wheel scrolling to propagate to
the <select> parent.  E.g. create a <select> with just a few
options that don't trigger a scrollbar on the dropdown menu 
itself.  After opening the dropdown menu, mouse-wheel scrolling
over it scrolls the viewport scrollbar (if any).  This doesn't
happen in Native or Parent mode currently and should probably
be prevented in Content mode too.

Comment 88

[Mats Palmgren (inactive)]

Regarding the add "if (!comboboxFrame->IsDisplayModeContent())"
around existing assertions in nsCSSFrameConstructor.cpp:
we should integrate that into the assertion condition instead,
i.e. we should assert that we do get a positioned dropdown list
and that it doesn't have a view, respectively.

The third one (wrapping the SetInitialChildList call) should
probably be "if (!listFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW))"
instead.

Comment 89

[Mats Palmgren (inactive)]

I /think/ the change to AppendDirectlyOwnedAnonBoxes is correct given
that the placeholder is now a child of the nsComboboxControlFrame
so any style changes should now be picked up from that.
(I'm guessing we needed it before since the dropdown frame is on
the SelectPopupList which isn't traversed by the style system.)

Emilio, can you confirm? (see "frame tree" attachment)

Comment 90

[Emilio Cobos Álvarez (:emilio)]

That doesn't sound right to me.

The style system doesn't traverse the frame tree itself, so unless the dropdown frame is anon content (which it isn't) that change seems wrong.

Why is that change needed in the first place?

Comment 91

[Emilio Cobos Álvarez (:emilio)]

Given this is an special anonymous box, in the sense that it's positioned, this probably needs a setup similar to the one we have for the table wrapper frames, in order to avoid optimizing away change hints.

Comment 92

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #86)
> We seem to use the wrong area inside <iframe>s when calculating
> how many options will fit inside its viewport.

Actually, it probably is correct for what we intended to do...
The unsolved issue is that the display items are still clipped
by the <iframe>'s viewport.
If I make nsSubDocumentFrame::ShouldClipSubdocument() always
return false, then it works as we intended.  What is missing
is to make add an exception for dropdown frames so that they
aren't affected by the DisplayListClipState that the
nsSubDocumentFrame adds.

Alternatively, just pretend all <iframe>s are in a separate
process and size the dropdown to the local viewport, i.e.
something like:
-  nsPresContext* pc = PresContext()->GetToplevelContentDocumentPresContext();
+  nsPresContext* pc = IsDisplayModeContent() ? PresContext() :
+    PresContext()->GetToplevelContentDocumentPresContext();
(in nsComboboxControlFrame::GetAvailableDropdownSpace)

Comment 93

[Mats Palmgren (inactive)]

Attachment: WIP - don't clip the dropdown frame in <iframe>s. (on top of the rollup patch)

Comment 94

[Mats Palmgren (inactive)]

Comment on attachment 9024485 [details] [diff] [review]
WIP - don't clip the dropdown frame in <iframe>s.  (on top of the rollup patch)

Fwiw, this makes the dropdown display outside the <iframe> for me on Linux.
I think it should be possible to do this for same-process <iframe>s at least.

Comment 95

[Neil Deakin]

(In reply to Mats Palmgren (:mats) from comment #86)
> A few initial comments and questions:
> 
> Is there a reason you introduce the :-moz-dropdown-list-parent pseudo?

This was a temporary hack solely so that the -moz-top-layer rule doesn't apply in the parent process where content-selects are not used, as it makes the position appear incorrect. A better solution would be welcome.


> I think the nsComboboxControlFrame::IsDisplayMode* functions
> should be static (and probably leave them in nsLayoutUtils to 
> avoid adding nsComboboxControlFrame.h #includes for those).
> Can we make all the prefs StaticPrefs?

I think we can do this.

> The new dependence on mozilla::dom::TabChild seems unfortunate.
> Is there a reason we can't use the existing global RollupListener
> for this? (gRollupListener)
> (I think we should eventually replace the DropdownDisplayMode_Native
> with this and then we don't have a TabChild (I think).  Do we have
> a TabChild when running "firefox -layoutdebug"?)

A quick glance suggests I could add a getter for gRollupListener and use that instead.

> It appears we reflow the nsListControlFrame twice; first from
> nsComboboxControlFrame::Reflow (via ReflowDropdown()), then
> again when nsCanvasFrame reflows its abs.pos. children.
> (I saw a few "ASSERTION: Out-of-flow frame got reflowed before
> its placeholder" which probably comes from this (ReflowDropdown
> is before nsBlockFrame::Reflow which reflows the placeholder).)

Yes, this is the part I was asking about how to fix.

Comment 96

[Mike Conley (:mconley) (:⚙️)]

Having consulted with Product on this, we've decided to cancel the content-select project. Tommy, Emma and Neil have done fantastic work here getting this started, but given how it breaks user expectations and accessibility under certain circumstances with Fission, we think this is the correct course of action.

Improvements to the <select> dropdown for additional styling capabilities and improved performance are still worth investing in, but I'm afraid rendering the dropdown in the content process, what with Fission bearing down on us, is not.

Comment 97

[Boris Zbarsky [:bzbarsky]]

Do we have an alternate plan for resolving bug 1118086?

Comment 98

[Mike Conley (:mconley) (:⚙️)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #97)

Do we have an alternate plan for resolving bug 1118086?

A few things come to mind:

1. We could send the <option>'s list to the parent in batches, and have it assemble it in multiple ticks of the event loop
2. We could investigate whether or not we're doing silly things when constructing and displaying the menulist itself.

Neither of these, however, are the focus of the front-end performance team right now (we're kinda laser-focused on start-up at the moment).

Comment 99

[Emilio Cobos Álvarez (:emilio)]

There are also a few styling issues that are not clear to me how to properly handle without reworking how our current select code works.

In particular, stuff like bug 1576820 (we have other dupes of that I'm sure)...

We should probably brainstorm a bit more about this... Can we render the popup in-content, and just send the rendered thing to the graphics layer directly, maybe? That seems hard for hit-testing or such... I don't know, but other browsers seem to handle that fine.

Do other browsers suffer from the same issues that made this bug wontfix?

Comment 100

[Mike Conley (:mconley) (:⚙️)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #99)

Do other browsers suffer from the same issues that made this bug wontfix?

I believe Blink-based browsers, old-Edge, IE and Safari all render their <select> dropdowns in the parent process, so they don't suffer any of the same issues that made this WONTFIX.

This also means that they, like us, only implement a small subset of the styling rules for <option> elements in <select> dropdowns.

Comment 101

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #100)

I believe Blink-based browsers, old-Edge, IE and Safari all render their <select> dropdowns in the parent process, so they don't suffer any of the same issues that made this WONTFIX.

Yet bug 1576820 works there, looks like, at least in Chrome... So they're clearly doing something different.

Comment 102

[Emilio Cobos Álvarez (:emilio)]

I'll dig a bit more.

Comment 103

[Emilio Cobos Álvarez (:emilio)]

So apparently Blink uses out-of-page (out of process) popups in Mac and Android, but not Windows and Linux:

• https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?l=721&rcl=ab4ecdb6a2dacf0a35344c2baa9509a9659eec86
• https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/page/chrome_client_impl.cc?l=856&rcl=ab4ecdb6a2dacf0a35344c2baa9509a9659eec86

That's a bit surprising, but not too much I guess?