339447 - Scrollbars in popup windows appear inactive (Cocoa)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Mark Mentovai]

Cocoa version of bug 339370.  Scrollbars in popup windows have an inactive appearance.  Testcase: autocomplete dropdown.  Popups are only brought forward, they never get key/main status in Cocoa widgets.

Comment 1

[Mark Mentovai]

See also bug 340585.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 3

[Håkan Waara]

I patched my tree a bit, and investigated this. Here are some findings:

* All our child windows are by default set to NSBorderlessWindowMask in nsCocoaWindow::StandardCreate(). According to the NSWindow docs, that makes a window unable to get key (unless you subclass and return YES for canBecomeKeyWindow), so our mozWindow friends need to do that.

* Child windows that are not sheets currently only get a orderFront:nil call in nsCocoaWindow::Show. After you use the correct key-windowable subclass for these windows, you also need to use makeKeyAndOrderFront:nil.

With those two steps, things look right.  Unfortunately, the main window loses its active appearance. I haven't been able to fix that. I see that there are some related things (stuff like setSuppressMakeKeyFront:) that I don't understand good enough, but they probably would need to change as well.

Comment 4

[Håkan Waara]

Let me revise my first bullet point there:

Actually, this may be the tipping point for us to have to subclass NSWindow. I just realized that we need to instantiate the key-focusable class (instead of NSWindow w/ a borderless mask) when allocating the window; this can't be done using a protocol.

I created a simple FocusableWindow subclass (w/ canBecomeKey returning YES) and used that, in my testing.

Comment 5

[Josh Aas]

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

Comment 6

[Håkan Waara]

I have a fix!

Comment 7

[Håkan Waara]

Attachment: Partial fix

This makes popupmenus work just like normal in forms (e.g, bugzilla), but not the autocomplete popup (yet). 

The fix is quite interesting... The requirements were something like this: We need a way to create a new popup window that 

* will receive key events, 
* its widgets look focused, 
* but without the window getting key (getting focused). Because if the window actually got key status for real, then the main browser window will look unfocused, which we don't want.

The fix is to concoct a special window subclass that can 

1) receive key events without being key (NSPanels have this feature, it turns out) and 
2) fool its children views that they are in the key window even when they're not (we override |isKeyWindow| for that).

Comment 8

[Stan Shebs]

Cool, I'm going to give it a try.

Comment 9

[Stan Shebs]

The patch works great for me. I must be a noob or something, because I've been playing with the autocomplete popup and it seems to work completely as expected - what's still not right with it?

Comment 10

[Håkan Waara]

