81416 - [FIX]Comboboxes will not roll up without special calls by embedding

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[rods (gone)]

We just fixed where combobox do not roll up when winembed was minimized, but 
there are still several problems for combos:

1) When Winembed window is moved with dropdown down
2) Menus are dropped down with the combo dropped down

The embedding app will need to call a special method anytime one of these 
actions happen to make sure everything gets rolled up correctly. This is because 
these type of messages do not make it to our message pump in such a way that we 
can identify them and do the right thing.

Kevin has pointed out that once we get rid of the native scrollbars on the 
dropdown or go to XBL (if XBL uses mouse capture) then it shouldn't be an issue 
anymore.

Comment 1

[rods (gone)]

Actually, this should be assigned to me. 

Comment 2

[Judson Valeski]

it's not reasonable to ask an embedding app to "call a special method" to roll 
up drop down lists. we need to solve the problem w/ events not being handled 
properly. this should all be handled internally.

Comment 3

[rods (gone)]

No events are generated for us to process when menus drop down. 
There is no really good way of knowing when the window is moved.

So this means our options are:
1) XBL (too far off)
2) Use Gfx scrollbars (been there, tried that, too many problems)
3) Resurrect the old native code (nearly impossible)

Wow, and today started out so well.

Comment 4

[Kevin McCluskey (gone)]

"No events are generated for us to process when menus drop down."

Just to clarify, we only receive events that are targetted to native windows
managed by nsIWidget instances. Mozilla rolls up, because it uses nsIWidget
instances which use the mozilla/widget/src/windows/nsWindow.cpp::WindowProc on
WIN32 for processing events. Embedding apps which manage their own windows
recieve all of the events for their windows directly. We don't have any chance
to process messages (window moved, resized, menu dropdown) which would lead to
rolling up the dropdown.

The native combo box initiates event capture when the dropdown is popped-down.
We can not initiate event capture because we use a combination of a gfx rendered
listbox and a native scrollbar. If we initiate event capture we can not scroll
the dropdown list using the native scrollbar because it is a separate window and
we will get the event that was targetted to the scrollbar window and rollup
instead of allowing the scrollbar to process the event. (A year+ ago. several
engineering days were spent trying to overcome this problem by forwarding events
to the scrollbar but without success.)  Native combo-boxes do not create a
separate window for the native scrollbar so they do have this problem. 

We also attempted to remove the native scrollbar used for the combobox by
replacing it with a gfx-scrollbar. In theory this could solve the problem if we
also initiated mouse capture for the gfx rendered combobox + scrollbar
combination. (6 months or so ago) at least 1-2 weeks of engineering time was
spent trying to convert the combobox to use gfx rendered scrollbars without success.

We abandoned all development on this issue in favor of waiting for an XBL
implementation of comboboxes which could also solve the problem by initiating
mouse capture on the dropdown. (This work was supended because higher priority
issues took precedence. No work is being done on this).

So were left with having to find a temporary solution. The simpliest but ugly
solution would be to pass the requirement back to the embedding app for now and
have them either call a method to popup the dropdown list or have them forward
the messages passed to their WindowProc's to a method on the embedding API's so
we can determine if the WIN32 message should result in rolling up the dropdown.

Comment 5

[Judson Valeski]

cc'ing conrad, I believe he had some thoughts on this topic.

so what "events" should cause a combo box roll up?

I wonder if hooking this rollup callback into the focus api implementation would
cover it??? the embedding app is already calling focus movement methods, perhaps
we just piggy back those.

we're obviously receiving click events (we're able to click links for example),
could/should we call rollupComboBox() internally when those clicks are made?

Comment 6

[Kevin McCluskey (gone)]

"I wonder if hooking this rollup callback into the focus api implementation
would cover it???"

That might do it. As long as the embedding app notifies us of all focus changes.

The embedding app would need to notify us of focus changes in these cases: 
* user clicks in the desktop (embedding app loses focus) 
* user clicks in the embedding app's chrome (menubar/toolbar etc.) (the
embedding app needs to explicitly remove focus from embedded content area)
* user clicks and holds down on the embedding app's window title bar (The
embedding app needs to explicitly remove focus from the embedded content area)



(inMsg == WM_ACTIVATE || inMsg == WM_NCLBUTTONDOWN || inMsg == WM_LBUTTONDOWN ||
      inMsg == WM_RBUTTONDOWN || inMsg == WM_MBUTTONDOWN || 
      inMsg == WM_NCMBUTTONDOWN || inMsg == WM_NCRBUTTONDOWN || inMsg ==
WM_MOUSEACTIVATE ||
      inMsg == WM_MOUSEWHEEL || inMsg == uMSH_MOUSEWHEEL || inMsg == WM_ACTIVATEAPP)

