281732 - <select> popups stopped working properly

Status: RESOLVED FIXED

(Camino Graveyard :: HTML Form Controls, defect)

Description

[Avi Drissman]

Nasty regression. With the 2005020908 nightly, popups don't work.

1. Go to the URL listed.
2. Click the popup.
3. Select the second item.
> Result: The item is not changed.
4. Click in the white space below the control.
> The control pops open.

And other brokenness ensues.

Works fine in 2005020808, broken in 2005020908.

This is not an accusation, but the only checkin that comes to mind in this time
frame is the one for bug 281470...

Comment 1

[Mike Pinkerton (not reading bugmail)]

simon, any thoughts?

Comment 2

[Mina]

Confirming using NB 2005020908 (v0.8+)
Mac OS X 10.3.8 (7U16)

Comment 3

[Elliot Roth]

I confirm the same problem.

Comment 4

[Simon Fraser [no longer active]]

I noticed this too (popups in Bugzilla and BLT are affected). Maybe some code
was relying on the broken WidgetToScreen()?

Comment 5

[Simon Fraser [no longer active]]

Backing out the WidgetToScreen() changes fixes this, so someone must be
depending on the backasswards coordinates.

Comment 6

[Hiro]

*** Bug 282066 has been marked as a duplicate of this bug. ***

Comment 7

[Simon Fraser [no longer active]]

I think this fix exposed a bunch of bugs in the list control frame, which,
previously were not being revealed because
nsListControlFrame::IsClickingInCombobox() always returned false. So, with this
fix, IsClickingInCombobox() returns the correct value, and we end up with mouse
capture being turned on etc.

I may need some help here from someone more familiar with the native form controls.

Comment 8

[Jasper]

I might also add to this bug that when dragging an image from a page and
releasing it before you exit the window the "return to original place" rectangle
snaps back to the oposite location. You can see it when you drag the mozilla
image at the top of this page.

Comment 9

[Simon Fraser [no longer active]]

With the flipped screen Y coordinates,
nsListControlFrame::IsClickingInCombobox() _always_ returned PR_FALSE because
the rect returned from mComboboxFrame->GetAbsoluteRect() assumed top-relative Y
coords.

Comment 10

[Simon Fraser [no longer active]]

Attachment: #ifdef out mouse capturing and dropdown showing for cocoa widgets

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 174250 [details] [diff] [review]
#ifdef out mouse capturing and dropdown showing for cocoa widgets

This isn't quite working yet.

Comment 12

[Simon Fraser [no longer active]]

Attachment: #ifdef out list control popup code, and fix nsCocoaWindow

This patch adds more #ifdefs to the form control code (yay!) to make sure that
we never turn on mouse capture for <select> frames, and it fixes nsCocoaWindow
creation so that the window type gets set even when the nsCocoaWindow is
created with a non-null parent. This is necessary in order to get the window
type set to eWindowType_popup, which is examined elsewhere in the code to
determine if a window is a popup.

Comment 13

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

You know what would be slightly nicer than all these #ifdefs? How about a method
in nsComboboxControlFrame "PRBool IsCocoaNativeWidget()" which is inline and
#ifdefed to just return 0 or 1. Then in the main code we just call that with
"if". This way we can ensure that we don't break compilation across platforms.

Comment 14

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

Comment on attachment 174270 [details] [diff] [review]
#ifdef out list control popup code, and fix nsCocoaWindow

minusing since I'd like that change. Other than that the patch is fine

Comment 15

[Bruce Davidson]

In testing the patch works fine. Tested on camino trees both before and after
the optgroup change (bug 149648) landed.

Comment 16

[Simon Fraser [no longer active]]

Attachment: Updated version of the layout part of the patch

I added  static PRBool ToolkitHasNativePopup(); to nsComboboxControlFrame,
after considering a few alternatives, none of which were simple or ideal.

I did some other minor tidyup as well.

Comment 17

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

Comment on attachment 174356 [details] [diff] [review]
Updated version of the layout part of the patch

Make ToolkitHasNativePopup() inline in nsComboboxControlFrame.h. That will let
the compiler remove unnecessary code.

Comment 18

[Simon Fraser [no longer active]]

> Make ToolkitHasNativePopup() inline in nsComboboxControlFrame.h. That will let
> the compiler remove unnecessary code.

I wanted to avoid the #ifdef in the header, and to permit the method to be
expanded in the future to maybe call into nsITheme or something.

Comment 19

[Simon Fraser [no longer active]]

Comment on attachment 174356 [details] [diff] [review]
Updated version of the layout part of the patch

Requesting 1.8 approval, since the patch touches layout code (but only affects
camino).

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 174356 [details] [diff] [review]
Updated version of the layout part of the patch

We want this for the Aviary branch too, for Camino 0.83

Comment 21

[Simon Fraser [no longer active]]

Attachment: nsCocoaWindow patch for the 1.7 branch

Comment 22

[Monica]

I have found that pressing the ENTER key after selecting the menu item from a
dropdown menu works. The menu is no longer selected and you can move on to
another form field.

HTH

Comment 23

[Jasper]

*** Bug 282518 has been marked as a duplicate of this bug. ***

Comment 24

[Mike Pinkerton (not reading bugmail)]

why are we removing the popup check on the branch only?

Comment 25

[Simon Fraser [no longer active]]

The nsCocoaWindow change went into the trunk too (but it was just the layout
part of the patch that needed re-review).

Comment 26

[Simon Fraser [no longer active]]

*** Bug 282636 has been marked as a duplicate of this bug. ***

Comment 27

[Simon Fraser [no longer active]]

This is now fixed.

Comment 28

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

Comment on attachment 174356 [details] [diff] [review]
Updated version of the layout part of the patch

approval1.7.7+ and approval-aviary1.0.3+ per discussion with Asa, caillon. 
Please land on MOZILLA_1_7_BRANCH and AVIARY_1_0_20050124_BRANCH .

Comment 29

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

Comment on attachment 174447 [details] [diff] [review]
nsCocoaWindow patch for the 1.7 branch

approval1.7.7+ and approval-aviary1.0.3+ per discussion with Asa, caillon. 
Please land on MOZILLA_1_7_BRANCH and AVIARY_1_0_20050124_BRANCH .

Comment 30

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

Sorry, second branch name was wrong.  That should have been MOZILLA_1_7_BRANCH
and AVIARY_1_0_1_20050124_BRANCH .

Comment 31

[Mike Pinkerton (not reading bugmail)]

Attachment: actual branch layout patch

the layout patch didn't apply on the branch, here's what actually landed on 17
and aviary.

Comment 32

[Mike Pinkerton (not reading bugmail)]

landed on 17branch and aviary. there is no "fixed1.7.7" keyword.

Comment 33

[Frank Wein [:mcsmurf]]

Now we got the keyword ;).