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.

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

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!

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

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

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

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

(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=...").

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?

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

(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

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

(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...)

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

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

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

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

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

(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

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

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!

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

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)

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".

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

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.

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

(In reply to James Teh [:Jamie]) Thank you, Jamie! I'll investigate more when implementing.

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

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

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.

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

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.

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.

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

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

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

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.

ni?ing myself for comment 36

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.

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

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

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

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.

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

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

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?

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.

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

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.

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

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

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)

Mats, Enn: Thoughts?

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.

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

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.

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

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

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

(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).

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

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.

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

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.

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

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

(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).

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

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.

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?

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.

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?

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

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.

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 on attachment 8935272 [details] [diff] [review] Fix accessibility group info for content select dropdown implementation. This patch was landed in bug 1453555.

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

(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.)

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

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.

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

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.

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

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

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

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

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.

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.

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)

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?

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.

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

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