SELECT menu requires 2 clicks to show up in some pages

VERIFIED FIXED

Status

()

Core
Layout
P4
major
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: stephen@ju-ju.com, Assigned: Håkan Waara)

Tracking

Trunk
PowerPC
Mac OS X
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite ?
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Updated

11 years ago
Version: unspecified → Trunk
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
It actually takes _three_ clicks on the <select>s at http://litmus.mozilla.org/manage_testcases.cgi
Flags: blocking1.9?

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+

Comment 3

10 years ago
We could really use a reduced testcase here.
(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.

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.

Updated

10 years ago
Target Milestone: --- → mozilla1.9 M10

Updated

10 years ago
Priority: -- → P5

Updated

10 years ago
Priority: P5 → P4
Target Milestone: mozilla1.9 M10 → ---
(Assignee)

Updated

10 years ago
Duplicate of this bug: 403282
(Assignee)

Comment 7

10 years ago
Created attachment 289251 [details] [diff] [review]
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.
(Assignee)

Comment 8

10 years ago
... and of course the nsComboboxControlFrame.cpp part of the diff is leftover that you can ignore.
Hmm.  Seems like the right general idea, I guess.  roc would know better.  You only want this for a listbox inside a combobox, though.
(Assignee)

Updated

10 years ago
Assignee: joshmoz → hwaara

Comment 10

10 years ago
I'm curious about why this doesn't happen on Windows and Linux given Håkan's explanation.
(Assignee)

Comment 11

10 years ago
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)
(Assignee)

Comment 12

10 years ago
Created attachment 289255 [details] [diff] [review]
Proposed patch (without debug leftover)
Attachment #289251 - Attachment is obsolete: true
Comment on attachment 289255 [details] [diff] [review]
Proposed patch (without debug leftover)

I think this is correct. Thanks for finding it!
Attachment #289255 - Flags: superreview+
Attachment #289255 - Flags: review+
I assume you carefully tested non-combobox listboxes?

Updated

10 years ago
Assignee: hwaara → nobody
Component: Widget: Cocoa → Layout
QA Contact: cocoa → layout

Updated

10 years ago
Assignee: nobody → hwaara
(Assignee)

Comment 15

10 years ago
I will do some testing of non-combobox listboxes tonight, and then check in if nothing pops up. 
In particular, test fixed-pos listboxes, so that they have widgets?
(Assignee)

Comment 17

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

Landed on trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?

Updated

10 years ago
Depends on: 404872
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

10 years ago
Created attachment 292308 [details] [diff] [review]
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?
Attachment #289255 - Attachment is obsolete: true
Attachment #292308 - Flags: review?(bzbarsky)
(Assignee)

Comment 21

10 years ago
Created attachment 292309 [details]
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.
(In reply to comment #20)
> Martijn, wanna test it?

The patch seems to work fine here on windows.
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.
Attachment #292308 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 24

10 years ago
Created attachment 292323 [details] [diff] [review]
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.
Attachment #292308 - Attachment is obsolete: true
Attachment #292323 - Flags: review?(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...
(Assignee)

Comment 26

10 years ago
Created attachment 292325 [details] [diff] [review]
Fix v4

Sorry - patch with inline method attached.
Attachment #292323 - Attachment is obsolete: true
Attachment #292325 - Flags: review?(bzbarsky)
Attachment #292323 - Flags: review?(bzbarsky)
Comment on attachment 292325 [details] [diff] [review]
Fix v4

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

frameType?
(Assignee)

Comment 28

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

D'oh, yes. Fixed.
Comment on attachment 292325 [details] [diff] [review]
Fix v4

With that fixed, and curlies around the one-line if body, r=bzbarsky
Attachment #292325 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

10 years ago
Attachment #292325 - Flags: superreview?(roc)
(Assignee)

Comment 30

10 years ago
(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.
(Assignee)

Comment 31

10 years ago
Created attachment 293303 [details]
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?
Attachment #292309 - Attachment is obsolete: true
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.  :(
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?
(Assignee)

Comment 34

10 years ago
(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.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → INVALID
I'm hoping that you marked this as INVALID as a mistake...
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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.
(Assignee)

Comment 37

10 years ago
(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!
Created attachment 293579 [details]
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.
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.
Duplicate of this bug: 409158
(Assignee)

Comment 41

10 years ago
Created attachment 294094 [details] [diff] [review]
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.
Attachment #292325 - Attachment is obsolete: true
Attachment #292325 - Flags: superreview?(roc)
(Assignee)

Updated

10 years ago
Attachment #294094 - Attachment is obsolete: true
(Assignee)

Comment 42

10 years ago
Created attachment 294107 [details] [diff] [review]
Better final patch

Better final patch :-). Also had to change a makefile in order to be able to include nsListControlFrame directly.
Attachment #294107 - Flags: superreview?(roc)
Attachment #294107 - Flags: review+
Attachment #294107 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 43

10 years ago
Fixed, and checked in with a testcase for bug 404872.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
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.)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.