Closed Bug 278655 Opened 20 years ago Closed 20 years ago

Scrollbars sometimes jump scrolling Java applet page (in Java 1.4.X)

Categories

(Core Graveyard :: Widget: Mac, defect)

Product:
Core Graveyard
Component:
Widget: Mac
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: smichaud, Assigned: smichaud)

References

(
URL
)

Details

Attachments

(1 file, 8 obsolete files)

Fix for Mozilla 1.8a6 (includes best ideas so far)
22.51 KB, patch
mikepinkerton
: review+
sfraser_bugs
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a6) Gecko/20050111
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a6) Gecko/20050111

When using the Java Embedding Plugin, the scrollbar (vertical or
horizontal) will sometimes jump to a new location after you've dragged
it somewhere with the mouse.  This happens in any browser page once
you've loaded an applet into it (so it will continue to happen in the
same page even after you go in it to another URL that contains no
applets).  The easiest way to demonstrate the problem is to drag the
scrollbar all the way to the top, bottom, left or right and let go.
But the problem sometimes also happens in the middle of the
scrollbar's range.  Usually you can then drag the scrollbar back from
where it jumped to, and this time it will stay put.

This only happens in Mozilla 1.8a6 and recent Mozilla "trunk" and
Firefox nightlies.  The problem appeared in the Mozilla "trunk"
nightlies somewhere between 2004-12-06-06-trunk (which doesn't have
the problem) and 2004-12-18-06-trunk (which does).  The Firefox
2004-12-14-08-trunk nightly doesn't have the problem, but all the
2005-01-01 nightlies do have it.  I don't have a complete collection
of nightlies for December 2004 (nor can I find one on the web), so I
can't pin its origin down more precisely.  The problem doesn't happen
in any version of Camino (even the most recent nightlies).

The problem doesn't happen if you move the scrollbar with a mouse
wheel or the arrow keys.  It doesn't happen in Java 1.3.1 (either
using the MRJ Plugin JEP without the JavaEmbeddingPlugin.bundle or
using Apple's Java Applet.plugin).

The problem happens in OS X 10.3.7 with Java 1.4.2 Update 2 and also
in OS X 10.2.8 with Java 1.4.1.



Reproducible: Always
After fiddling around with cvs diff, my hunch is that this bug is
caused by the changes discussed at bug 151249, which seem to have been
committed on 2004-12-16 (they show up in a diff between 2004-12-15 and
2004-12-16).


