Open Bug 1428701 Opened 6 years ago Updated 2 years ago

Allow to toggle the dropdown frame when content-select is enabled

Categories

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

Product:
Core
Component:
Layout: Form Controls
Type:
enhancement

Tracking

()

People

(Reporter: kuoe0.tw, Unassigned)

References

Details

Attachments

(2 files, 16 obsolete files)

Part 1 - nsBaseWidget::GetActiveRollupListener should return null when there is no open popup
12.82 KB, patch
Details | Diff | Splinter Review
Part 2 - capture rollup events in the parent process and send them to the select as needed
44.90 KB, patch
Details | Diff | Splinter Review 
After getting rid of the widget/view usage to show the dropdown frame, we have to build the display list in the content process to show it. And the dropdown frame should be toggleable by clicking the combobox.
Priority: -- → P3
Attachment #8941749 - Flags: review?(mats)
Attachment #8941750 - Flags: review?(mats)
Comment on attachment 8941749 [details]
Bug 1428701 - (Part 1) Build the dropdown frame as the top layer frame in the viewport frame

https://reviewboard.mozilla.org/r/211990/#review217920
Attachment #8941749 - Flags: review?(mats) → review+
Comment on attachment 8941750 [details]
Bug 1428701 - (Part 2) Handle the drop-down and the roll-up actions for content-select

https://reviewboard.mozilla.org/r/211992/#review217926

::: layout/forms/nsComboboxControlFrame.cpp:1199
(Diff revision 1)
> +  // We handle the drop-down and roll-up actions here.
> +  if (nsLayoutUtils::IsContentSelectEnabled() &&
> +      aEvent->mMessage == eMouseDown) {
> +    ShowDropDown(!mDroppedDown);
> +  }

Can you explain why this is needed? (and the change to nsListControlFrame::MouseDown)
Why doesn't the existing non-e10s event handling work as is?
Attachment #8941749 - Flags: review?(xidorn+moz)
Attachment #8941750 - Flags: review?(mats)
Hi Xidorn, could you also review the part 1 patch? I think you're the person who is most familiar with the code of top layer contents.
(In reply to Mats Palmgren (:mats) from comment #4)
> Can you explain why this is needed? (and the change to
> nsListControlFrame::MouseDown)
> Why doesn't the existing non-e10s event handling work as is?

I have tried to use the existing non-e10s event handling in `nsListControlFrame::MouseDown()` to toggle the dropdown menu. The result is that I can drop the menu down, but I cannot roll it up. After analyzing the code, I found the roll-up behavior is triggered from the widget code such as 'cocoa/nsChildView.mm' on macOS. And also, I think handling the drop-down and roll-up behaviors triggered by clicking on the combo box only in combo box is more reasonable to me. That's why I put the handling code there and skip some code in `nsListControlFrame::MouseDown()`. I'm not sure that this is the best way to do that, so if there is any better way to do that, please let me know. Thanks!
Flags: needinfo?(mats)
Comment on attachment 8941749 [details]
Bug 1428701 - (Part 1) Build the dropdown frame as the top layer frame in the viewport frame

https://reviewboard.mozilla.org/r/211990/#review218040

::: layout/generic/ViewportFrame.cpp:192
(Diff revision 1)
> +    for (nsIFrame* frame : canvasFrame->GetChildList(nsIFrame::kAbsoluteList)) {
> +      if (frame->IsListControlFrame()) {
> +        BuildDisplayListForTopLayerFrame(aBuilder, frame, aList);
> +      }
> +    }

Questions:
1. When is the list control frame added to the absolute list of canvas frame? I did some search in the code, but failed to find anything. And it seems to me that canvas frame doesn't allow manipulating non-principal frame list in general.
2. How is the display list of this frame currently being built? In general, we move something from elsewhere to the top layer, which indicates you should be skipping building the display list for it somewhere else when you are adding it here. Are we building display list for this frame twice now? Otherwise this frame was never shown before?
Attachment #8941749 - Flags: review?(xidorn+moz) → review-
Comment on attachment 8941749 [details]
Bug 1428701 - (Part 1) Build the dropdown frame as the top layer frame in the viewport frame

https://reviewboard.mozilla.org/r/211990/#review218044

::: layout/generic/ViewportFrame.cpp:191
(Diff revision 1)
>        if (nsIFrame* frame = container->GetPrimaryFrame()) {
>          BuildDisplayListForTopLayerFrame(aBuilder, frame, aList);
>        }
>      }
> +
> +    // Build display items for dropdown menus

Another thing, did we conclude that we want to draw the dropdown on top of UI stuff like highlighter and probably screenshot buttons?

I tend to think those should always be painted on top of anything that content can affect, including the dropdown list.
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #7)

> 1. When is the list control frame added to the absolute list of canvas
> frame? I did some search in the code, but failed to find anything. And it
> seems to me that canvas frame doesn't allow manipulating non-principal frame
> list in general.

The list control frame would be a top level absolute frame when content-select is enabled, see [1]. And it is added to the absolute list at [2] when the function in [2] is called by [3].

[1]: https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/layout/style/res/forms.css#425-430
[2]: https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/layout/base/nsCSSFrameConstructor.cpp#1427
[3]: https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/layout/base/nsCSSFrameConstructor.cpp#1092

> 2. How is the display list of this frame currently being built? In general,
> we move something from elsewhere to the top layer, which indicates you
> should be skipping building the display list for it somewhere else when you
> are adding it here. Are we building display list for this frame twice now?
> Otherwise this frame was never shown before?

In e10s mode, this frame was never rendered as we use the platform dropdown menu. And without using platform dropdown menu, we render list control frame in the widget. When content-select is enabled, we won't use widgets to render the list control frame. So, we don't need to worry about building the display list twice.