(In reply to comment #9)
> The patch works great for me. I must be a noob or something, because I've been
> playing with the autocomplete popup and it seems to work completely as expected
> - what's still not right with it?

Are the items selected when you mouse over them? Can you scroll in it with your mousewheel (if there's a scrollbar)?

Comment 11

[Stan Shebs]

You're right, for the autocomplete popup, highlighting doesn't happen, and the scrollwheel makes list items move up and down but the scrollbar doesn't update its position.

Comment 12

[Josh Aas]

Comment on attachment 245568 [details] [diff] [review]
Partial fix

+      case eWindowType_popup:
+        break;
+      case eWindowType_invisible:
         break;

The break for the popup type is not necessary.

+    if (mWindowType == eWindowType_popup || mWindowType == eWindowType_child)

I don't think we need eWindowType_child in there. That is for child views iirc. If you get rid of that then you can put the setOpacityValue: call up with the other popup window stuff like setLevel:.

+    } else if (mWindowType == eWindowType_invisible) {

Please leave the style the way it was, we don't want the else on the same line with the closing bracket.

I'm not sure we want to do this until we figure out what is going on with the autocomplete popup. However, it would be nice to have this significant improvement in ASAP for Gecko 1.9 alpha. Are you making any progress with autocomplete?

Comment 13

[Håkan Waara]

(In reply to comment #12)
> (From update of attachment 245568 [details] [diff] [review] [edit])
> +      case eWindowType_popup:
> +        break;
> +      case eWindowType_invisible:
>          break;
> 
> The break for the popup type is not necessary.

It is to separate the invisible window from the two constants above. See one line above:

+      // these two will get the NSNonactivatingPanelMask upon creation.

> 
> +    if (mWindowType == eWindowType_popup || mWindowType == eWindowType_child)
> 
> I don't think we need eWindowType_child in there. That is for child views iirc.
> If you get rid of that then you can put the setOpacityValue: call up with the
> other popup window stuff like setLevel:.

Ok, I'll remote it, and add an assertion that will fire if that window type would ever be used.

> 
> +    } else if (mWindowType == eWindowType_invisible) {
> 
> Please leave the style the way it was, we don't want the else on the same line
> with the closing bracket.
> 
> I'm not sure we want to do this until we figure out what is going on with the
> autocomplete popup. However, it would be nice to have this significant
> improvement in ASAP for Gecko 1.9 alpha. Are you making any progress with
> autocomplete?

Haven't worked on this lately, I'll see if I can spend some time on it in the coming days.

Comment 14

[Håkan Waara]

I have a patch now that makes the scrollbar in the autocomplete popup appear active as well, however the items themselves (in autocomplete) don't hover when mousing over them. 

Debugging, I see that the window is receiving mouse-in/mouse-out and mouse-moved events. 

Does someone understand the autocomplete popupmenus well enough to give me a clue why the items aren't highlighted?   One of my theories is that it's because the window doesn't have key, but it should be enough for gecko that it receives mouse and key events, right?   

Please comment any ideas you guys have, thanks.

Comment 15

[Håkan Waara]

Sorry, to clarify: the menuitems in the autocomplete menus don't *highlight* when mousing over them.

Scrollbars are now active, and the scrollwheel even works on them! So we're pretty close.

Comment 16

[Asaf Romano (gone)]

Don't we have the same issue with context menus in cocoa builds?

Comment 17

[Håkan Waara]

(In reply to comment #16)
> Don't we have the same issue with context menus in cocoa builds?

No, do you? :)   Maybe you're thinking of bug 356528 that I fixed a few days ago.

Comment 18

[Asaf Romano (gone)]

Yeah :-/

Comment 19

[Håkan Waara]

(In reply to comment #18)
> Yeah :-/

If you really mean in contextmenus that are brought up on right-click, I haven't seen that. Wanna file a bug?   If you mean in popupmenus like bugzilla and in autocomplete, that's what this bug is about.

Comment 20

[Asaf Romano (gone)]

"Yeah" as a reply to "Maybe you're thinking of bug 356528".

Comment 21

[Håkan Waara]

(In reply to comment #20)
> "Yeah" as a reply to "Maybe you're thinking of bug 356528".

Ah, that's good news.

Comment 22

[Josh Aas]

Comment on attachment 245568 [details] [diff] [review]
Partial fix

Clearing review on "Partial  fix" since it seems like there is another patch on the way.

Comment 23

[Håkan Waara]

Status update:  Still debugging a fix for this (in the evenings, when I have time) but it's a very intricate bug; it doesn't make it easier that it's a mix of focus and popup handling, both of which are broken in their own ways.

Comment 24

[Håkan Waara]

Attachment: WIP patch v2

To keep you updated, this is what I have right now. It's a slimmed-down version of the previous patch (some nice refactoring can be done in nsCocoaWindow::StandardCreate if we go with this patch, but that cleanup is not included in this patch).

It fixes scrollbars to work (with scrolling etc) everywhere, but the autocomplete popups don't work correctly - not when hovering (no selection of the row you're mousing over) nor when clicking (the click is not dispatched correctly).

Comment 25

[Josh Aas]

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

Comment 26

[Håkan Waara]

It turns out the bug I saw in comment 24 was not due to my changes. It's was already filed as bug 355631. 

Comment 27

[Håkan Waara]

Ok, I think I found the fix for why autocomplete items are not selected, and clicking in them don't work. I will post that to bug 355631 and after that I will hopefully be able to post a final patch here.

Comment 28

[Josh Aas]

fixed on trunk by 370439