379199 - Key events (delete/backspace, escape, etc.) aren't propagated up the view hierarchy

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus S.]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a5pre) Gecko/20070428 Camino/1.2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.9a5pre) Gecko/20070428 Camino/1.2+

Cmd-left and Cmd-right no longer works for back and forward which is a bitch when using en-US Camino builds with a non-US keyboard layout.
This has already been discussed in Bug 160556 but I've been told there that "We don't re-open five-year-old bugs that happen to have the same symptoms as a
new issue; please file a new bug".
So here is the new bug.

Maybe the patch in Bug 160556 can be re-applied to the current trunk.

Reproducible: Always

Comment 1

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

I can confirm this in my trunk build.  They work fine in today's branch nightly.

Finding when this regressed would be helpful.

Comment 2

[Stuart Morgan]

(In reply to comment #0)
> Maybe the patch in Bug 160556 can be re-applied to the current trunk.

That code hasn't changed, nor does it differ between branch and trunk.

Josh, from some gdb-ing the issue is a change in keyDown: handling; things used to always be forwarded up to the superclass, which would eventually result in us getting an moveToBeginningOfLine: call, but now the logic is:

  if (nsTSMManager::IsIMEEnabled())
   [super interpretKeyEvents:[NSArray arrayWithObject:theEvent]];
  else if (nonDeadKeyPress && !dispatchedKeyPress)
   [self insertText:[theEvent characters]];

So the event dies there. I'm not sure offhand what the right solution is.

Comment 3

[Markus S.]

(In reply to comment #1)

> Finding when this regressed would be helpful.

I have currently only a very slow internet connection, thus I can't download a bunch of nightlies just to check when it happend. (Maybe after an electrician on wednesday had a look at my lines, but no promisses.)
I’m not sure, but I think it happend within the last week. Maybe two weeks, at most.

Comment 4

[Stuart Morgan]

We don't really need a regression range now that we know what code change triggered this.

Comment 5

[philippe (part-time)]

Probably related, noticed yesterday, the esc key fails to stop animated gif images to animate. That worked correctly on build 04/18, but I haven't had the time to check for more.

Comment 7

[Paul Borokhov (lensovet)]

can we put the word backspace somewhere into this bug? i filed the dupe because of this...

Comment 8

[Stuart Morgan]

Re-summarizing and moving to Core, since that's where the issue actually is.

Comment 9

[Chris Lawson (gone)]

Another summary tweak.

Comment 12

[Chris Lawson (gone)]

This might be due to bug 376093; if someone wants to build with that patch backed out and test, feel free.

Comment 13

[Chris Lawson (gone)]

Scratch that; this is probably bug 358899 instead.

Comment 14

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

This definitely broke between 2007-04-15 05:00 to 2007-06-16 05:00, which does squarely point to bug 358899.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=Camino&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-04-15+05%3A00&maxdate=2007-04-16+05%3A00&cvsroot=%2Fcvsroot

Notes:

1) During that period, focus wasn't being given to the page after load is complete, so you had to click on the page to perform any action (e.g., space-as-page-down as well as the broken Cmd-Arrow or delete); the focus regression appears to have been fixed since.

2) This range is for Cmd-Arrows and Delete (backspace); Esc broke a few days later (between 26 Apr and 28 Apr)

Comment 15

[philippe (part-time)]