(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #8)
> Another thing, did we conclude that we want to draw the dropdown on top of
> UI stuff like highlighter and probably screenshot buttons?

I think I didn't consider this case in my patch. In this case, where would be the better position that you think to put this code?
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #9)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #7)
> 
> > 1. When is the list control frame added to the absolute list of canvas
> > frame? I did some search in the code, but failed to find anything. And it
> > seems to me that canvas frame doesn't allow manipulating non-principal frame
> > list in general.
> 
> The list control frame would be a top level absolute frame when
> content-select is enabled, see [1]. And it is added to the absolute list at
> [2] when the function in [2] is called by [3].

Oh, okay, so canvas frame is the default containing block for absolute frames. That actually makes me a bit concerned about the performance of iterating over the absolute list unconditionally... It is probably okay, but it would definitely be better if we only do that when we know we have a dropdown to display, or even better, know the frame we want to display. Is that possible?

Also, please assert that the frame indeed has mTopLayer == NS_STYLE_TOP_LAYER_NONE, so that we know this, as an absolute frame, is guaranteed to be skipped in normal display list building.

> > 2. How is the display list of this frame currently being built? In general,
> > we move something from elsewhere to the top layer, which indicates you
> > should be skipping building the display list for it somewhere else when you
> > are adding it here. Are we building display list for this frame twice now?
> > Otherwise this frame was never shown before?
> 
> In e10s mode, this frame was never rendered as we use the platform dropdown
> menu. And without using platform dropdown menu, we render list control frame
> in the widget. When content-select is enabled, we won't use widgets to
> render the list control frame. So, we don't need to worry about building the
> display list twice.

Does it mean that when the content-select is enabled, we don't display the dropdown at all currently (without the patch)?

> (In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #8)
> > Another thing, did we conclude that we want to draw the dropdown on top of
> > UI stuff like highlighter and probably screenshot buttons?
> 
> I think I didn't consider this case in my patch. In this case, where would
> be the better position that you think to put this code?

So it should probably be put before custom content container, which is where we put UI stuff in.
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #6)
> I have tried to use the existing non-e10s event handling in
> `nsListControlFrame::MouseDown()` to toggle the dropdown menu. The result is
> that I can drop the menu down, but I cannot roll it up.

Right, but the changes you propose only partly solves the problem
and adds new bugs (such as right-click opening the dropdown when
it shouldn't).  This is why I think reusing the current non-e10s
code, which have been in production use for decades (although not
so much recently after e10s became default), seems like an easier
solution that has a better chance at handling events correctly.
But, as I said in bug 1421229 comment 28, we need to figure out
how to do the event capturing that we're currently doing using
the <select> widget.  For example, when the dropdown is open
and a scroll-wheel event occurs it should close the dropdown
(no scrolling should occur).

Can we use the root widget instead for the event capturing?
Can we use nsXULPopupManager code to initiate the rollup on
the combobox frame somehow? (I think we always have one of those
and it's a nsIRollupListener).
Flags: needinfo?(mats)
It seems that the abs.pos. dropdown frame is always reflowed and contributes
to the scrollable overflow of the viewport.  That seems wrong to me - that
frame needs to have zero size when the dropdown is closed.
Probably your best bet is to create something (probably a method added to nsIRollupListener) in the parent process which keeps track of the rect of the pretend dropdown and then change the callers of Rollup() (these are different on each platform) to be able to handle checking that instead of the popup widget's rect that they normally check. Then have nsIRollupListener::Rollup send a message down to the child to close the popup.

That would probably handle the events the best and would allow the interaction with other popups that might open/close to remain consistent.

But you may also want event capturing and the key/mouse grabs on Linux as well which would need some form of real native widget.
(In reply to Mats Palmgren (:mats) from comment #12)
> It seems that the abs.pos. dropdown frame is always reflowed and contributes
> to the scrollable overflow of the viewport.  That seems wrong to me - that
> frame needs to have zero size when the dropdown is closed.

I know this is wrong, and I plan to fix it in Bug 1429691.
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #10)
> Oh, okay, so canvas frame is the default containing block for absolute
> frames. That actually makes me a bit concerned about the performance of
> iterating over the absolute list unconditionally... It is probably okay, but
> it would definitely be better if we only do that when we know we have a
> dropdown to display, or even better, know the frame we want to display. Is
> that possible?

I decide to add the frame that would be dropped down into the canvas frame. So, we can get the dropdown frame directly if there is a dropdown frame should be rendered. I'll add the following code to the place where we toggle the dropdown menu. What do you think about this idea?

```
nsCanvasFrame* canvasFrame = do_QueryFrame(mDropdownFrame->GetParent());
if (mDroppedDown) {
  canvasFrame->SetDropdownFrame(mDropdownFrame);
} else {
  canvasFrame->SetDropdownFrame(nullptr);
}
```

> Also, please assert that the frame indeed has mTopLayer ==
> NS_STYLE_TOP_LAYER_NONE, so that we know this, as an absolute frame, is
> guaranteed to be skipped in normal display list building.

I'm not sure where the place that you wish me to add the assertion is.

> Does it mean that when the content-select is enabled, we don't display the
> dropdown at all currently (without the patch)?

Yes, we don't display any dropdown frame currently if content-select is enabled.

> So it should probably be put before custom content container, which is where
> we put UI stuff in.

Okay!
Flags: needinfo?(xidorn+moz)
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #17)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #10)
> > Oh, okay, so canvas frame is the default containing block for absolute
> > frames. That actually makes me a bit concerned about the performance of
> > iterating over the absolute list unconditionally... It is probably okay, but
> > it would definitely be better if we only do that when we know we have a
> > dropdown to display, or even better, know the frame we want to display. Is
> > that possible?
> 
> I decide to add the frame that would be dropped down into the canvas frame.
> So, we can get the dropdown frame directly if there is a dropdown frame
> should be rendered. I'll add the following code to the place where we toggle
> the dropdown menu. What do you think about this idea?
> 
> ```
> nsCanvasFrame* canvasFrame = do_QueryFrame(mDropdownFrame->GetParent());
> if (mDroppedDown) {
>   canvasFrame->SetDropdownFrame(mDropdownFrame);
> } else {
>   canvasFrame->SetDropdownFrame(nullptr);
> }
> ```

I guess it is okay as far as you take care of the lifetime of the dropdown frame.

> > Also, please assert that the frame indeed has mTopLayer ==
> > NS_STYLE_TOP_LAYER_NONE, so that we know this, as an absolute frame, is
> > guaranteed to be skipped in normal display list building.
> 
> I'm not sure where the place that you wish me to add the assertion is.

Probably just before you call into BuildDisplayListForTopLayerFrame.
Flags: needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #18)
> > > Also, please assert that the frame indeed has mTopLayer ==
> > > NS_STYLE_TOP_LAYER_NONE, so that we know this, as an absolute frame, is
> > > guaranteed to be skipped in normal display list building.
> > 
> > I'm not sure where the place that you wish me to add the assertion is.
> 
> Probably just before you call into BuildDisplayListForTopLayerFrame.

I feel a little confuse here. I think we should assert that the frame has "mTopLayer != NS_STYLE_TOP_LAYER_NONE" not "mTopLayer == NS_STYLE_TOP_LAYER_NONE", because the display list that we want to build by `BuildDisplayListForTopLayerFrame()` must has "-moz-top-layer: top". Or, did I misunderstand what you mean of the assertion?
Flags: needinfo?(xidorn+moz)
(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #19)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (PTO Jan 19 ~ 29) from comment #18)
> > > > Also, please assert that the frame indeed has mTopLayer ==
> > > > NS_STYLE_TOP_LAYER_NONE, so that we know this, as an absolute frame, is
> > > > guaranteed to be skipped in normal display list building.
> > > 
> > > I'm not sure where the place that you wish me to add the assertion is.
> > 
> > Probably just before you call into BuildDisplayListForTopLayerFrame.
> 
> I feel a little confuse here. I think we should assert that the frame has
> "mTopLayer != NS_STYLE_TOP_LAYER_NONE" not "mTopLayer ==
> NS_STYLE_TOP_LAYER_NONE", because the display list that we want to build by
> `BuildDisplayListForTopLayerFrame()` must has "-moz-top-layer: top". Or, did
> I misunderstand what you mean of the assertion?

Oh, I mean mTopLayer != NS_STYLE_TOP_LAYER_TOP. I'm not sure why I typed the other way... Sorry for causing confusion.
Flags: needinfo?(xidorn+moz)
I mean... mTopLayer == NS_STYLE_TOP_LAYER_TOP :/
(In reply to Mats Palmgren (:mats) from comment #11)
> Can we use the root widget instead for the event capturing?
> Can we use nsXULPopupManager code to initiate the rollup on

I have tried to use the root widget instead for the event capturing. The result is that we can roll the dropdown menu up by clicking, but we can roll it up when wheel-scrolling happens outside the dropdown frame. I think we trigger the roll-up behavior only when the wheel-scrolling event happens outside the widget that we specified[1], so using the root widget is not working because the event is always on it.

[1]: https://searchfox.org/mozilla-central/source/widget/cocoa/nsChildView.mm#4104

> the combobox frame somehow? (I think we always have one of those
> and it's a nsIRollupListener).

I didn't understand what this means. Could you explain more? Thanks!

(In reply to Neil Deakin from comment #13)
> Probably your best bet is to create something (probably a method added to
> nsIRollupListener) in the parent process which keeps track of the rect of
> the pretend dropdown and then change the callers of Rollup() (these are
> different on each platform) to be able to handle checking that instead of
> the popup widget's rect that they normally check. Then have
> nsIRollupListener::Rollup send a message down to the child to close the
> popup.
> 
> That would probably handle the events the best and would allow the
> interaction with other popups that might open/close to remain consistent.
> 
> But you may also want event capturing and the key/mouse grabs on Linux as
> well which would need some form of real native widget.

Did you mean that we should have an empty widget that has the same rect as the dropdown frame? If there is an empty widget on over the dropdown frame, can we still handle the event that should happen in the dropdown frame, such as choosing an option in the dropdown menu?
Flags: needinfo?(mats)
Flags: needinfo?(enndeakin)
There are two things:

1. The platform-specific stuff done by nsIWidget::CaptureRollupEvents to set up a listener for events outside the Firefox application while the popup is open. This allows the popup to hide when clicking or using the mousewheel or in response to other events. On gtk at least, a widget is needed, but you might be able to modify the code to use the main widget, but I haven't looked in detail. You'll want to do something here, otherwise clicking on another application might not hide the popup.

2. I believe the rollup code itself only needs the rect of the popup widget, so although the code currently gets the rect from the popup widget, it could be changed to just ask the current nsIRollupListener for the rect, which in turn would just use some cached value for select dropdowns and use the real widget for other popups. This would hopefully remove the need for the widget to be used directly from the rollup methods.
Flags: needinfo?(enndeakin)
Comment on attachment 8941749 [details]
Bug 1428701 - (Part 1) Build the dropdown frame as the top layer frame in the viewport frame

https://reviewboard.mozilla.org/r/211990/#review219536

Sorry for the delay. I didn't notice that you've updated the patch.

r=me with the nits addressed.

::: layout/generic/ViewportFrame.cpp:187
(Diff revision 2)
>  
>    nsIPresShell* shell = PresShell();
>    if (nsCanvasFrame* canvasFrame = shell->GetCanvasFrame()) {
> +    // Build display items for the dropped-down menu
> +    if (nsIFrame* dropdownFrame = canvasFrame->GetDropdownFrame()) {
> +      BuildDisplayListForTopLayerFrame(aBuilder, dropdownFrame, aList);

As before, please add an assertion here that
```c++
MOZ_ASSERT(dropdownFrame->StyleDisplay()->mTopLayer == NS_STYLE_TOP_LAYER_TOP);
```

::: layout/generic/nsCanvasFrame.cpp:260
(Diff revision 2)
> +  if (aDropdownFrame) {
> +    MOZ_ASSERT(aDropdownFrame->IsListControlFrame(),
> +               "Only nsListControlFrame can be dropped down.");
> +  }

```c++
MOZ_ASSERT_IF(aDropdownFrame, aDropdownFrame->IsListControlFrame(), ...);
```
Attachment #8941749 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8941750 [details]
Bug 1428701 - (Part 2) Handle the drop-down and the roll-up actions for content-select

https://reviewboard.mozilla.org/r/211992/#review219538

::: layout/forms/nsComboboxControlFrame.cpp:355
(Diff revision 2)
> +    // Update the dropdown frame in the canvas frame
> +    nsCanvasFrame* canvasFrame = do_QueryFrame(mDropdownFrame->GetParent());
> +    if (mDroppedDown) {
> +      canvasFrame->SetDropdownFrame(mDropdownFrame);
> +    } else {
> +      canvasFrame->SetDropdownFrame(nullptr);

You should probably also add this to `DestroyFrom` so that canvas frame wouldn't hold a dangling pointer.
These days, I have tried to use root widget to handle the roll-up listener. And I think it wouldn't work on e10s mode, because we can get the right root widget on e10s mode (We have to use the cocoa function, GetXULWindowWidget to get the window widget). I think we can only get the puppet widget on e10s mode, so we have to do some IPC to pass the event that comes from the parent process to the parent process to the right root widget. But I don't have much time to experiment it now.

Please feel free to take this bug and bug 1421229 (content-select) if anyone wants to work on this. My basic idea is on bug 1421229 and there is also a prototype patch. Because of the change of Mozilla organization, I think I can't be very active on this bug and all related bugs. If the people who take this bug have any question, feel free to ask me. I'll do my best to answer your questions! And if no people will work on this bug, I'll still work on this slowly when I have time.
Comment on attachment 8941750 [details]
Bug 1428701 - (Part 2) Handle the drop-down and the roll-up actions for content-select

https://reviewboard.mozilla.org/r/211992/#review220318

Cancelling this review for now until we have a better understanding of
how to handle events without a content process widget.
Attachment #8941750 - Flags: review?(mats)
Flags: needinfo?(mats)
Assignee: kuoe0.tw → nobody
Status: ASSIGNED → NEW
Assignee: nobody → emalysz
Comment on attachment 8989488 [details] [diff] [review]
Applied Above patches, handling dropdown when content-select enabled in TabParent/TabChild

This is a good start. Some stylistic comments:
> 
>+  nsBaseRollupListener* aListener;
>+

You'll want to make sure that this is cleared out when TabParent is destroyed.

I'd also suggest a better name than nsBaseRollupListener since it currently is specific to select elements such as SelectRollupListener.


>+mozilla::ipc::IPCResult
>+TabParent::RecvCaptureRollupEvents(const nsRect& aRect, const bool& aDroppedDown)
>+{
>+  nsresult rv;
>+  if (aDroppedDown){
>+    if (this->aListener){
>+      this->aListener->setRect(aRect);
>+    } else {
>+      nsBaseRollupListener* listener = new nsBaseRollupListener();
>+
>+      listener->setRect(aRect);
>+      this->setListener(listener);

Mozilla style is to put spaces before curly braces after closing parantheses, and to use initial capitals for C++ method names, and to have fields start with 'm', and arguments starts with 'a'.


+    nsRect rect = mDropdownFrame->GetRect();
     if (widget) {
-      widget->CaptureRollupEvents(this, false);
+      // widget->CaptureRollupEvents(this, false);
+      mozilla::dom::TabChild::GetFrom(aShell)->SendCaptureRollupEvents(rect, false);


You should just use an empty rectangle when setting to false since it won't be used. The TabParent could make sure to clear the rectangle when false is passed.


>+void
>+nsBaseRollupListener::NotifyGeometryChange()
>+{
>+  return;
>+}
>+
>+nsIWidget*
>+nsBaseRollupListener::GetRollupWidget()
>+{
>+  nsIWidget* widget = nullptr;
>+  return widget;
>+}
>+
>+void
>+nsBaseRollupListener::setRect(nsRect aRect)
>+{
>+  this->aRect = aRect;
>+}
>+
>+nsRect
>+nsBaseRollupListener::getRect()
>+{
>+  return aRect;
>+}

These simple implementations could just go in the header file.
Attachment #8991151 - Attachment is obsolete: true
Attachment #8991761 - Attachment is obsolete: true
Attachment #8989488 - Attachment is obsolete: true
Attachment #8992790 - Attachment is obsolete: true
Attachment #8992691 - Attachment is obsolete: true
Attachment #8992069 - Attachment is obsolete: true
Comment on attachment 8993537 [details] [diff] [review]
7/19 - updated code, still issues with ScreenEvent coordinate


>+
>+    /**
>+      * Checks for rollup commands
>+    */
>+    async CaptureRollupEvents(nsRect aRect, bool aDroppedDown);

You'll want separate functions for the child->parent and parent->child calls. CaptureRollupEvents is fine for the former but the latter should be something like RollupPopup.



>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>+
>+  this->mListener = nullptr;
>+  delete this->mListener;

You'll want to delete before setting it to null.


>+  if (aDroppedDown) {
>+    if (this->mListener) {
>+      this->mListener->SetRect(aRect);
>+    } else {
>+      SelectRollupListener* listener = new SelectRollupListener(aDroppedDown);
>+      listener->SetParent(this);
>+      listener->SetRect(aRect);
>+      this->SetListener(listener);
>+      listener->SetTopLevel(widget); 

You might want to just pass all of these in the SelectRollupListener constructor rather that using separate methods.

>+    }
>+    nsBaseWidget::SetActiveRollupListener(this->mListener);
>+    // widget->CaptureRollupEvents(this->mListener, true);
>+  } else {
>+    this->mListener = nullptr;
>+    nsBaseWidget::SetActiveRollupListener(this->mListener);
>+    // widget->CaptureRollupEvents(this->mListener, false); 
>+    delete this->mListener;
>+  }

Did we want to use CaptureRollupEvents in the end? This should ensure that we trap all mouse events.

>+
>+  NS_ENSURE_SUCCESS(rv, IPC_OK());
>+  return IPC_OK();
>+}

Since this always returns IPC_OK(), no need for the NS_ENSURE_SUCCESS line.

>diff --git a/layout/forms/nsComboboxControlFrame.h b/layout/forms/nsComboboxControlFrame.h
>   // static class data member for Bug 32920
>   // only one control can be focused at a time
>   static nsComboboxControlFrame* sFocused;
>+  nsIPresShell* aShell;
> 
You can get always get a PresShell for a frame using PresContext()->GetPresShell() so no need to store it here.


>   // fire a popup dom event if it is safe to do so
>   nsCOMPtr<nsIPresShell> shell = PresContext()->GetPresShell();
>   if (shell && nsContentUtils::IsSafeToRunScript()) {
>+    printf("do we do this?"); 
>     nsEventStatus status = nsEventStatus_eIgnore;
>     WidgetMouseEvent event(true, aShowPopup ? eXULPopupShowing : eXULPopupHiding,
>                            nullptr, WidgetMouseEvent::eReal);
> 
>     shell->HandleDOMEventWithTarget(mContent, &event, &status);
>   }

This is probably left over from long ago, so we probably don't need this event even in non-content-select mode. Maybe file a bug to remove it.

> }
> 
> bool
> nsComboboxControlFrame::ShowList(bool aShowList)
> {
>-
>-  // TODO(kuoe0) Remove this function when content-select is enabled.
>-  //
>-  // This function is used to handle the widget/view stuff, so we just return
>-  // when content-select is enabled. And the following callee, ShowPopup(), will
>-  // also be ignored, it is only used to show and hide the widget.
>+  // nsIWidget* widget = 
>   if (nsLayoutUtils::IsContentSelectEnabled()) {
>+    if (aShowList){
>+      nsRect rect = mDropdownFrame->GetRect();
>+      mozilla::dom::TabChild::GetFrom(aShell)->SetCombobox(this);
>+      mozilla::dom::TabChild::GetFrom(aShell)->SendCaptureRollupEvents(rect, true);
>+      // widget->CaptureRollupEvents();
>+    } else {
>+      nsRect emptyRect(0, 0, 0, 0);
>+      mozilla::dom::TabChild::GetFrom(aShell)->SetCombobox(this);
>+      mozilla::dom::TabChild::GetFrom(aShell)->SendCaptureRollupEvents(emptyRect, false);
>+    }

You want to set the combobox to null when hiding the list. Also, nsRect initializes to 0, so
you can just write:

SendCaptureRollupEvents(nsRect(), false)

>@@ -952,27 +999,27 @@ nsComboboxControlFrame::ShowDropDown(boo
>   if (!mDroppedDown && aDoDropDown) {
>     nsFocusManager* fm = nsFocusManager::GetFocusManager();
>     if (!fm || fm->GetFocusedElement() == GetContent()) {
>       DropDownPositionState state = AbsolutelyPositionDropDown();
>       if (state == eDropDownPositionFinal) {
>-        ShowList(aDoDropDown); // might destroy us
>+        ShowList(aDoDropDown);

Should keep this comment here and the next one a few lines below.

>-  if (ShowList(false))
>-    mListControlFrame->CaptureMouseEvents(false);
>+  if (ShowList(false)){

Space before '{'

> NS_IMETHODIMP
> nsComboboxControlFrame::RestoreState(PresState* aState)
> {
>   if (!aState) {
>     return NS_ERROR_FAILURE;
>   }
>-  ShowList(aState->droppedDown()); // might destroy us
>   return NS_OK;
> }

I assume we still want this line here. This can get called in certain cases if the page modifies the style on the page in such a way the the <select> frames get destroyed and re-created. This is called to reopen the popup if needed.



> 
> // Append a suffix so that the state key for the combobox is different
> // from the state key the list control uses to sometimes save the scroll
> // position for the same Element
> NS_IMETHODIMP
> nsComboboxControlFrame::GenerateStateKey(nsIContent* aContent,
>diff --git a/layout/forms/nsListControlFrame.cpp b/layout/forms/nsListControlFrame.cpp
>--- a/layout/forms/nsListControlFrame.cpp
>+++ b/layout/forms/nsListControlFrame.cpp
>@@ -176,16 +176,22 @@ nsListControlFrame::BuildDisplayList(nsD
>   // XXX why do we need this here? we should never reach this. Maybe
>   // because these can have widgets? Hmm
>   if (aBuilder->IsBackgroundOnly())
>     return;
> 
>   DO_GLOBAL_REFLOW_COUNT_DSP("nsListControlFrame");
> 
>   if (IsInDropDownMode()) {
>+    if (nsLayoutUtils::IsContentSelectEnabled() &&
>+      !mComboboxFrame->IsDroppedDown()) {
>+      // Don't build the display list when the list is not dropped down.
>+      return;
>+    }
>+
>     NS_ASSERTION(NS_GET_A(mLastDropdownBackstopColor) == 255,
>                  "need an opaque backstop color");
>     // XXX Because we have an opaque widget and we get called to paint with
>     // this frame as the root of a stacking context we need make sure to draw
>     // some opaque color over the whole widget. (Bug 511323)
>     aLists.BorderBackground()->AppendToBottom(
>       MakeDisplayItem<nsDisplaySolidColor>(aBuilder,
>         this, nsRect(aBuilder->ToReferenceFrame(this), GetSize()),
>@@ -567,17 +573,17 @@ nsListControlFrame::ReflowAsDropdown(nsP
>   // we're depending on?
>   nsHTMLScrollFrame::DidReflow(aPresContext, &state);
> 
>   // Now compute the block size we want to have.
>   // Note: no need to apply min/max constraints, since we have no such
>   // rules applied to the combobox dropdown.
> 
>   mDropdownCanGrow = false;
>-  if (visibleBSize <= 0 || blockSizeOfARow <= 0 || XRE_IsContentProcess()) {
>+  if (visibleBSize <= 0 || blockSizeOfARow <= 0) {
>     // Looks like we have no options.  Just size us to a single row
>     // block size.
>     state.SetComputedBSize(blockSizeOfARow);
>     mNumDisplayRows = 1;
>   } else {
>     nsComboboxControlFrame* combobox =
>       static_cast<nsComboboxControlFrame*>(mComboboxFrame);
>     LogicalPoint translation(wm);
>@@ -1831,16 +1837,19 @@ nsListControlFrame::MouseDown(dom::Event
>     CaptureMouseEvents(true);
>     AutoWeakFrame weakFrame(this);
>     bool change =
>       HandleListSelection(aMouseEvent, selectedIndex); // might destroy us
>     if (!weakFrame.IsAlive()) {
>       return NS_OK;
>     }
>     mChangesSinceDragStart = change;
>+  } else if (nsLayoutUtils::IsContentSelectEnabled()) {
>+    // We handle the drop-down and roll-up action in the combo box.
>+    return NS_OK;
>   } else {


The block of code after this is causing problems in content-select mode? We should figure out why, but I'll need to look at this code a bit more.


>+  nsIWidget* GetTopLevel()
>+    { return this->mWidget; }
>+
>+protected:
>+  nsRect mRect;
>+  mozilla::dom::TabParent* mParent; 
>+  bool mDroppedDown; 
>+  nsIWidget* mWidget; 

You can get the widget from the TabParent using GetTopLevelWidget() so no need to cache it here.

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm

>+        consumeEvent = (BOOL)selectRollupListener->Rollup(popupsToRollup, true, nullptr, nullptr);
>+      }
>+
>+    }
>+    selectRollupListener = nullptr;
>+    delete selectRollupListener; 

Don't delete this here. Instead delete it in TabParent during Rollup().

    
  

        Attachment #8993537 -
        Attachment is obsolete: true

    
  

        Attachment #8994697 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        7/31 -without Windows File (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #8997140 -
        Attachment is obsolete: true

    
  

        Attachment #8995680 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        8/3: Linux Build Failures (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #8997222 -
        Attachment is obsolete: true

    
  

        Attachment #8997517 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
  
Comment on attachment 8997960 [details]
Bug 1428701- (Part 3) Allow a SelectRollupListener to handle the roll-up actions

https://reviewboard.mozilla.org/r/261302/#review268946

::: dom/ipc/TabChild.h:48
(Diff revision 2)
>  #include "AudioChannelService.h"
>  #include "PuppetWidget.h"
>  #include "mozilla/layers/GeckoContentController.h"
>  #include "nsDeque.h"
>  #include "nsISHistoryListener.h"
> +#include "SelectRollupListener.h"

Don't think this is used in this file.

::: dom/ipc/TabChild.cpp:1328
(Diff revision 2)
>  mozilla::ipc::IPCResult
> +TabChild::RecvRollupPopup()
> +{
> +  //At this point the child will know that we need to close the popup
> +  //We will call rollup using the nsIRollupListener within nsCombobox
> +  this->mCombobox->Rollup(0, false, nullptr, nullptr);

No need to write 'this->'. Remove this in various places in the patch.

::: dom/ipc/TabChild.cpp:1333
(Diff revision 2)
> +  this->mCombobox->Rollup(0, false, nullptr, nullptr);
> +  return IPC_OK();
> +}
> +
> +void
> +TabChild::SetCombobox(nsComboboxControlFrame* aCombobox)

It might be better to store an nsIRollupListener rather than specifically a combobox.

::: dom/ipc/TabParent.cpp:403
(Diff revision 2)
>      ContentParent::NotifyTabDestroying(this->GetTabId(), Manager()->ChildID());
>    }
>  
>    mMarkedDestroying = true;
> +
> +  this->mListener = nullptr;

These lines need to be reversed.

::: dom/ipc/TabParent.cpp:2549
(Diff revision 2)
> +{
> +  nsCOMPtr<nsIWidget> widget = GetTopLevelWidget();
> +
> +  if (aDroppedDown) {
> +    if (this->mListener) {
> +      this->mListener->SetRect(aRect);

No need for 'this->' throughout this method.

::: dom/ipc/TabParent.cpp:2554
(Diff revision 2)
> +      this->mListener->SetRect(aRect);
> +    } else {
> +      SelectRollupListener* listener = new SelectRollupListener(this, aRect);
> +      this->SetListener(listener);
> +    }
> +    nsBaseWidget::SetActiveRollupListener(this->mListener);

I don't think you need to call SetActiveRollupListener as CaptureRollupEvents does this, no?

::: dom/ipc/TabParent.cpp:2559
(Diff revision 2)
> +    nsBaseWidget::SetActiveRollupListener(this->mListener);
> +    widget->CaptureRollupEvents(this->mListener, true);
> +  } else {
> +    delete this->mListener;
> +    this->mListener = nullptr;
> +    nsBaseWidget::SetActiveRollupListener(this->mListener);

Nor here.

::: dom/ipc/TabParent.cpp:2560
(Diff revision 2)
> +    widget->CaptureRollupEvents(this->mListener, true);
> +  } else {
> +    delete this->mListener;
> +    this->mListener = nullptr;
> +    nsBaseWidget::SetActiveRollupListener(this->mListener);
> +    widget->CaptureRollupEvents(this->mListener, false);

Probably clearer to pass nullptr instead of mListener.

::: layout/forms/nsComboboxControlFrame.cpp:316
(Diff revision 2)
>  nsComboboxControlFrame::ShowPopup(bool aShowPopup)
>  {
>    // TODO(kuoe0) Remove this function when content-select is enabled.
>  
> +  //this is where the canvasframe frame's dropdown should be set and cleared
> +  //when SetViewVisibility is called -- come to this function

The last phrase of this comment seems cut off or something. Also capitalize the first word.

::: layout/forms/nsComboboxControlFrame.cpp:321
(Diff revision 2)
> +  //when SetViewVisibility is called -- come to this function
> +
> +  if (nsLayoutUtils::IsContentSelectEnabled()) {
> +    nsCanvasFrame* canvasFrame = do_QueryFrame(mDropdownFrame->GetParent());
> +    if (aShowPopup){
> +      this->mDroppedDown = true;

No 'this->'

::: layout/forms/nsComboboxControlFrame.cpp:325
(Diff revision 2)
> +    if (aShowPopup){
> +      this->mDroppedDown = true;
> +      canvasFrame->SetDropdownFrame(mDropdownFrame);
> +    } else {
> +      PresShell()->FrameNeedsReflow(mDisplayFrame,
> +                                                 nsIPresShell::eStyleChange,

Fix thie indenting here calling FrameNeedsReflow

::: layout/forms/nsComboboxControlFrame.cpp:328
(Diff revision 2)
> +    } else {
> +      PresShell()->FrameNeedsReflow(mDisplayFrame,
> +                                                 nsIPresShell::eStyleChange,
> +                                                 NS_FRAME_IS_DIRTY);
> +      canvasFrame->SetDropdownFrame(nullptr);
> +      mDroppedDown = false;

Set mDroppedDown before SetDropdownFrame (so that it is in the same order as the other condition)

::: layout/forms/nsComboboxControlFrame.cpp
(Diff revision 2)
>    } else {
>      viewManager->SetViewVisibility(view, nsViewVisibility_kHide);
>      nsRect emptyRect(0, 0, 0, 0);
>      viewManager->ResizeView(view, emptyRect);
>    }
> -

We should leave this in. I suspect accessibility uses it, but we could investigate in another bug.

::: layout/forms/nsComboboxControlFrame.cpp:352
(Diff revision 2)
> -  //
> -  // This function is used to handle the widget/view stuff, so we just return
> -  // when content-select is enabled. And the following callee, ShowPopup(), will
> -  // also be ignored, it is only used to show and hide the widget.
>    if (nsLayoutUtils::IsContentSelectEnabled()) {
> +    if (aShowList){

Space between ) and {

::: layout/forms/nsComboboxControlFrame.cpp:355
(Diff revision 2)
> -  // also be ignored, it is only used to show and hide the widget.
>    if (nsLayoutUtils::IsContentSelectEnabled()) {
> +    if (aShowList){
> +      LayoutDeviceIntRect rect = LayoutDeviceIntRect::FromAppUnitsToNearest(mDropdownFrame->GetScreenRectInAppUnits(),
> +                                  mDropdownFrame->PresContext()->AppUnitsPerDevPixel());
> +      mozilla::dom::TabChild::GetFrom(PresContext()->GetPresShell())->SetCombobox(this);

You could get once mozilla::dom::TabChild::GetFrom(PresContext()->GetPresShell()) near the beginning of the method and reuse the value in both conditon blocks.

::: layout/forms/nsComboboxControlFrame.cpp:361
(Diff revision 2)
> +      mozilla::dom::TabChild::GetFrom(PresContext()->GetPresShell())->SendCaptureRollupEvents(rect, true);
> +    } else {
> +      LayoutDeviceIntRect emptyRect = LayoutDeviceIntRect::FromAppUnitsToNearest(nsRect(),
> +                                     mDropdownFrame->PresContext()->AppUnitsPerDevPixel());
> +      mozilla::dom::TabChild::GetFrom(PresContext()->GetPresShell())->SetCombobox(nullptr);
> +      mozilla::dom::TabChild::GetFrom(PresContext()->GetPresShell())->SendCaptureRollupEvents(emptyRect, false);

You can just pass LayoutDeviceIntRect() here.

::: layout/forms/nsComboboxControlFrame.cpp:613
(Diff revision 2)
>                                                    LogicalPoint* aTranslation)
>  {
> +  if (!nsLayoutUtils::IsContentSelectEnabled()) {
> +    // TODO(kuoe0) remove this assertion after content-select is enabled
> -  MOZ_ASSERT(!XRE_IsContentProcess());
> +     MOZ_ASSERT(!XRE_IsContentProcess());
> +  }

This is a reminder that we'll need to investigate and test with the preference on and off, with <select> elements in content and in the parent process.

::: layout/forms/nsComboboxControlFrame.cpp:734
(Diff revision 2)
> -  // if there is room.  If there is no room for it on either side then place
> -  // it after (to avoid overlapping UI like the URL bar).
> +  // if there is room. If there is no room for it on either side then place
> +  // it after (to avoid overlapping UI like the URL bar)
>    bool b = dropdownSize.BSize(wm)<= after || dropdownSize.BSize(wm) > before;
>    LogicalPoint dropdownPosition(wm, 0, b ? BSize(wm) : -dropdownSize.BSize(wm));
>  
> +  nsSize containerSize = GetSize();

Add a todo type comment here that we need to investigate when the select is in a fixed position container so we don't forget this.

::: layout/forms/nsComboboxControlFrame.cpp:745
(Diff revision 2)
> +  LogicalPoint dropdownPositionConverted(wm, pos, containerSize);
> +
> +
>    // Don't position the view unless the position changed since it might cause
>    // a call to NotifyGeometryChange() and an infinite loop here.
> -  nsSize containerSize = GetSize();
> +  // mDropdownFrame->SetRect(convertedRect, true);

Remove the commented out line.

::: layout/forms/nsComboboxControlFrame.cpp:1490
(Diff revision 2)
>      }
>    }
>  
> +  nsCanvasFrame* canvasFrame = do_QueryFrame(mDropdownFrame->GetParent());
> +  canvasFrame->SetDropdownFrame(nullptr);
> +

Do we need to clear the rollup listener here?

::: widget/SelectRollupListener.h:55
(Diff revision 2)
> +
> +  virtual uint32_t GetSubmenuWidgetChain(nsTArray<nsIWidget*> *aWidgetChain) override
> +    { return 0; }
> +
> +  virtual nsIWidget* GetRollupWidget() override
> +    { nsIWidget* widget = nullptr; return widget; }

Use some linebreaks here.

::: widget/SelectRollupListener.cpp:10
(Diff revision 2)
> +
> +#include "SelectRollupListener.h"
> +
> +SelectRollupListener::SelectRollupListener(mozilla::dom::TabParent* aParent, LayoutDeviceIntRect aRect)
> +{
> +  this->mParent = aParent;

Remove 'this->' throughout this file.

::: widget/cocoa/nsChildView.mm:59
(Diff revision 2)
>  #include "nsMenuBarX.h"
>  #include "NativeKeyBindings.h"
>  #include "ComplexTextInputPanel.h"
>  
>  #include "gfxContext.h"
> +#include "mozilla/dom/TabParent.h"

Unused I think.

::: widget/cocoa/nsChildView.mm:4102
(Diff revision 2)
> +
> +  return false;
> +}
> +
> +// //also pass in the event
> +- (BOOL)HandleListener:(NSEvent*)theEvent : (nsIRollupListener*)aRollupListener

This method should have a more specific name that describes what it does.

::: widget/cocoa/nsChildView.mm:4105
(Diff revision 2)
> +
> +// //also pass in the event
> +- (BOOL)HandleListener:(NSEvent*)theEvent : (nsIRollupListener*)aRollupListener
> +{
> +  nsCOMPtr<nsIWidget> rollupWidget = aRollupListener->GetRollupWidget();
> +  if ([self IsSelectListener:aRollupListener]) {

You should be able to just check (!rollupWidget) Then I think isSelectListener isn't even needed.

::: widget/cocoa/nsChildView.mm:4107
(Diff revision 2)
> +- (BOOL)HandleListener:(NSEvent*)theEvent : (nsIRollupListener*)aRollupListener
> +{
> +  nsCOMPtr<nsIWidget> rollupWidget = aRollupListener->GetRollupWidget();
> +  if ([self IsSelectListener:aRollupListener]) {
> +    SelectRollupListener* selectRollupListener = (SelectRollupListener*) aRollupListener;
> +    if (selectRollupListener){

Space between ) and {

::: widget/cocoa/nsChildView.mm:4130
(Diff revision 2)
>  
>    nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener();
>    NS_ENSURE_TRUE(rollupListener, false);
> -  nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget();
> -  if (rollupWidget) {
> -    NSWindow* currentPopup = static_cast<NSWindow*>(rollupWidget->GetNativeData(NS_NATIVE_WINDOW));
> +  if (rollupListener) {
> +    if (![self HandleListener:theEvent :rollupListener]) {
> +      if ([self IsSelectListener:rollupListener]){

This section of code seems unused.

::: widget/cocoa/nsCocoaUtils.mm:145
(Diff revision 2)
> +{
> +  //we need to convert both the event and rect to the same coordinate space
> +
> +  NSRect cocoaRect = DevPixelsToCocoaPoints(aRect, aScaleFactor);
> +
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;

This should go at the beginning of the function.

::: widget/cocoa/nsCocoaUtils.mm:148
(Diff revision 2)
> +  NSRect cocoaRect = DevPixelsToCocoaPoints(aRect, aScaleFactor);
> +
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
> +
> +  float xMax = cocoaRect.origin.x + cocoaRect.size.width;
> +  float newY = MenuBarScreenHeight() - cocoaRect.origin.y;

I know the y coordinates go upwards on Mac, but I think it would be clearer math if the y position and height were just used as is and compared.

::: widget/cocoa/nsCocoaUtils.mm:151
(Diff revision 2)
> +
> +  float xMax = cocoaRect.origin.x + cocoaRect.size.width;
> +  float newY = MenuBarScreenHeight() - cocoaRect.origin.y;
> +  float yMax = newY - cocoaRect.size.height;
> +
> +  if ((ScreenLocationForEvent(anEvent).y <= newY && ScreenLocationForEvent(anEvent).y >= yMax) &&

Just call ScreenLocationForEvent once and store the result.

::: widget/gtk/nsWindow.cpp:5256
(Diff revision 2)
>  }
>  
> +static bool
> +is_mouse_in_rect (LayoutDeviceIntRect aRect, gdouble aMouseX, gdouble aMouseY)
> +{
> +  gint xMax = aRect.x + aRect.width;

Use aRect.XMost() and aRect.YMost() instead and just use them directly in the conditon. Then you can just use one line, as in:

return (aMouseX >= ...)

::: widget/gtk/nsWindow.cpp:5281
(Diff revision 2)
> +
> +  return false;
> +}
> +
> +static bool
> +handle_listener(nsIRollupListener* aRollupListener, gdouble aMouseX, gdouble aMouseY)

Better name here as with Mac.

::: widget/gtk/nsWindow.cpp:5284
(Diff revision 2)
> +
> +static bool
> +handle_listener(nsIRollupListener* aRollupListener, gdouble aMouseX, gdouble aMouseY)
> +{
> +  nsCOMPtr<nsIWidget> rollupWidget = aRollupListener->GetRollupWidget();
> +  if (is_select_listener(aRollupListener)) {

Similar comment here as for Mac. Just compare !rollupWidget

::: widget/gtk/nsWindow.cpp:5307
(Diff revision 2)
> -    bool retVal = false;
> +     bool retVal = false;
> -    auto *currentPopup =
> -        (GdkWindow *)rollupWidget->GetNativeData(NS_NATIVE_WINDOW);
> -    if (aAlwaysRollup || !is_mouse_in_window(currentPopup, aMouseX, aMouseY)) {
> +    if (rollupListener) {
> +      if (aAlwaysRollup || !handle_listener(rollupListener, aMouseX, aMouseY)) {
> +        if (is_select_listener(rollupListener)) {
> +          SelectRollupListener* rollupListener = (SelectRollupListener*) rollupListener;
> +        }

This doesn't do anything.

::: widget/nsBaseWidget.cpp:1820
(Diff revision 2)
>  
>    return nsXULPopupManager::GetInstance();
>  }
>  
>  void
> +nsBaseWidget::SetActiveRollupListener(nsIRollupListener* aRollupListener)

This may not be needed.

::: widget/windows/nsWindow.cpp:7894
(Diff revision 2)
>  
> +bool
> +nsWindow::EventIsInsideRect(LayoutDeviceIntRect rollupRect){
> +
> +  //Screenlocation for event:
> +  DWORD pos = ::GetMessagePos(); 

There are a bunch of spaces at the ends of lines in this method.

::: widget/windows/nsWindow.cpp:7899
(Diff revision 2)
> +  DWORD pos = ::GetMessagePos(); 
> +  POINT mp;
> +  mp.x = GET_X_LPARAM(pos);
> +  mp.y = GET_Y_LPARAM(pos);
> +
> +  float xMax = rollupRect.x + rollupRect.width; 

Use XMost and YMost here.

::: widget/windows/nsWindow.cpp:7927
(Diff revision 2)
> +  }
> +}
> +
> +//static
> +bool
> +nsWindow::HandleRollup(nsIRollupListener* aRollupListener) {

Better name here as on other platforms.

::: widget/windows/nsWindow.cpp:7928
(Diff revision 2)
> +}
> +
> +//static
> +bool
> +nsWindow::HandleRollup(nsIRollupListener* aRollupListener) {
> +  if (!IsSelectRollupListener(aRollupListener)) {

Just use if (rollupWidget)

::: widget/windows/nsWindow.cpp:7944
(Diff revision 2)
> +  }
> +  return false; 
> +}
> +
> +bool
> +nsWindow::EventIsInsideRect(LayoutDeviceIntRect rollupRect, LayoutDeviceIntRect buttonRect){

I think this function is leftover and no longer needed.

::: widget/windows/nsWindow.cpp
(Diff revision 2)
>  
> +  bool consumeEvent = false;
>    static bool sSendingNCACTIVATE = false;
>    static bool sPendingNCACTIVATE = false;
>    uint32_t popupsToRollup = UINT32_MAX;
> -

Don't remove the blank line here.

::: widget/windows/nsWindow.cpp:8102
(Diff revision 2)
>        {
>          WinPointerEvents pointerEvents;
>          if (!pointerEvents.ShouldRollupOnPointerEvent(nativeMessage, aWParam)) {
>            return false;
>          }
> -        if (!GetPopupsToRollup(rollupListener, &popupsToRollup)) {
> +        if (!IsSelectRollupListener(rollupListener)) {

We'll need to fix this to handle the select popup somehow.

::: widget/windows/nsWindow.cpp:8280
(Diff revision 2)
>      nsIContent* lastRollup;
>      consumeRollupEvent =
>        rollupListener->Rollup(popupsToRollup, true, &pos, &lastRollup);
>      nsAutoRollup::SetLastRollup(lastRollup);
>    } else {
> +    if (rollupListener) {

rollupListener was null-checked near the beginning of the method.

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8997613 -
        Attachment is obsolete: true

    
    

    
  
Remaining tasks: fix error within Windows file when dropdown is shown and user clicks out, have someone from accessibility test it, create test cases for layout featuers

    
    

    
  
Reviewer suggestions for the platform-specific pieces:

Windows: aklotz or jimm
macOS: mstange or spohl
GTK: karlt (I wouldn't actually ask karlt to review, but ask him who should review, since I believe karlt is backing off of GTK reviews these days)

    
    

    
  


  

        Attachment #8941749 -
        Attachment is obsolete: true

        Attachment #8941750 -
        Attachment is obsolete: true

        Attachment #8997960 -
        Attachment is obsolete: true

    
    

    
  
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.


Assignee: emalysz → nobody

    
  
Severity: normal → S3

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: