392040 - SELECT menu requires 2 clicks to show up in some pages

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[[e-mail]]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a8pre) Gecko/2007081304 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a8pre) Gecko/2007081304 Minefield/3.0a8pre ID:2007081304

Menu created with SELECT some times requires clicking at it twice to show the menu items.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.us.playstation.com/Search
2. Click at the REFINE YOUR SEARCH menu
Actual Results:  
The menu gets the hilite ring but not menu items show up. Clicking outside of the menu then the menu again pops up the menu items.

Expected Results:  
Menu items pop up with the 1st click.

This problem occurs in some pages, not all. Also, after the 2nd click, the menu works normally until the page is reloaded.

Comment 1

[Shawn Wilsher :sdwilsh]

I'm seeing this in the most recent hourly.  This only happens after page load, and subsequent tries work as expected.  It should be noted that click the arrows works the first time, but clicking the text does not.

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

It actually takes _three_ clicks on the <select>s at http://litmus.mozilla.org/manage_testcases.cgi

Comment 3

[Josh Aas]

We could really use a reduced testcase here.

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #1)
> I'm seeing this in the most recent hourly.  This only happens after page load,
> and subsequent tries work as expected.  It should be noted that click the
> arrows works the first time, but clicking the text does not.

No I'm seeing the click on the arrow fail as well.

More fun, the select does appear to be opening just not painting. If I press and hold on the arrow, no display, then move the mouse down to where the popup would be and release and a new value gets selected. This is why it takes 3 clicks, click once opens invisibly, click again closes, click again to open and be visible. Oddly though if you single click you can't select a new value but clicking where the popup would be doesn't cause a click on anything that happens to be beneath there.

Can't get a definite way to reproduce but interestingly when I do see it on a page it affects every select on the page for the first open.

Comment 5

[Dave Townsend [:mossop]]

Just found reliable STR.

1. Load a bugzilla bug and wait for the load to complete.
2. Change text zoom.

Now all the select elements will fail to open on first attempt. Change text zoom again and they break.

Turns out I see this a lot when loading bugs in a background tab, when I switch to the tab the site-specific prefs realises I like a different text zoom on bmo and changes it thus breaking the selects.

Comment 7

[Håkan Waara]

Attachment: Proposed patch

I've been debugging this today, and think I found the bug. I don't know if this is the correct fix, but time (or more correctly, Boris :-) ) will tell.

Here's the deal:

1. When you click a popup, nsComboboxControlFrame::ShowPopup is called, which will tell the combobox's view to either show or hide.

2. When we do page zoom, nsPresContext::SetFullZoom restyles the world, and views are updated. Eventually in this flow, nsContainerFrame::SyncFrameViewProperties is called, which assumes that all views should display by default (unless this is a menuPopupFrame).

3. So in the listboxControlFrame case (which is our combobox), the view is getting shown, by setting its visibility to nsViewVisibility_kShow, but the widget isn't showing.

4. When you later click the combobox, we get to ShowPopup() as usual (as in #1), but this time the call to show the view returns early, because the view is already marked as showing...

Summary: So the bug is that the view is "shown", while it isn't showing, when the page zoom restyling is happening, so we get into an inconsistent state. My fix is to handle the listcontrolFrame the same as we currently do with menuPopupFrame, so we don't get into an inconsistent default state for the combobox's view.

Comment 8

[Håkan Waara]

... and of course the nsComboboxControlFrame.cpp part of the diff is leftover that you can ignore.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  Seems like the right general idea, I guess.  roc would know better.  You only want this for a listbox inside a combobox, though.

Comment 10

[Josh Aas]

I'm curious about why this doesn't happen on Windows and Linux given Håkan's explanation.

Comment 11

[Håkan Waara]

I should clarify that this patch fixes both of these different testcases:

* The popupmenu at http://www.us.playstation.com/Search (comment 0)
* Page zooming on a bugzilla bug and then using any popupmenu (comment 5)

Comment 12

[Håkan Waara]

Attachment: Proposed patch (without debug leftover)

Comment 13

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

Comment on attachment 289255 [details] [diff] [review]
Proposed patch (without debug leftover)

I think this is correct. Thanks for finding it!

Comment 14

[Boris Zbarsky [:bzbarsky]]

I assume you carefully tested non-combobox listboxes?

Comment 15

[Håkan Waara]

I will do some testing of non-combobox listboxes tonight, and then check in if nothing pops up. 

Comment 16

[Boris Zbarsky [:bzbarsky]]

In particular, test fixed-pos listboxes, so that they have widgets?

Comment 17

[Håkan Waara]

Select multiple-listboxes seems to work fine, both with fixed position, without, and after page zoom. 

Landed on trunk.

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007112016 Minefield/3.0b2pre on both:

1) http://litmus.mozilla.org -and-
2) http://www.clorox.com/products/ (Cleaning Advisor on the right)