I tried backing out bug 358899, but that fails completely... :-(
> patching file content/base/public/nsContentUtils.h
> Hunk #1 FAILED at 1014.
> 1 out of 1 hunk FAILED
....

Comment 16

[Chris Lawson (gone)]

(In reply to comment #15)
> I tried backing out bug 358899, but that fails completely... :-(

Yeah, not a big surprise -- there's been a ton of stuff in that file that's changed since April, unfortunately, so you'd probably have to back it out by hand (and there may be code that's been checked in since that depends on the code in that checkin).

Comment 17

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

(In reply to comment #14)
> Esc broke a few days later (between 26 Apr and 28 Apr)
 
Or maybe not; Esc seems to work if the page is not focused, but not if the page is focused (completely the opposite of Cmd-Arrows).  It's a separate bug/regression, I guess; my head hurts too much to chase it now.

Comment 18

[Josh Aas]

Maybe the bug here is how Camino implements things like cmd-[left-arrow]. Event flow is like this:

1. Camino BrowserWindow performKeyEquivalent: called
2. Camino BrowserContentView's performKeyEquivalent: called
3. nsChildView performKeyEquivalent: called
4. nsChildView keyDown: called

We call [super interpretKeyEvents:] from nsChildView's mouseDown only if IME is enabled. I'm not at this moment in a position to comment on the issue of that being the right choice only when IME is enabled, but it seems like whether or not we call that Camino shouldn't rely on it.

It seems reasonable that Camino should intercept the command via performKeyEquivalent: instead of waiting to get it in a 5th step that relies on nsChildView behaving in a certain way re: the text input protocol. If the browser window intercepts the event in pKE and return "YES" because it handled it, then gecko views never have to even see it.

Comment 19

[Stuart Morgan]

Intercepting pKE: does not give us the desired behavior. We don't want first crack at the event, we want last crack, so that IME, text fields, etc. can take events like command-arrow if applicable, and we assign them the history meaning only if no-one else consumed the event.

We need to have at least the keyDown: (if not editor events) propagate up to Camino before anything can be done at the Camino layer. Gecko needs to not be a black hole for key events that it doesn't actually consume.

Comment 20

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

Josh, is this bug also the cause of cmd-opt-arrow not switching tabs any more in Minefield, too, or is that some other bug?

Comment 21

[Josh Aas]

Not sure, but I'll be posting a fix for this pretty soon.

Comment 22

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

(In reply to comment #20)
> Josh, is this bug also the cause of cmd-opt-arrow not switching tabs any more
> in Minefield, too, or is that some other bug?

For the record, that turned out to be bug 404184.

Comment 23

[Josh Aas]

Attachment: fix v0.5

This is a backup of my first attempt at fixing this bug. Turns out we have some more fundamental issues with embedding and native event routing that we need to tackle, so this won't really work. More on that later.

Comment 24

[Josh Aas]

I am closing this bug for now because I don't think it is a bug. The situation is tricky though, so I'll write out the whole thing here. If someone convinces me that this analysis is wrong, then we can reopen this bug.

One thing that I didn't know when I started working on this bug is that there is really no way to tell if Gecko handled a particular event, like a key event. Every event we send return some status, but that is only accurate for some events. It is expected to be ignored for many basic events. As I understand it this is because any number of scripts in a web page or within gecko could do something and there is no way to reliably report a handled status, or at least that isn't going to happen any time soon. At this time, it is not considered a bug that this happens and that issue is far beyond the scope of this bug.

Given this, we have no choice but to assume that Gecko handled any event that it gets. If we don't we'll have major problems with events that get handled twice - once by gecko and once by the embedder. So, when Gecko gets an event it eats it and that is that. No Mac embedder can have any features that rely on knowing whether or not Gecko handled an event, which includes things like Camino's command-arrow navigation feature. This isn't really as bad as it sounds, in fact it shouldn't be problematic at all. Command-arrow back/forward navigation, for example, was a shoddy and unreliable feature from the start.

Embedders have plenty of opportunities to respond to events before Gecko does even if Gecko is the first responder. Look into the paths for keyDown: and performKeyEvent:, for example. Embedders can see either before the first responder via the top-level window's API for the event or via its sendEvent: method.

Comment 25

[Stuart Morgan]

Alrighty, bug 405744 now covers the currently planned approach to fixing the original command-back/forward bug for Camino.

Comment 26

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

(In reply to comment #5)
> Probably related, noticed yesterday, the esc key fails to stop animated gif
> images to animate. That worked correctly on build 04/18, but I haven't had the
> time to check for more.

philippe, can you re-file this part against Camino?  The regression range for it still makes my head hurt (comment 14 pgh 2 and comment 17), moreso since the regression range cl and I found using 10.3 and 10.4 doesn't at all mesh with what I'm seeing on 10.5.

Comment 27

[philippe (part-time)]

(In reply to comment #26)

> philippe, can you re-file this part against Camino?  The regression range for
> it still makes my head hurt (comment 14 pgh 2 and comment 17), moreso since the
> regression range cl and I found using 10.3 and 10.4 doesn't at all mesh with
> what I'm seeing on 10.5.
> 
done, bug 405748.

Comment 28

[Stuart Morgan]

I think this bug needs to be re-opened. I have a serious problem with this statement:
> No Mac embedder can have any features that rely on
> knowing whether or not Gecko handled an event

It is the case that Minefield has delete-as-back handling, which presumably relies on exactly the information that I need to implement the same thing in Camino. 

But here's what it appears I'm going to have to do instead if I want to make the same feature work in Camino again (since delete doesn't trigger pKE:, and Gecko eats keyDown:)
- Override sendEvent: at the window level
- Check every event to see if it's a delete keypress
- If so, figure out whether the event is destined for a Gecko view
- If so, figure out whether that Gecko view is in a state where the delete needs to be sent to it (e.g., if a text field is focused).
- If not, intercept the delete.
Not only is that logic much more error-prone, but it has to happen at the window level, which is several layers above the point where these kinds of decisions should be made, so it's architecturally unpleasant.

I don't think forcing embeddors to go that far out of their way to do key handling is reasonable.


JavaScript provides a way to stop an event from being propagated; why is it unreasonable to say that if an event is not stopped before it propagates all the top level, then it should continue to be propagated out into Cocoa? I understand that a page could intercept an event, do something, and let it keep propagating, but the same thing is true in Cocoa. If a keypress has been handled to the point that having a second handler would be wrong, then whoever did that handling has a responsibility to stop the event.

Comment 29

[Stuart Morgan]

(Retitling to clarify the current state of the bug)

Comment 30

[Stuart Morgan]

My understanding from Josh is that this isn't going to be re-opened.

I find it extremely unfortunate that despite the fact that both JS and Cocoa have similar event propagation models in this regard, breaking the chain at the embedding layer--and thus denying embedders access to the information that web pages and Minefield have about event handling, and making ChildView behave contrary to expected NSView behavior--is considered correct behavior.

Comment 31

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

I'm sorry, but it seems unreasonable to me to tell Cocoa embedding clients of a widget toolkit that was originally designed specifically for Cocoa embedding that they're no longer going to be given access to information they need to implement common browser features (and, in this case, a feature that worked prior to this).  Similarly, telling Cocoa clients that their NSView implementation is going to behave completely contrary to expected Cocoa NSView behavior boggles the mind.

This bug was originally WONTFIXed when another strategy was suggested as a viable alternative to fixing the un-Cocoa and non-embedding-friendly behaviors this bug described; however, as Stuart points out in comment 28, it turns out that strategy is not really viable (or performant, or architecturally reasonable, or robust).  

Because that suggested alternative turns out not to be much of an alternative after all, because I'm not aware of any alternate alternative being suggested or having a bug, and because further conversations in closed bugs are often not noticed, I'm reopening this bug and renominating for blocking1.9.  Stuart has raised some serious issues with the outcome of this bug in his recent comments, and as far as I can tell, they are not being addressed at all.  I understand that this is at least a moderately, if not very, difficult issue, but leaving a major architectural issue WONTFIXed just because it is hard doesn't seem like the right solution, either.

Comment 32

[Josh Aas]

We've gone over this many times in bugs and chats, we can't fix this in an ideal sense now and we're certainly not going to do it for Gecko 1.9. I don't think what we're doing now needs fixing anyway. Camino does have access to the information it needs to implement the browser features you're talking about (its pretty easily available too), my understanding is that smorgan just doesn't want to do it because he doesn't think he should have to override sendEvent: or use similar methods. If he doesn't want to do it that's fine, but he has the choice.

I'm not going to argue the open/closed status of this bug, it doesn't really mean anything at this point. I think closed reflects the true status of things better, but if you want to leave it open in the sense that it is open for discussion and new ideas in the future, go for it.

Comment 33

[Stuart Morgan]

(In reply to comment #30)
> [...] making ChildView behave contrary to expected NSView behavior

(In reply to comment #32)
> my understanding is that smorgan just doesn't want to do it

Making ChildView have completely the wrong semantics for a Cocoa view isn't just about my opinion or laziness.

> I don't think what we're doing now needs fixing anyway.

Yes, we've gone over this many times, but I can neither recall nor find an explanation of why you believe this to be true, and it is fundamental to whether this bug remains open or not, so it would be very helpful if you could record here:
  1) Your reason for believe that the current behavior is correct for an NSView subclass (or if that's not your assertion, then why having the wrong behavior doesn't need fixing).
  2) The reason that it's impossible to get this information, when internal to Gecko the information about whether an event is stopped or not already exists (as evidenced by the fact that JS event propagation does in fact work).

Comment 34

[Mike Pinkerton (not reading bugmail)]

it seems to me that forcing embedding apps to override at the window level (which we shouldn't be subclassing at all, mind you) is *wrong*, and the completely backwards way to fix this issue.

Who sr'd the changes originally to allow this behavior to change? What's the reluctance to fixing it the right way? 

Comment 35

[Josh Aas]

The reluctance is that we don't know of a correct solution. If somebody proposes a reasonable solution I'd be happy to do it. It's not as if we have a good solution and I don't want to do it because I'm in a bad mood or I hate Camino :) I don't know who sr'd the change you don't like but I'll take responsibility for it happily because I think it was the right thing to do, I committed it, and I'm the module owner. Not a pleasant thing to do, but the right thing given what I know now.

You can theorize about similarities between JS and Cocoa event propagation models all you want but that doesn't change the fact that as far as I know we have no way of knowing whether or not Gecko handled an event when we need to make a decision about propagation. I suspect our sloppy 1.8 branch Cocoa widgets impl was propagating events that Gecko already handled and it coincidentally didn't matter much because there wasn't much overlap with your command set, and that is why you think it "worked" before. Feel free to prove me wrong though!

Comment 36

[Stuart Morgan]

(In reply to comment #35)
> If somebody proposes a reasonable solution I'd be happy to do it.

I have proposed a potential solution: propagate an event out of ChildView if and only if nothing calls stopPropagation (or the equivalent backing implementation) on it. What is conceptually unreasonable about that?

Comment 37

[Josh Aas]

Your proposal isn't conceptually unreasonable, I never argued that. It is, however, unreasonable in practice as I've said before. I know this conclusion is disappointing conceptually but we have to be practical, even more-so than usual because of how close we are to shipping Gecko 1.9.

The problem is that so many parts of Gecko don't return that value the way you think they should. Really basic parts of Gecko, like interacting with radio buttons. It might be theoretically possible to fix that, might not be, but either way practically we're not going to re-architect event handling all over Gecko to make that happen. The solution you're proposing is not a reasonable one given my understanding of things at this time.

Comment 38

[Stuart Morgan]

(In reply to comment #37)
> It might be theoretically possible to fix that, might not be

Perhaps one of the people more familiar with Gecko's event architecture could comment on the fundamental event architecture question involved here.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #24)
> One thing that I didn't know when I started working on this bug is that there
> is really no way to tell if Gecko handled a particular event, like a key event.
That is true, but one can check if default handling was cancelled. If I understand this bug correctly, that is what this is about.

(In reply to comment #37) 
> The problem is that so many parts of Gecko don't return that value the way you
> think they should. Really basic parts of Gecko, like interacting with radio
> buttons.
What you mean here? Interacting with radio buttons happens on DOM level and
that is specified in WebForms2.

Comment 41

[Stuart Morgan]

Josh, can you explain why the return value of processKeyDownEvent: (to pick the keydown case; similar return values are propagated out of SendEvent for others) in ChildView--which according to the comments in the code and according to a bunch of tests I did logging that value for, e.g., the delete key in a variety of cases (in a text field, not in a text field on a normal page, not in a text field on a page that consumes the delete key, etc.) is *exactly* the correct value for making this decision--is not the right switch for deciding whether or not to propagate to super?

I'm trying to reconcile the claim that this is an unsolvably hard problem with the fact that we appear to have the correct information up until the very last minute, where it's discarded.

Comment 42

[Stuart Morgan]

And to be extremely clear about what I am asking: The question is not "Is this information completely and utterly infallible in every case?", but "Given that ChildView appears to already have access to the information that every non-embedding app would use to decide whether or not the event should continue to be propagated, why should embedding apps be denied that best-guess?"

I'll I'm asking is that the same information that currently decides if events should propagate for non-embedding cases also be applied to embedding (which is all I was asking in comments 30 and 36). Arguing that because the information isn't perfect, embedding can't have it (even though all the non-embeddors can), which seems to be what comment 37 is saying, seems very Taming-of-the-Shrew.

Comment 43

[Josh Aas]

Attachment: camino-only fix v1.0 (1.9.0 branch)

Comment 44

[Stuart Morgan]

Comment on attachment 403544 [details] [diff] [review]
camino-only fix v1.0 (1.9.0 branch)

r=smorgan

Josh and I have discussed this approach, and we agree that while it's not a complete long-term solution, it's a good localized change that gives Camino enough information to prevent functional regressions in 2.0.

Comment 45

[Daniel Veditz [:dveditz]]

Comment on attachment 403544 [details] [diff] [review]
camino-only fix v1.0 (1.9.0 branch)

Approved for 1.9.0.15, a=dveditz

Comment 46

[Josh Aas]

landed "camino-only fix v1.0 (1.9.0 branch)" on 1.9.0 branch

Checking in widget/src/cocoa/nsChildView.mm;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.370; previous revision: 1.369

Comment 47

[Eiichi]

Landing an attachment 403544 [details] [diff] [review] seems to cause bug 519922.

Comment 48

[Stuart Morgan]

(In reply to comment #47)
> Landing an attachment 403544 [details] [diff] [review] seems to cause bug 519922.

I'll be dealing with this at the Camino layer; trying to get more accurate information about IME event consumption is beyond the scope of what we want to do for 1.9.0 (and having some IME bleed-through from ChildView isn't a regression from 1.8.1 anyway).

Comment 49

[Stuart Morgan]

We're getting to the point where we have viable Camino builds off of 1.9.2, and this is one of our most serious regressions. From the Camino perspective we need to either:
a) land the current patch (i.e., ifdef'd for Camino) on 1.9.2, or
b) land a version of the patch with no ifdef

Which would be preferable, and can we start the process moving on whichever one that is?

Comment 50

[Stuart Morgan]

It would also be nice if we could get one of those options landed on the trunk now so we don't have to repeat this with each new branch until there is a more comprehensive fix.

(I would vote for (b) since there's no downside to providing better information than "nothing" to embedders now even if there will eventually be more improvements.)

Comment 51

[Stuart Morgan]

Comment on attachment 403544 [details] [diff] [review]
camino-only fix v1.0 (1.9.0 branch)

Per discussion with Josh, since there's been no general solution yet this ifdef'd version should be landed on 1.9.2 and trunk so that Camino can continue to track newer branches without waiting for a generalized fix.

I've verified that this part of the method hasn't changed since 1.9.0 (there's just been minor bitrot in the patch due to context changes outside the method).

Josh, do I need any new reviews or approval to land this on trunk?

Comment 52

[Josh Aas]

Comment on attachment 403544 [details] [diff] [review]
camino-only fix v1.0 (1.9.0 branch)

I'm fine with taking this on 1.9.2.5 on the same justification we had for 1.9.0.15.

Comment 53

[LegNeato]

Comment on attachment 403544 [details] [diff] [review]
camino-only fix v1.0 (1.9.0 branch)

a=LegNeato for 1.9.2.6. Please land this on mozilla-1.9.2 default.

Code freeze is this Friday @ 11:59 pm PST.

Comment 54

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

Landed on 1.9.2.6: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dd17e4ac68bb  (Stuart and I weren't clear who wrote the patch anymore after all this time, so I just copied the commit message from 1.9.0.)

Comment 55

[philippe (part-time)]

verified with: Camino Version 2.1a1pre (1.9.2.6pre 20100625004649)
(gecko rev: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7dca7be78aad)

delete/backspace is fully functional on Gecko 1.9.2 based Camino builds

Comment 56

[Mehmet]

(In reply to comment #55)
> delete/backspace is fully functional on Gecko 1.9.2 based Camino builds


Delete/backspace doesn't work on following page:

STR:

1.) go to http://code.google.com/p/adblockforchrome/issues/list
2.) click on a bug from the list
3.) Press delete/backspace to go back to the list

AR: delete/backspace does not work


OS: 10.6.4
Camino Version 2.1a1pre (1.9.2.8pre 20100705001228)


Note: With Firefox 3.6.6 (and Safari 5) it works fine there!


Thanks and regards
Mehmet

Comment 57

[philippe (part-time)]

(In reply to comment #56)

> Delete/backspace doesn't work on following page:

WFM, but if it fails with your keyboard (German ?), please file a Camino bug.

Comment 58

[Mehmet]

(In reply to comment #57)
> WFM, but if it fails with your keyboard (German ?), please file a Camino bug.

Yes, I have a german keyboard. Filed a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=577059

Comment 59

[Josh Aas]

There is a proper fix for this on trunk in bug 584965.

Comment 60

[Josh Aas]

Fixed on mozilla-central by the patch in bug 584965.

Comment 61

[Josh Aas]

Fix was backed out because of regressions.

Comment 63

[Josh Aas]

Fixed (again) on mozilla-central by the patch in bug 584965.