134837 - BeOS PopUp window implementation is suspicious

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Sergei Dolgov]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:0.9.9+) Gecko/20020328
BuildID:    

It seems that implementation of PopUp-s in BeOS as derivative of BWindow, not
BView wasn't the best solution. Windows are too "independent" in BeOS. PopUp-s
stay on place, when "parent" window is moved, and don't close when mouse click
appears not only outside "parent" window, but even if on "parent" window title
tab. In some cases it cause Mozilla crash.

Reproducible: Always
Steps to Reproduce:
1.Open some PopUp, in Munu bar, by right-clicking on page or by filling URL in
autocomplete mode. Now, immediatelly grab yellow title bar and try to move main
window.
2.Crash: Open two windows, e.g. Navigator+Composer.
3.Open PopUp in one of windows.
4. Close parent window of PopUp via clicking on close button on TitleBar.
5. Now try to open some PopUp in remaining window.
6. In some builds ther isn't any need to close window to fool mozilla with PopUp
pointer - just open two windows and click menu bar on both without closing PopUp
in another window.

Actual Results:  Step 1 - PopUp stays fixed - Main window is moved.
Steps 5 and 6 - Mozilla crashes.

Expected Results:  Step 1 - move PopUp together with parent window.
Step 5 - Open Popup
Step 6 - Close PopUp in inactive window and open in activated window

In theory, such widgets as PopUp, if implemented as BWindow derivative, should
use B_MODAL_*_FEEL. But 1) there was discussion already about modal windows in
BeOS Mozilla - bug  # 66809  2)If PopUp is created in MODAL feel in current
BeOS-Mozilla version - it causes problems with focus/accessibility in
combo-boxes, like URL bar+autocomplete drop-down-list - after first match is
found, whole application including URL input field goes inacessible).
As notice - native BeOS PopUp-s (PopUp menus) are implemented as BView derivative.

Comment 1

[Sergei Dolgov]

Related bugs:
http://bugzilla.mozilla.org/show_bug.cgi?id=58217
http://bugzilla.mozilla.org/show_bug.cgi?id=67305
http://bugzilla.mozilla.org/show_bug.cgi?id=66809
http://bugzilla.mozilla.org/show_bug.cgi?id=128753

Comment 2

[Boris Zbarsky [:bzbarsky]]

Over to a more reasonable component.

timeless, is there someone in particular who should be cced on BeOS issues like
this one?

Comment 3

[timeless]

.

Comment 4

[Paul]

What do you mean?  Popup-windows or Popup-menus?  If you are only talking about 
popup-menus, I believe they are implemented as views in mozilla.  In fact, I
would bet on it.  If you are talking about popup-windows, then, it must be
a derivitive of a BWindow.  (See BAlert, which is a beos native Popup-window).
Popup-windows need to be movable, resisable (maybe), etc, which you cannot get
using a BView, without writing your own entire Window Class, which would also
not be controlled by the app_server, which is yet another bad thing.

I agree, we need to find a solution, but, one has not been found yet.

There was talk by Daniel Switkin, that he might, once he gets a chance,
rewrite a lot of nswindow.cpp code.  That code is VERY ugly in places, and 
also, very old.  If Daniel does not have the time to work on it, someone else
definitely should.  (I'm not volunteering :) ).

Comment 5

[Sergei Dolgov]

there are two types of  auxiliary items in Mozilla,Paul - eWindowType_dialog and
eWindowType_popup. About first type i'm agree that it should be BWindow.
And in fact, you may loose your bet about menus - AFAIK it is call to create
object which will be used further in all menus and even drop-down lists: 
if (mWindowType == eWindowType_popup) {
<SKIP>
w = new nsWindowBeOS(this, winrect, "", B_NO_BORDER_WINDOW_LOOK, feel,         
                   B_NOT_CLOSABLE | B_AVOID_FOCUS | B_ASYNCHRONOUS_CONTROLS    
                              | B_NO_WORKSPACE_ACTIVATION | B_OUTLINE_RESIZE);
And more, first phrase in first link on bug i provided here above, explains it:
http://bugzilla.mozilla.org/show_bug.cgi?id=58217 :
"Until now, menus, popups, dialogs, tooltips didn't appear on BeOS port. 
That is because nsWindow creates only BView when it needed to create a new 
window(BWindow)"
Sorry for reposting.

What about solutions - temporary workaround may be fix for RollUp function, but
i didn't have success with it yet.

Comment 6

[Paul]

Well, according to the BeBook, a BView must be attached to a BWindow.  Now, just because
a BMenu/BPopupMenu extends from a BView, does not mean that underneath, it does not 
create a borderless window to attach itself too.  Which, I'm guessing, since I don't have
code for the BMenu to look at, it must do, in order to draw outside of the window.

Really, no items are derivitives of BWindows in mozilla.  If you
look, a view is always created first, and then if a window is needed to encapsulate that
view, one is created, and the view is added to it.  That is why nsWindow keeps track
of only the BView object, and not the BWindow.

Granted, I might have been a little confused by your question at first, especially since
I have not looked at the code in a while, so hopefully I've explained myself better now.

Now, as for a fix, I don't think the rollup function is the problem.  Instead, I think 
we need to fire off a rollup events on window moved/closed/resized in nsWindowBeOS, plus,
fire off the events when clicking outside the window.

I haven't tried this, but, if a popup is created, I think the gRollupListener by be
instantiated.  So, check if it is not null, and if so, do a gRollupListener->RollUp().

Comment 7

[Paul]

marking dependency

Comment 8

[Paul]

Setting the priority, so I can better organize the issues in bugzilla I need to
take care of
marking assigned.  will be looking into this again VERY soon (need to clean
up/fix some things in my patch before I'm ready to post it)

Comment 9

[Paul]

Attachment: all inclusive patch - should fix popup issue, plus others

All inclusive patch:
- should fix popup windows bug
- should fix "empty gray window" bug#187286
- adds new mouse cursors
- should fix focus issue bug#190562
- should fix modal subset windows bug#66809

Comment 10

[Paul]

setting blockers, since the patch here should fix those

Comment 11

[Paul]

Comment on attachment 126427 [details] [diff] [review]
all inclusive patch - should fix popup issue, plus others

Sergei, can you check this, to make sure I didn't screw anything up?

Comment 12

[Sergei Dolgov]

Probably i cannot review, until i reach my sweet home, but who knows. (Week or
bit more)
Anyway, one notice. Some of focus and activation problems are dependent on
Makoto's hack in nsAppShell.cpp where he was introduced event priority hack.
This hack breaks logical event order in some cases and makes things messy - for
our problem i mean sequence of clicks, actiation and window creation.
So some of code i proposed for focus fix is, in reality, workaround for that
hack results.
Will see.

Comment 13

[Sergei Dolgov]

first notice on that huge patch:)
 Fire Popup-menu with right click, then click on yellow title tab - menu don't
close, only main window goes active/foreground...maybe it is good, maybe not.

Comment 14

[Paul]

Sergei, I'll look into the "AppServer msg priority hack", and see if removing
that changes anything.

Also, I'll look into the popup "issue" you described.  I did some quick checks,
and on a window close with a child "popup", the popup is closed.  It should also
be closed, however, on a focus change, like you describe.

Thanks for looking into this, as I know you are not in front of you primary
development box.

Comment 15

[Sergei Dolgov]

Hello, Paul.
I think that also good ideas are adding check for coordinate/dimension changes
in :Move(x,y) and :Resize(width,height) - to avoid activity in case of unchanged
parameters, as it is done in other ports, and also implementation of
:Resize(x,y,width,height) via two previous methods calls, to avoid code
duplication and inheriting automatically all code changed.
Also maybe my name must be in contributors list too, as i forget it to put there
with previous patches

Comment 16

[Paul]

Attachment: updated patch

- Cleaned up Move/Resize, as per Sergei's suggestion
- Removed priority queueing in nsAppShell, except for syncronous messages
  (i don't know if it is me, but this seems to make mozilla more responsive)
- Fixed a long standing bug, where a right-click to brin up a popup menu, would
require a second click to select on the menu.  Now, right click, highlight
item, release, and the item is selected, as it should be

Sergei, as for your comment about popups not dissappearing when clicking on the
title bar, well, we don't seem to get an event for that, and so I can't check
for it.  I don't think it is a big deal, as if you do anything else, it is
handled properly.

Comment 17

[Paul]

Comment on attachment 126603 [details] [diff] [review]
updated patch

Sergei, I know you are away, but, since you were able to review my previous
patch, and, there really is no one else to review the patch, could you please
review this updated one as well?

Comment 18

[Sergei Dolgov]

Just arrived at home from abroud voyage, though, too late here. Downloading 1.4
release sources now.
Will try that patch tomorrow with those source, i hope, but cannot guarantee fo
sure - only if build will be successful and fast.
As i'm very familiar with most changes in nsWindow.cpp, i think only changes in
nsAppShell need more careful look and testing.

Comment 19

[Sergei Dolgov]

Well, Paul, i tried new patch, and unfortunately, get same sad results as with
my own experiments with changes in nsAppShell - scrolling on low-end machines
goes irritating (for me at least).
What i mean, that previously lagged rather scroll-knob/slider than view itself.
Now i have feeling that scrolling queue or stack collects all events and lag
appears for both - knob and view itself. E.g if you intensively drag slider up
and down several times and then release it, view will perform all that movements.
Before those patches, your and my local too, it rather ignored that "tremor" and
scrolled immediately to last position. While on fast machines such behaviour may
be not so ned and even bring feeling of more softer scrolling, on low-end
machines this is really awful.
Difference of my unpublished patch for nsAppShell (amongst other, irrelevant for
our case though), that i tried to save ConsumeRedundantMouseMoves Method in
code, but it didn't seem to have effect on that problem, rather caused
occasional crashes.
So i'm in doubts:(

Comment 20

[Paul]

The changes to nsAppShell were not meant to increase the speed of anything, I
just thought it seemed a little quicker/more repsonsive on my box, after I made
the change.  Plus, even though I would love to get mozilla running better on
low-end hardware under BeOS, I think there are plenty of other things to worry
about, and with only 2 primary developers for the BeOS port, trying to do too
many things at once will lead us no where.  
The changes were made to nsAppShell to help ensure that generated events are
handled in the order in which they are recieved.  I believe you had mentioned
that some of the work arounds in nsWindow are do to the fact that nsAppShell
does not handle events in their proper order, because of the "priority" event
queues, which I removed.  I don't know the full reasoning behind having the
priority queues in the first place, and the more I thought about it, the more it
seemed like a bad idea anyway.  
Either way, I plan on completely re-writing nsAppShell at some point in the not
too distant future.  In order to get the IPC code under BeOS working, the
BApplication needs to be running in the same thread as nsAppShell, or, at least
be able to communicate with it.  I feel it would be better to have nsAppShell
extend from BApplication, and have all messageing to/from nsAppShell handled by
BMessages instead of ports.  I need to do some more research before that change,
and since IPC has not gone "live" yet, it can wait until I get more of my other
bugs in my list resolved.

Now, does the patch fix the problems it is supposed to have fixed?  I have tried
reproducing the bugs that this bug blocks, to see if fixes those problems, and I
have not been able to reproduce those problems with this patch on my machine. 
Also, there are probably a couple other nsWindow related bugs that may be fixed
by this patch, due to focussing issues, etc.

Comment 21

[Sergei Dolgov]

I understand what you did, but i don't understand why you removed
ConsumeRedundantMouseEvent, if you still have there priority levels array.

I consider new behaviour as significant regression, and arrogance about
experience on low-level machines isn't best way to address numerous beos-port
performance issues:(

If we have these two levels of priority, we still can put these separate case for 
::ONMOUSE, with ConsumeRedundantMouseEvent, AddNewEventItem and break, and try
if it helps.

If you disagree, really prefer to address nsAppShell problems in another bug, as
you proposed yourself.

Comment 22

[Paul]

First, it is NOT arrogance, it is prioritizing!!!  And right now, my priority is
not to make mozilla run on old hardware under beos.  That would be nice to do,
yes, but, I would like to remain focussed on the patch at hand, and what it is
supposed to deal with.  This patch is not meant to fix anything to do with
scrolling issues or with making things run better on old hardware!

Priority levels were removed.  They do not exist after this patch, except for
syncronous messages, as they should be dealt with ASAP, so that whatever called
the code, can return ASAP.

I removed the ConsumeRedundantMouseEvents for really no particular reason, and I
can put it back very easily.  Does keeping it around really affect anything? 
IMO, no.  Have I noticed any major differences with or without that method, no.

The whole purpose of modifying nsAppShell in this patch for one reason, and one
reason only:  to ensure that events are handled in the order that they are
generated.

Comment 23

[Paul]

Attachment: same patch, but removed nsAppshell changes

Ok, I removed the nsAppShell changes, as per Sergei's request.
Sergei: You had mentioned to me problems with mouse event's being "sticky".  I
cannot reproduce the problem here.  If you can try again, with a clean
widget/src/beos directory, I would appreciate it.

getting this checked in should close a couple other currently open bugs

Comment 24

[Sergei Dolgov]

Comment on attachment 130739 [details] [diff] [review]
same patch, but removed nsAppshell changes

r=sergei@fi,tartu.ee
I put review here, because it addresses some issues, like focus/activation
problems. In order to allow further work. 
But i think that we still have far from perfect implementation,
hope it may be improved with less generic patches in future.
(Destroy is still imperfect, i still dislike pop-ups behaviour - wondering why
you put patch in this bug:), etc.)
With short testing i didn't get mouse/focus sticking, but if it appears, i can
fix it fast.

Comment 25

[Sergei Dolgov]

I got mouse bug. Though, it is hard to reproduce.
Had several tabs opened, scrolled, refreshed pages, switched between tabs,
and noticed, that when i move mouse up and down in tab i switched recently to,
it scrolls, inspite mouse button is released.
Will ask people to help me test it/found proper way to reproduce

Comment 26

[Paul]

Agreed that we are far from perfect, but, we are much closer :-)

I have checked this in, marking fixed.

Sergei, I will try and go through BugZilla a bit, and close off some other bugs,
that may have been fixed by this patch.  If you could do the same, or at least
verify for me, the ones that I do close, I would greatly appreciate it.

Comment 27

[Benjamin Smedberg]

This was checked in without approval, and sr=arougthopher. Does this patch fall
under the special exception for platform-specific code?

Comment 28

[Paul]

sr=arougthopher is me.  I am the module owner for this code http://www.mozilla.org/owners.html.I guess I added the "s" accidentally, sorry.