Thanks for the fix, hwaara; keep them coming!

Comment 19

[Boris Zbarsky [:bzbarsky]]

I backed this patch out to fix bug 404872.  Please make sure to check in a testcase for that bug when you reland this patch.

Comment 20

[Håkan Waara]

Attachment: Fix v2

OK, here's a better fix. 

It only applies the logic to popupmenu-selects. I needed to expose a useful method (to be able to in a simple way to tell a listbox-select from a popupmenu-select) from nsListControlFrame through nsIListControlFrame.

Martijn, wanna test it?

Comment 21

[Håkan Waara]

Attachment: My testcase

Here's a simple testcase for selects that I've played around with when testing my fix.

If everyone is happy with the patch, I'll look into creating reftests for the regression and for this bug, if possible.

Comment 22

[Martijn Wargers (dead)]

(In reply to comment #20)
> Martijn, wanna test it?

The patch seems to work fine here on windows.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 292308 [details] [diff] [review]
Fix v2

>Index: layout/forms/nsListControlFrame.h
>+  virtual PRBool IsInDropDownMode() const;

I guess we don't call this enough to really affect performance by this change, right?

>Index: layout/generic/nsContainerFrame.cpp
>+    // Find out if we're a menupopup, because then we don't 

s/if/whether/

>+    // button has been clicked.

What button?

>+    PRBool isMenuPopup = aFrame->GetType() == nsGkAtoms::menuPopupFrame;
...

Can we make all this an inline function instead?  That way we can avoid doing this stuff unless we really need the information (which we don't always).

And use a local for the type, so you don't GetType() twice?

>+    // We're a menupopup if we're a <select> too, unless it's a <select multiple>.

This is false.  One can have a listbox without "multiple".  Just say that we're a menupopup if we're the list control frame dropdown for a combobox.

Comment 24

[Håkan Waara]

Attachment: Fix v3

* Created a static method for the check
* Remembered to rev the IID for nsIListControlFrame :-)

Not sure what a reasonable way to measure perf impact for this is, if that is really necessary.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 292323 [details] [diff] [review]
Fix v3

Didn't I ask for an inline method, not a static one?  There's exactly one caller, so no reason not to inline it...

Comment 26

[Håkan Waara]

Attachment: Fix v4

Sorry - patch with inline method attached.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 292325 [details] [diff] [review]
Fix v4

+  if (aFrame->GetType() == nsGkAtoms::listControlFrame) {

frameType?

Comment 28

[Håkan Waara]

(In reply to comment #27)
> (From update of attachment 292325 [details] [diff] [review])
> +  if (aFrame->GetType() == nsGkAtoms::listControlFrame) {
> 
> frameType?

D'oh, yes. Fixed.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 292325 [details] [diff] [review]
Fix v4

With that fixed, and curlies around the one-line if body, r=bzbarsky

Comment 30

[Håkan Waara]

(In reply to comment #29)
> (From update of attachment 292325 [details] [diff] [review])
> With that fixed, and curlies around the one-line if body, r=bzbarsky

Boris, would you be fine with r+sr-ing this, or do you think we need a separate super-reviewer?

I'm now working on testcases for this bug and bug 404872. It's taking a while because it's my first real encounter with the new testcase world.

Comment 31

[Håkan Waara]

Attachment: Mochitest for bug 404872

Here's a testcase for bug 404872. 

I'm not sure how to make a testcase for this bug... How do I trigger full page zoom using only JS/DOM, and how do I then verify that the menu is being shown as requested?

Comment 32

[Boris Zbarsky [:bzbarsky]]

I think it would be a good idea for roc to double-check this patch.

> How do I trigger full page zoom using only JS/DOM

I think there are existing mochitests that do this using UniversalXPConnect privs and the document viewer zoom API...

> how do I then verify that the menu is being shown as requested

This is the hard part.  I'm not sure we have a way to do it.  :(

Comment 33

[Colin Barrett [:cbarrett]]

If the menu is shown, it will take key events, right?

Maybe send a key event and see if the menu takes it from the document?

Comment 34

[Håkan Waara]

(In reply to comment #33)
> If the menu is shown, it will take key events, right?
> 
> Maybe send a key event and see if the menu takes it from the document?

Key events are unfortunately already picked up, after the click, even if the menu does not show up. This is because after the first click, the select is focused.

Comment 35

[Stephen Donner [:stephend] Not actively reading bugmail]

I'm hoping that you marked this as INVALID as a mistake...

Comment 36

[Colin Barrett [:cbarrett]]

What about having a link on the page where the menu is supposed to show up, and then send the click event to a particular point? So, if the menu fails to show up, the click goes to the link? The seems a bit fragile though.

Comment 37

[Håkan Waara]

(In reply to comment #35)
> I'm hoping that you marked this as INVALID as a mistake...

Thanks for catching that Stephend, I shouldn't play with *these* popupmenus when testing. ;-)

(In reply to comment #36)
> What about having a link on the page where the menu is supposed to show up, and
> then send the click event to a particular point? So, if the menu fails to show
> up, the click goes to the link? The seems a bit fragile though.

Hmm. I guess I can put an onclick handler on one of the <option>s or something like that, and then trigger a synthesized mouse click. That might work!

Comment 38

[Martijn Wargers (dead)]

Attachment: Maybe useful

Maybe this code is useful to get a working mochitest for?
I don't have a mac myself, but from the description of the bug and the steps to reproduce, it seems that at the time this bug wasn't fixed, you don't see a select popup opening up with this testcase after 500ms. You need to download the testcase to your computer because of the use of enhanced privileges.

Comment 39

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

Wny not just #include nsListControlFrame.h instead of extending nsIListControlFrame, and static_cast instead of doing a QueryInterface?

+inline PRBool
+IsMenuPopup(nsIFrame *aFrame)

just make it "static". I respectfully disagree with Boris:
> Didn't I ask for an inline method, not a static one?  There's exactly one
> caller, so no reason not to inline it...
A good compiler, or even gcc, will inline a static method if it's only used once, and by making it "static" instead of "inline" it will *stop* inlining if we add another call site somewhere.

+  if (aFrame->GetType() == nsGkAtoms::listControlFrame) {

This should be using frameType.

Otherwise looks good.

Comment 41

[Håkan Waara]

Attachment: Final patch

Here's the final patch with roc's comments addressed. I'll check this in and the mochitest for bug 404872 while I keep on trying to create a testcase for this bug.

Comment 42

[Håkan Waara]

Attachment: Better final patch

Better final patch :-). Also had to change a makefile in order to be able to include nsListControlFrame directly.

Comment 43

[Håkan Waara]

Fixed, and checked in with a testcase for bug 404872.

Comment 44

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified with:

1) Testcase from comment 0
2) Testcase from comment 2
3) Testcase from comment 5, and, finally
4) Testcase from bug 404872

using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011804 Minefield/3.0b3pre

(I trust that this is enough this time; if not, please do reopen, and I'll let others do the final honors.)