Comment 7

[Kevin McCluskey (gone)]

The previous snippet of code covers all of the cases where we currently rollup.
Basically button down, mouse wheel, and window activate's cause us to rollup.

Comment 8

[Judson Valeski]

I believe all of those focus events are currently handled. If one/some of them
are not, we could make it a requirement that if you want drop down roll-up, you
have to treat focus this way. IMO, we should go this route as it's more self
contained.

we would need to hook into nsIWebBrowserFocus in here
http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWebBrowser.cpp#1353

Comment 9

[Conrad Carlen (not reading bugmail)]

In looking at this on the Mac, the problem is less severe. The only thing which
causes roll up not to happen when it should is minimizing the window
(window-shading in Mac parlance). Possibly the top-level widget keeps track of
windows which need to be rolled up, and then, on idling, checks whether they
need to be rolled up. CC'ing Pink since he probably knows how this works. The
reason I bring this up is that, whatever the Mac widget code is doing, it's very
close to right and maybe the idea could be used XP.

Comment 10

[Mike Pinkerton (not reading bugmail)]

you give the mac widget code far too much credit ;) It's not doing anything so
evolved. 

As with every other platform, when we see a click, check if there is a popup and
roll it up. It's probable that since mac doesn't have multiple OS-native
'windows', that gecko always gets a crack at the event. Other OS's don't exhibit
this behavior because we never see clicks in other 'windows' (that's how the os
works). 

The not rolling up on a 'windowshade' is a known bug, but nothing we can do much
about since we don't ever get notified of the minimization (unless we move to
carbon)...though your 'check on idle' idea might help that, but god does it make
me feel icky ;)

Comment 11

[rods (gone)]

Attachment: patch for this bug and Bug 79037

Comment 12

[Judson Valeski]

I don't know this code well enough to review it, but if it solves our roll-up
woes; great! it seems to be contained in our window impl which makes me smile :).

Comment 13

[Kevin McCluskey (gone)]

Rod: I Just tried patch 35469. 

It rolled up the dropdown in MfcEmbed:
- When clicking and moving the window
- Trying to resize the window.

Which is great!

But, it didn't rollup the window when:
- When clicking on a menu item in MfcEmbed. 
- When clicking on the toolbar in MfcEmbed

Comment 14

[rods (gone)]

Attachment: Better patch for this bug (and Bug 79037)

Comment 15

[Phil Peterson]

Rod, are you getting reviews for this patch?

Comment 16

[Kevin McCluskey (gone)]

I applied the patch on WINNT and ran both mfcembed and mozilla. It closed the 
combobox dropdown on mfcembed when moving or sizing the window, clicking on the 
toolbar, clicking in the desktop, and clicking on a menu item.  

r=[e-mail]

Comment 17

[David Hyatt]

sr=hyatt, although I must admit that I don't understand any of this code.  Your 
Windows fu is superior. :)

Comment 18

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1

Comment 19

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1

Comment 20

[rods (gone)]

fixed.

Comment 21

[:deanis74]

Ouch, a WH_CALLWNDPROC hook?  I just remember some 16-bit code I had that hooked 
into another app.  Even though I returned immediately out of the CallWndProc() 
function, adding strings seemed to call the wndproc.  So it took about 10 
seconds to add 150 strings one-by-one to a combobox.

However, I notice that between the 16 and 32-bit API documentation, they removed 
the line that reads "The WH_CALLWNDPROC hook affects system performance. It is 
supplied for debugging purposes only."  So it may well be that this isn't an 
issue anymore.

Comment 22

[Kevin McCluskey (gone)]

The hook is registered only when the dropdown is popped down for the current UI
thread. It is un-registered when the dropdown is popped up, so it shouldn't
cause any general performance problems since the next action by the user within
the thread will be to either make a selection or do something which will cause
the dropdown to rollup. Both actions will result in the removal of the hook.

Comment 23

[:deanis74]

OK, cool.  As an aside, does anyone know if there are still performance hits 
using a WH_CALLWNDPROC in Win32?

Comment 24

[rods (gone)]

I haven't seen anything.

Comment 25

[John Morrison]

May God have mercy on us all. The 212 bug spam-o-rama is Now!