(Following up comment #1)

I've confirmed my hunch:  If you add the changes discussed at bug
151249 (https://bugzilla.mozilla.org/attachment.cgi?id=157220, plus
small mods that were made when it was committed) to the source for
Mozilla 1.7.5 and compile it, you start getting the errors described
in this bug.

I'll be looking for some way to fix that code.


This patch adds the bug 151249 fix (allow the middle mouse button to
follow a link) to Mozilla 1.7.5, plus a fix for bug 278655 (the same
one as in the previous patch, for Mozilla 1.8a6).  The point of this
is to demonstrate that the bug 151249 fix with my tweak to it is
modular -- it still works in a different environment than the one for
which it was intended (i.e. Mozilla 1.7 "branch" as opposed to Mozilla
1.8 "trunk").
I've found a fix for this bug, which I just attached in two different
forms (see my previous two comments).

The bug 151249 fix changes how mouse up and mouse down events are
received from the OS:  Previously they were picked off an OS event
queue using GetEvent() in nsMacMessagePump::DoMessagePump() (in
nsMacMessagePump.cpp).  Now the OS feeds them to a "Carbon event
handler", nsMacMessagePump::CarbonMouseHandler().  DoMessagePump()
used to send them on to nsMacMessagePump::DispatchEvent() (which feeds
them to the appropriate Mozilla handler).  Now CarbonMouseHandler()
does this.

The problem is that the original code assumed that, under the new
arrangement, no mouse up or down events would slip through to the old
"handler" (DoMessagePump()).  But this assumption is false, especially
(but not only) when the Java Embedding Plugin is involved.  When a
mouse up or down event does "slip through", it gets handled twice by
the Mozilla handlers, which can lead to strange results (including the
jumping scrollbar that's the subject of this bug).

My new code stops any mouse up or down events that "slip through" to
GetEvent() from being sent along to Mozilla's handlers.  In its
current form it also logs to the console whenever it swallows one of
these events.  This is useful for testing, but should probably be
eliminated in the final version (the one that gets committed).  Or my
code could be altered to only log this information in a debugging
version of Mozilla.

The reason the Java Embedding Plugin is so badly effected has to do
with the contortions I (the JEP's developer) have to go through to get
mouse and keyboard events to work in it at all -- I have to get around
a previously installed set of Carbon event handlers (the ones that
handle the Cocoa/Carbon interface) that normally don't let _any_
keyboard or mouse events through to the underlying application (in
this case the Mozilla browser).  To do this I have to send each
keyboard and mouse event to _every_ installed Carbon handler (and
finally also to the GetEvent() event queue), instead of just to the
one most recently installed (which is the default behavior).

(By the way, here's a link to the Java Embedding Plugin, which I
forgot to include earlier:  http://javaplugin.sourceforge.net.)


See also bug 277837.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Revised fix for Mozilla 1.8a6 (obsolete) — — Splinter Review 
The previous version of this patch might break the Profile functions
in nsMacMessagePump::DoMessagePump().
Attachment #171681 - Attachment is obsolete: true
Attached patch Revised fix for Mozilla 1.8a6 (obsolete) — — Splinter Review 
Oops ... wrong patch.
Attachment #171758 - Attachment is obsolete: true
The previous version of this patch might break the Profile functions
in nsMacMessagePump::DoMessagePump().
Attachment #171684 - Attachment is obsolete: true
Steven: can you test my patch in bug 277837 and see if it fixes the double
scrolling issue, eliminating the need for JEP hacks? If so, then you'll want to
roll it into this patch.
I've already tested your bug 277837 patch.  It doesn't, by itself,
solve the jumping scrollbars issue.  I've just started testing a
combination of your patch and mine.  When I'm done I'll post patches
and comments here.  That should be in a few hours.

By the way, beware that the build process for the Mac "widgets" stuff
is partly broken:  If you only make changes to nsMacMessagePump.cpp,
they won't get into the Mozilla "binary".  You also need to change the
timestamp on nsMacMessagePump.h.  If you've been rebuilding without
doing this, it might explain why your tests show your bug 277837 patch
fixing the jumping scrollbars issue.
Assignee: general → smichaud
Component: General → Widget: Mac
Product: Mozilla Application Suite → Core
This patch combines my fix to the bug 151249 patch (which stops mouse
up and down events from being handled twice) and Simon Fraser's
(https://bugzilla.mozilla.org/attachment.cgi?id=171712, which stops
nsMacMessagePump::CarbonMouseHandler() from eating mouse up and down
events that Mozilla doesn't handle).  It resolves all the issues I'm
aware of, but of course needs further testing.
Attachment #171759 - Attachment is obsolete: true
This is just a port of my Mozilla 1.8a6 patch (which combines my
previous patch with Simon Fraser's) to Mozilla 1.7.5.  It's intended
to serve as a kind of control -- to eliminate (or identify)
potentially extraneous factors in the Mozilla 1.8a6 environment.
Attachment #171760 - Attachment is obsolete: true
I've been playing around with combinations of my patch with Simon
Fraser's (from bug 277837).  They seem to get along together, but each
is also necessary in its own right.  Together they resolve all the
issues that have been discussed here and at bug 277837 ... including
the one that Javier Pedemonte just discovered:

https://bugzilla.mozilla.org/show_bug.cgi?id=277837#c15

Before the bug 151249 bug was committed, the pre-Carbon GetEvent() API
was used to its maximum extent (to obtain from the OS all the kinds of
events that it knows about).  (Other events (like mouse-wheel events)
were handled by Carbon handlers, but that's a side issue.)

Apple has (more or less) deprecated GetEvent() and the event queue it
gets its events from.  So the GetEvent() event queue (generally) only
receives the more-favored Carbon handlers' leftovers:  Carbon handlers
(OS-installed and app-installed ones) get first crack at system
events.  Only those events they don't handle (generally) find their
way to the GetEvent() event queue.  But, as we've seen, there are some
exceptions to this rule.

The original fix for bug 151249 (the one that's currently in the
trunk) took mouse up and down events away from GetEvent() and gave
them to nsMacMessagePump::CarbonMouseHandler().  But it neglected to
add code to nsMacMessagePump::DoMessagePump() to ensure that no mouse
up or down event slips through to GetEvent() and gets handled twice --
hence the need for my original patch.

The original CarbonMouseHandler() swallowed all events Mozilla's
handlers _could_ handle, whether or not they actually did handle them.
This meant that OS-installed Carbon handlers (such as those used by
the Cancel and Replace buttons mentioned in the description of bug
277837), which are installed before app-installed Carbon handlers,
often didn't see mouse events they should see.  Hence the need for
Simon Fraser's patch.

But Simon's patch has a side effect:  For reasons that still aren't
entirely clear to me, his original patch causes a lot more mouse up
and down events to slip through to GetEvent() and be handled a second
time, after they've already been handled via CarbonMouseHandler().
This is what causes the problem Javier Pedemonte noticed.  And this is
why adding my patch to Simon's clears it up.



As I've said, the combination of my original patch with Simon Fraser's
bug 277837 patch clears up all the bug-151249-fix issues that I'm
aware of.  And it may clear up all those that exist, but it needs more
testing.

Here's one issue to keep in mind while you're testing:  The order in
which OS-installed Carbon handlers and Mozilla's high-level handlers
(called from nsMacMessagePump::DispatchEvent()) receive mouse up and
down events is different now that these events are going through
CarbonMouseHandler().  The OS-installed Carbon handlers used to get
them first, but now Mozilla's high-level handlers do ... which means
that the OS-installed Carbon handlers may still not be receiving some
events that they used to receive.  (This doesn't apply to JEP users
who've loaded an applet into the current window -- the JEP sends
keyboard and mouse events to _all_ installed Carbon handlers.)

Another thing to remember (I've already mentioned it above, but it was
a pain in the rear to figure out, so it's worth mentioning again):

> Beware that the build process for the Mac "widgets" stuff is partly
> broken:  If you only make changes to nsMacMessagePump.cpp, they
> won't get into the Mozilla "binary".  You also need to change the
> timestamp on nsMacMessagePump.h.
Flags: blocking-aviary1.1+
Steven, you should nominate the bug (i.e. "?") instead of +ing it, thanks.
Flags: blocking-aviary1.1+
Oops ... sorry.
Flags: blocking-aviary1.1?
One reason why you might be seeing events being dispatched both through the
Carbon Event handler and the GetEvent() path is that DispatchEvent() in the
handler is returning PR_FALSE for regular mouse clicks, which means that they
get fed into WaitNextEvent (as they should).

So I think the real bug is that somewhere, gecko lies about whether it has
handled the event. In addition, we need to ensure that events targeted at
plugins report their 'handled' state correctly.
Bug 277837 has a new attachment that should fix almost all the double handling
issues.
(From comment #5)

> My new code stops any mouse up or down events that "slip through" to
> GetEvent() from being sent along to Mozilla's handlers.

(From comment #15)

> Apple has (more or less) deprecated GetEvent() and the event queue
> it gets its events from.

I should really have been talking about WaitNextEvent() :-(

Here's an Apple doc on "Carbon Events Versus WaitNextEvent":

http://developer.apple.com/documentation/Carbon/Conceptual/Carbon_Event_Manager/Concept/chapter_17_secti\
on_4.html


This is Simon Fraser's latest bug 277837 patch
(https://bugzilla.mozilla.org/attachment.cgi?id=171850) plus his idea
to make nsMacMessagePump::CarbonMouseHandler() only handle middle
mouse clicks (i.e. to "emasculate" it).

I think this is the best patch so far, largely because it makes the
smallest change to the pre-bug-151249-patch behavior of Mozilla (and
Firefox).  It preserves the bug-151249-patch's functionality
(including making the middle mouse button behave like the left mouse
button in at least some plugins, like the QuickTime plugin).  It's no
longer possible for both CarbonMouseHandler() and DoMessagePump() to
pass the same event to nsMacMessagePump::DispatchEvent(), and thus
cause it to be handled twice (this fixes our bug 278655).  The flow of
left-button and right-button events to previously installed Carbon
handlers and to WaitNextEvent() in now completely unimpeded.  The code
now makes a best-faith effort to ensure that middle-button events are
consumed only when they should be, and are otherwise passed along to
previously installed Carbon handlers (OS-installed, app-installed or
plugin-installed ones).  Left-button and right-button events are now
once again handled in their previous order -- once again Carbon
handlers get them before Mozilla's high-level handlers do (via
DoMessagePump()).

Last but not least, this patch resolves both this bug (278655) and bug
277837, without (as far as I know) adding any side effects.

Please test it in as many different ways as you can think of -- in
particular with as many oddball plugins as you can find ... including
the Java Embedding Plugin :-)

(On a side note, I tried to track down the build problems I've been
having ... and was unable to reproduce them.  I was probably doing
something stupid, but do still keep an eye out for changes not getting
into the binary on rebuild.)

(Another note:	Due to recent (unrelated) changes in
nsMacEventHandler.cpp, this patch might no longer apply to the latest
cvs version of Mozilla.  There should be no problems applying it to
the Mozilla 1.8a6 source tarball.)
Attachment #171806 - Attachment is obsolete: true
Attachment #171808 - Attachment is obsolete: true
Attachment #171809 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 172010 [details] [diff] [review]
Fix for Mozilla 1.8a6 (includes best ideas so far)

This patch looks good to me, but I haven't had time to test it yet.
Attachment #172010 - Flags: superreview+
Attachment #172010 - Flags: review?(pinkerton)
will this conflict with all the recent middle-click event changes? i see there's
specific middle-click handling code in here.
> all the recent middle-click event changes

I don't know what this refers to.  Please let me know.


Comment on attachment 172010 [details] [diff] [review]
Fix for Mozilla 1.8a6 (includes best ideas so far)

looks good enough to me, though i also haven't tried it. r=pink
Attachment #172010 - Flags: review?(pinkerton) → review+
I've now tested attachment 172010 [details] [diff] [review] with all the plugins listed at
http://plugindoc.mozdev.org/OSX.html (except for the Acrobat Reader
plugin, which PluginDoc says doesn't work on OS X, and the Windows
Media Player plugin, which I couldn't get to work properly even in
Mozilla 1.7.5 and Firefox 1.0).  I tested with Mozilla 1.8a6 (patched)
and the CVS version of Mozilla (current as of last night), but not
with Firefox.  (I'll test in Firefox once I've gotten it to build
correctly from CVS.)

Everything (basically) worked fine -- which is to say that (basically)
the middle mouse button always worked like the left mouse button.
There were problems with the complex slider control that you sometimes
get when you click and hold on the "sound" button:  When you use the
left mouse button a floating slider appears that you can drag up and
down (to raise or lower the sound level).  But when you use the middle
mouse button the slider appears momentarily and then disappears.  I'm
not sure that anything can be done about this in Mozilla (i.e. it may
be a case of the plugin getting "confused" with input it doesn't
expect).  And in any case I think it's a relatively minor problem.
(It also happens with the unaltered bug 151249 patch, as present in
the unpatched Mozilla 1.8a6.)

It's easy to find Acrobat Reader documents that try to load inline
(examples of using the Schubert-it PDF Browser Plugin), so I won't
mention any examples.  For QuickTime I used the movie trailers at
http://www.apple.com/trailers.  Here's a list of the examples I used
for the other plugins:

Adobe SVG Viewer

  The "Chemical Markup Language Demo" at
  http://www.adobe.com/svg/demos/main.html.

Macromedia Flash Player

  http://www.macromedia.com/software/flash/flashpro/video/gallery/

Macromedia Shockwave Player

  http://www.forgefx.com/casestudies/prenticehall/ph/catapult/catapult.htm

RealOne Player

  "Latest News in Videa and Audio" at http://news.bbc.co.uk/

  (The first time you try this it makes you choose between Real Player
  format and Windows Media Player format.  If you've already chosen
  the "wrong" one, you'll need to delete the cookie that stores this
  information and then try again.)



Steven,

Just a question for clarification; when you say the middle button works like the
left button, are you saying a middle-click on a link no longer opens a new tab,
but rather just goes to the linked page?
Yes, the left and middle mouse buttons do (still) behave (a little)
differently when following links.  I should have been clearer.

In Mozilla 1.8a6 (and the current CVS version of Mozilla), patched
with attachment 172010 [details] [diff] [review] or unpatched, following a link with the left
mouse button opens the target in the same page, but following a link
with the middle mouse button opens the target in a new page.

In recent Firefox nightlies (unpatched) and the (patched) current CVS
version of Firefox (which I've finally managed to build) following a
link with the left mouse button behaves the same way, but following a
link with the middle mouse button opens the target in a new tab.

So attachment 172010 [details] [diff] [review] hasn't changed this behavior in either Mozilla or
Firefox.

It's interesting that this "open the target in a new page or tab"
behavior sometimes causes problems:  For example, left-clicking on the
"Latest News in Video and Audio" button on the BBC News home page
(http://news.bbc.co.uk/) (which runs a JavaScript command) works fine,
but middle-clicking it causes a JavaScript error.  With Mozilla it
also opens a blank window (though not a blank tab on Firefox).


The JavaScript link issue is a general problem with opening a JS link in a new window/tab. They won't 
work if you control-click (command-click on Mac) or right-click (control-click on Mac... ;) and select 
"Open Link in New Window" either.
On a side note, bug 279742 has been opened on Firefox 1.0 reporting
that Autoscroll doesn't work on OS X.  crot0@infoseek.jp responded
that it does work on recent nightlies that have the bug 151249 fix.

Autoscroll still works (if you've turned it on in your Preferences) on
the current CVS version of Firefox with attachment 172010 [details] [diff] [review].

crot0@infoseek.jp says that it works very awkwardly.  I didn't think
so -- either on a recent Firefox nightly or on my patched CVS version.

In any case, it seems to work the same with or without attachment
172010 [details] [diff] [review].


Steven: thanks for the thorough testing. Do you have cvs checkin privs, or do
you want me to land this?
(In response to comment #32)

You're quite welcome.

If someone's given me cvs checkin privileges, they didn't tell me
about it ... which I assume means I don't have them.

Please do go ahead and check the patch in.  Let's give it some
exposure in the nightlies.  We've still got time before the FF 1.1
release to fix problems that might arise.


(In response to comment #30)

You're right.  Right-click (on BBC News's "Latest News in Video and
Audio" button) plus open-link-in-new-window fails in more or less the
same way with IE on both the Mac and Windows.

Interestingly, right-clicking on this button does work, sort of, in
Safari:  The choices you get are "Copy Link to Clipboard", "Open Image
in New Window", "Save Image As ..." and "Copy Image To Clipboard".
The "link" is the JavaScript command.  The "image" is the button
image.


(Following up comment #27)

I've repeated my tests of attachment 172010 [details] [diff] [review] in the current CVS version
of Firefox.  With Autoscroll off, I got the same results as with
Mozilla 1.8a6 and the latest Mozilla CVS version.

With Autoscroll on, the results were, shall we say, a bit wierd -- the
plugins displayed more or less as before (though with some anomalies),
but Firefox did the Autoscroll thing even when the middle-click took
place over the plugin.  I'd say this is a bug in OS X Firefox -- it
doesn't happen in Windows Firefox (at least with a Java applet).  It
also happens in a recent Firefox nightly (i.e. one with the bug 151249
fix but without attachment 172010 [details] [diff] [review]).

Should I file a bug report ... or does someone know that this has
already been reported?


(Following up comment #35)

> it doesn't happen in Windows Firefox (at least with a Java applet)

To expand on this a bit:  Autoscroll works correctly in Firefox 1.0 on
Windows.  It does get invoked when it should.  It doesn't get invoked
when you middle-click on a plugin.


I checked the patch in to the trunk. Thanks Steven!
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
(Following up comment #35 and comment #36)

> Should I file a bug report ... or does someone know that this has
> already been reported?

I looked and couldn't find one, so I've opened bug 279958.  I even
posted a fix there.
Flags: blocking-aviary1.1?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: