Closed Bug 339447 Opened 16 years ago Closed 15 years ago

Scrollbars in popup windows appear inactive (Cocoa)

Categories

(Core :: Widget: Cocoa, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: mark, Assigned: cbarrett)

References

Details

(Whiteboard: [blocking1.9a1+])

Attachments

(1 file, 1 obsolete file)

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.
See also bug 340585.
*** Bug 354758 has been marked as a duplicate of this bug. ***
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.
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.
*** Bug 356467 has been marked as a duplicate of this bug. ***
Flags: blocking1.9+
Whiteboard: [blocking1.9a1+]
Assignee: joshmoz → stanshebs
I have a fix!
Assignee: stanshebs → hwaara
Attached patch Partial fix (obsolete) — Splinter Review
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).
Attachment #245568 - Flags: review?(joshmoz)
Cool, I'm going to give it a try.
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?
(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)?
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 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?
(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.
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.
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.
Don't we have the same issue with context menus in cocoa builds?
(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.
(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.
"Yeah" as a reply to "Maybe you're thinking of bug 356528".
(In reply to comment #20)
> "Yeah" as a reply to "Maybe you're thinking of bug 356528".

Ah, that's good news.
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.
Attachment #245568 - Flags: review?(joshmoz)
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.
Attached patch WIP patch v2Splinter Review
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).
Attachment #245568 - Attachment is obsolete: true
*** Bug 363030 has been marked as a duplicate of this bug. ***
It turns out the bug I saw in comment 24 was not due to my changes. It's was already filed as bug 355631. 
Blocks: 300710
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.
Depends on: 355631
fixed on trunk by 370439
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee: hwaara → cbarrett
You need to log in before you can comment on or make changes to this bug.