Closed Bug 277837 Opened 20 years ago Closed 20 years ago

[Mac] Replace/Cancel buttons don't respond to mouseclicks during "Save As" operation (filepicker)

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: marcia, Assigned: sfraser_bugs)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Be more aggressive about saying we handled mouse events
23.59 KB, patch
Details | Diff | Splinter Review
Follow Simon's suggestion to "emasculate that Carbon Event handler"
1.52 KB, patch
Details | Diff | Splinter Review 
Found using 2005-01-10-05-trunk/

STR:

1.  Download a file to any location, using Save As from the file menu.
2.  Download the same file again to the same location. Wait to get the "Item
already exists - do you want to replace it dialog"
3.  Click on either cancel or replace, nothing happens.

I tested this on two different Macs with the same results.  Haven't seen
problems with other buttons.  Also tested on Windows and did not see this issue
on 20050110-06.

Will try to see if this is a regression.
this also occurs in today's Firefox 2005011007-trunk Mac OS X (10.3.7) build.
moving over to Core:File Handling.

note: this is not a problem with Firefox 1.0. perhaps aviary-landing issue...?
will look into regression window.
Assignee: general → file-handling
Component: General → File Handling
Flags: blocking1.8b?
Flags: blocking-aviary1.1?
Keywords: aviary-landing, regression
Product: Mozilla Application Suite → Core
QA Contact: general → ian
btw, no js console errors seen. :(
clarifying summary: the buttons in this dialog don't respond to mouseclicks.
interestingly, they do respond to keyboard shortcuts like Esc, Return and spacebar.
Summary: Replace/Cancel buttons not functioning properly during "Save As" operation → Replace/Cancel buttons don't respond to mouseclicks during "Save As" operation
not due to aviary-landing, afaict, since it regressed btwn 15-Dec and 20-Dec.
getting narrower window...
Keywords: aviary-landing
regressed between 2004121508-trunk (works) and 2004121608-trunk (broken). could
the fix for bug 151249 be the cause?
OS: All → MacOS X
Summary: Replace/Cancel buttons don't respond to mouseclicks during "Save As" operation → [Mac] Replace/Cancel buttons don't respond to mouseclicks during "Save As" operation
dup of bug 277678.
*** Bug 277678 has been marked as a duplicate of this bug. ***
This sounds like some event filtering issue.
*** Bug 278801 has been marked as a duplicate of this bug. ***
[*** Bug 278801 has been marked as a duplicate of this bug. ***]

Yet even today, this bug doesn't show up in the search page for FireFox.  How in
the world are we supposed to find these bugs before duplicate reporting?  Oh,
well, it will be fixed Real Soon Now?

Maybe somebody will change the status to assigned?
Summary: [Mac] Replace/Cancel buttons don't respond to mouseclicks during "Save As" operation → [Mac] Replace/Cancel buttons don't respond to mouseclicks during "Save As" operation (filepicker)
This happens because our carbon event handler picks up the mouse events and
swallows them.
Assignee: file-handling → sfraser_bugs
This was regressed by the fix for bug 151249, for the very reason that I was
worried about (https://bugzilla.mozilla.org/show_bug.cgi?id=151249#c59).
I tried testing the effects of this patch on bug 278655, but wasn't able to
reproduce it (possibly because the JEP hides the problem).
Comment on attachment 171712 [details] [diff] [review]
Patch to make sure we only return noErr from the Carbon Event handler if we've handled the event

r=pink
Attachment #171712 - Flags: review+
Hmm, interesting side-effect.  In a build with this patch applied, open two
windows.  Make sure that the close button of the front most window overlays the
titlebar of the window behind it.  Now click on the close button of the front
most window and release.  You should see that you can now drag around the
previously obscured window.  Any ideas what this could be, Simon?
(In reply to comment #15)

I've posted a patch at bug 278655 that fixes this problem.  It
combines Simon Fraser's patch (attachment 171712 [details] [diff] [review]) with one that I've
been working on.


I can't reproduce the phenomenon described in comment 15 with the patch.
This patch is similar to the last one, but it adds code that makes mouse events
report that they have been handled if they have been dispatched to a widget.
This fixes almost all the 'double handling' issues described in bug 278655.

The only time that WNE returns a mouse down now is if you click on a background
Mozilla window. In that case, the carbon event handler never sees the event
(but it does receive and handle the subsequent mouse up event).
Attachment #171712 - Attachment is obsolete: true
(In reply to comment #17)

> I can't reproduce the phenomenon described in comment 15 with the
> patch.

(I assume you were talking about the first version of your patch.)

That may because the Mac widgets' build process is (partly) broken (in
Mozilla 1.7.5 and 1.8a6):  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.

Were you aware of this?  Is it a known issue?  I haven't investigated
it fully, but I don't think the problem has existed for a long time (I
don't believe I've seen it on Mozilla 1.6).

In any case, I'll be testing the new version of your patch.
> If you only make changes to nsMacMessagePump.cpp, they won't get into the
Mozilla "binary"

I don't understand this. nsMacMessagePump is compiled into the widget library,
which is symlinked into the components directory, so a simple 'make' in
mozilla/widget/src/mac is enough. I was adding prints, so I knew when the
changed file was compiled.

On another note, I wonder if we should just emasculate that Carbon Event handler
so that it _only_ propages middle mouse clicks, and let everthing else back
through WNE?
> > If you only make changes to nsMacMessagePump.cpp, they won't get
> > into the Mozilla "binary"
>
> I don't understand this. nsMacMessagePump is compiled into the
> widget library, which is symlinked into the components directory, so
> a simple 'make' in mozilla/widget/src/mac is enough.

I know it's not supposed to be necessary to touch nsMacMessagePump.h
... but I had to do it.  I've been using source tarballs for Mozilla
1.7.5 and Mozilla 1.8a6, downloaded from ftp.mozilla.org.

Here's a simple test:  Add a printf statement somewhere in
nsMacMessagePump.cpp, rebuild, and see if the results show up at the
console when you run the resulting binary.  I'll bet they don't.

> On another note, I wonder if we should just emasculate that Carbon
> Event handler so that it _only_ propages middle mouse clicks, and
> let everthing else back through WNE?

It's certainly worth trying.


> Here's a simple test...

I was doing this repeatedly yesterday and it worked fine (I build from CVS).

> emasculate that Carbon Event handler

How much would that fix? It should fix this bug (I guess we'd still want the
'haveHandled' changes so that we didn't swallow middle clicks that aren't for
us), and it should remove all double handling issues for left and right clicks.

It would be nice to hear from Brion Vibber on this issue, since he caused it all.
Status: NEW → ASSIGNED
(In reply to comment #22)
> > emasculate that Carbon Event handler
> 
> How much would that fix? It should fix this bug (I guess we'd still want the
> 'haveHandled' changes so that we didn't swallow middle clicks that aren't for
> us), and it should remove all double handling issues for left and right clicks.

Since the current carbon event handler hack uses an undefined field (for that event type) in the event 
structure to hold the button info (unwise, but it's clearly labeled a hack ;) it might be best to use a 
custom event type of some sort for middle-click events rather than using the mousedown/mouseup if 
that field isn't always under our control. I don't know offhand whether there's a standard way of doing 
this on the mac with the classic event structures.

I do though wish this had been hashed out in the ~seven months since I posted the patch so it could be 
tested and reviewed, rather than all of a sudden after somebody committed it.
> emasculate that Carbon Event handler

I've now tried this out, and in my limited tests it works perfectly --
it resolves both this bug (277837) and bug 278655, without (as far as
I can tell) adding any side effects.

My patch should be applied after yours (attachment 171850 [details] [diff] [review]), and makes
minimal changes on it.	It could be neatened up, but it demonstrates
the point that "emasculation" works just fine for the problems at
hand.

> Since the current carbon event handler hack uses an undefined field
> (for that event type) in the event structure to hold the button info
> (unwise, but it's clearly labeled a hack ;) it might be best to use
> a custom event type of some sort for middle-click events rather than
> using the mousedown/mouseup if that field isn't always under our
> control.

If we can guarantee that your hacked middle-mouse-button event never
escapes to WaitNextEvent() (or if it does that it's never sent to
DispatchEvent() a second time), I don't see anything wrong with
continuing to use it.  It would never be seen by OS code (over which
we have no control, and which might (in principle) get confused by
it).  As long as we can make sure that none of Mozilla's high-level
event handlers (those called from DispatchEvent()) will get confused.

> I don't know offhand whether there's a standard way of doing this on
> the mac with the classic event structures.

I don't think there is one.
> If we can guarantee that your hacked middle-mouse-button event never
> escapes to WaitNextEvent() (or if it does that it's never sent to
> DispatchEvent() a second time)

Oops.  Your hacked event will never be seen by OS code, no matter what
we do.  Any event that might "escape to WaitNextEvent()" would have
been generated by the OS itself (and fed to the WaitNextEvent() event
queue).

So all we have to worry about it making sure Mozilla's own high-level
event handlers won't get confused by it.


(In reply to comment #25)
> Oops.  Your hacked event will never be seen by OS code, no matter what
> we do.

IIRC the hacked event does get passed to plug-ins, though. In my testing some months back, the 
QuickTime plugin responded to middle-clicks as if they were left-clicks (mouseDown/mouseUp) with 
the carbon events hack in, whereas without it ignored them (nullEvent).

> Any event that might "escape to WaitNextEvent()" would have
> been generated by the OS itself (and fed to the WaitNextEvent() event
> queue).

My concern was more that an OS-generated event might stuff a value in that presently undefined field 
which mistakenly triggers the middle-click interpretation. Of course this is a more or less deprecated 
interface provided for source code compatibility with old apps, so we might consider it unlikely.

(In reply to comment #26)

> IIRC the hacked event does get passed to plug-ins, though. In my
> testing some months back, the QuickTime plugin responded to
> middle-clicks as if they were left-clicks (mouseDown/mouseUp) with
> the carbon events hack in, whereas without it ignored them
> (nullEvent).

You're right.  I tried the QuickTime plugin (using Apple's movie
trailers) with (unaltered) Mozilla 1.7.5 and 1.8a5 (the last version
that didn't have the bug 151249 fix) -- the left and right mouse
buttons work on the pause/play button, but the middle one has no
effect.  But with both the latest cvs version of Mozilla (with the
151249 fix as you wrote it) and Mozilla 1.8a6 with the "emasculated"
patch, the middle mouse button has the same effect (on the pause/play
button) as the left and right ones.

I'm not sure what can (or should) be done about this.  One possibility
would be to send a nullEvent to the plugin in the place of one of your
middle-mouse-button events.

> My concern was more that an OS-generated event might stuff a value
> in that presently undefined field which mistakenly triggers the
> middle-click interpretation.

I see.

> Of course this is a more or less deprecated interface provided for
> source code compatibility with old apps, so we might consider it
> unlikely.

I agree that we should.


(Following up comment #24)

> > emasculate that Carbon Event handler
>
> I've now tried this out, and in my limited tests it works perfectly
> -- it resolves both this bug (277837) and bug 278655, without (as
> far as I can tell) adding any side effects.

I forgot to add that with your "unemasculated" patch (attachment
171850 [details] [diff] [review]), bug 278655 still happens.


(Following up comment #27)

> One possibility would be to send a nullEvent to the plugin in the
> place of one of your middle-mouse-button events.

On second thought I really don't like this suggestion.  People have
already made a big fuss (at bug 151249) about making the middle mouse
button behave the same way as the left one when following links.  This
would stop the QuickTime plugin (and possibly other plugins) from
treating the middle mouse button like the left one.  (Yes, as you
(Brion) pointed out in
https://bugzilla.mozilla.org/show_bug.cgi?id=151249#c52, a plugin that
wanted middle-mouse-button events could install its own Carbon event
handler -- but this would most likely be used to give the middle mouse
button a special meaning.)

My hunch is that it's best to keep your hacked middle-mouse-button
events going to plugins, and hope for the best.

Side note:  It's too bad that most code changes (i.e. trunk changes)
go to Firefox at the same time they go to Mozilla alphas and betas --
speculative changes make sense in an alpha or beta, but can be
dangerous in something your grandmother uses.

Another side note:  It's _really_ too bad that (on OS X) an event is
stored in the nsPluginEvent struct as an EventRecord (which is by now
grossly outdated and limiting).


I've posted a cleaned-up version of the "emasculated" version of Simon
Fraser's attachment 171850 [details] [diff] [review] patch at bug 278655.  It fixes all known
problems, and (I think) covers all the bases about as well as they can
be covered.  Please take a look and test it.


*** Bug 279464 has been marked as a duplicate of this bug. ***
Fixed by the patch in bug 278655.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
*** Bug 276171 has been marked as a duplicate of this bug. ***
*** Bug 279973 has been marked as a duplicate of this bug. ***
Flags: blocking1.8b?
Flags: blocking-aviary1.1?
*** Bug 283815 has been marked as a duplicate of this bug. ***
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: