Closed Bug 379199 Opened 15 years ago Closed 11 years ago
Key events (delete/backspace, escape, etc
.) aren't propagated up the view hierarchy
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
I can confirm this in my trunk build. They work fine in today's branch nightly. Finding when this regressed would be helpful.
(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.
(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.
We don't really need a regression range now that we know what code change triggered this.
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.
Summary: Cmd-left and Cmd-right no longer works for back and forward → Cmd-left and Cmd-right, delete, shift-delete no longer work for back and forward
Target Milestone: --- → Camino2.0
can we put the word backspace somewhere into this bug? i filed the dupe because of this...
Re-summarizing and moving to Core, since that's where the issue actually is.
Assignee: nobody → joshmoz
Component: Accessibility → Widget: Cocoa
Product: Camino → Core
QA Contact: accessibility → cocoa
Summary: Cmd-left and Cmd-right, delete, shift-delete no longer work for back and forward → Dead-key commands (command-left/right, delete, escape, etc.) aren't propagated up the view hierarchy
Target Milestone: Camino2.0 → ---
Another summary tweak.
Summary: Dead-key commands (command-left/right, delete, escape, etc.) aren't propagated up the view hierarchy → Dead-key commands (command-left/right, delete/backspace, escape, etc.) aren't propagated up the view hierarchy
Duplicate of this bug: 394994
This might be due to bug 376093; if someone wants to build with that patch backed out and test, feel free.
Scratch that; this is probably bug 358899 instead.
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)
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 ....
(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).
(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.
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.
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.
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?
Not sure, but I'll be posting a fix for this pretty soon.
(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.
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.
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: blocking1.9+ → blocking1.9-
Priority: P2 → --
Resolution: --- → WONTFIX
Alrighty, bug 405744 now covers the currently planned approach to fixing the original command-back/forward bug for Camino.
(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.
(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.
(Retitling to clarify the current state of the bug)
Summary: Dead-key commands (command-left/right, delete/backspace, escape, etc.) aren't propagated up the view hierarchy → Key events (delete/backspace, escape, etc.) aren't propagated up the view hierarchy
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.
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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.
(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).
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?
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!
(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?
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.
(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.
(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.
Duplicate of this bug: 432138
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.
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 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.
Attachment #403544 - Flags: review?(stuart.morgan+bugzilla) → review+
Comment on attachment 403544 [details] [diff] [review] camino-only fix v1.0 (1.9.0 branch) Approved for 22.214.171.124, a=dveditz
Attachment #403544 - Flags: approval126.96.36.199+
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
(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).
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?
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 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?
Attachment #403544 - Flags: approval188.8.131.52?
Comment on attachment 403544 [details] [diff] [review] camino-only fix v1.0 (1.9.0 branch) I'm fine with taking this on 184.108.40.206 on the same justification we had for 220.127.116.11.
Comment on attachment 403544 [details] [diff] [review] camino-only fix v1.0 (1.9.0 branch) a=LegNeato for 18.104.22.168. Please land this on mozilla-1.9.2 default. Code freeze is this Friday @ 11:59 pm PST.
Attachment #403544 - Flags: approval22.214.171.124? → approval126.96.36.199+
Landed on 188.8.131.52: 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.)
verified with: Camino Version 2.1a1pre (184.108.40.206pre 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
(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 (220.127.116.11pre 20100705001228) Note: With Firefox 3.6.6 (and Safari 5) it works fine there! Thanks and regards Mehmet
(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.
(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
There is a proper fix for this on trunk in bug 584965.
Assignee: nobody → joshmoz
Fixed on mozilla-central by the patch in bug 584965.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 11 years ago
Resolution: --- → FIXED
Fix was backed out because of regressions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 564152
Fixed (again) on mozilla-central by the patch in bug 584965.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.