Closed Bug 53667 Opened 24 years ago Closed 24 years ago

alt back arrow and alt forward broken

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: dennisle, Assigned: jag+mozbugs)

References

Details

(Keywords: regression, Whiteboard: [rtm+] Fix on trunk, reviewed, approved, fixes crasher 54694)

Attachments

(5 files)

[patch] This should fix alt arrows and some alt combinations
1013 bytes, patch
Details | Diff | Splinter Review
[patch](-uw) Fixes alt + arrows and some other stuff
4.48 KB, patch
Details | Diff | Splinter Review
[patch] same as above, now with whitespace fixes
6.01 KB, patch
Details | Diff | Splinter Review
[patch] Better patch, cleaning up some cruft.
4.88 KB, patch
Details | Diff | Splinter Review
[branch-patch] -u -w Diff between branch and fix.
2.31 KB, patch
Details | Diff | Splinter Review
the alt back arrow key combo worked yesterday but todays build it activates pull down menu QA semular to bug 53528 I think?
Sairuh, can you reproduce this?
Assignee: don → mcafee
Confirmed on Linux build 20000921.
Status: UNCONFIRMED → NEW
Ever confirmed: true
am currently unable to test the back and fwd accelerators (be it alt+arrow or ctrl+arrow) due to my window manager (methinks) setup. am using afterstep on rh 6.2 (with gnome). see my comments in bug 49407. haven't had time recently to clear up my wm stuff (if that's even the issue), sorry...
Keywords: qawanted
I've been seeing this bug on-and-off for several days now on the Linux nightlies. Currently I'm using build 2000092821 and the accelerators work fine.
I'm seeing this too. Also, for me, alt-right sometimes activates the Search menu... Very odd bug, this, not everyone is seeing it.
focus problem?
This worksforme on the trunk. Keeping open in case there's something else that triggers this to happen.
From bug 55438 (got confused, sowwy): ------- Additional Comments From Peter ``jag'' Annema 2000-10-06 14:36 ------- This appears to have regressed between 2000-09-19-08-M18 and 2000-09-19-21-M18. I suspect this has to do with the GTK key handling fixes akkana checked in around that time, except she's not seeing any of this. cc'ing her.
Updating summary, attaching patch, adding keywords.
Keywords: qawanted → patch, regression, review
Summary: alt back arrow broken today → alt back arrow and alt forward broken
nominating for rtm so I can check this into the trunk.
Keywords: rtm
*** Bug 55438 has been marked as a duplicate of this bug. ***
worksforme Linux build from Oct 7 17:55:43 CEST 2000.
BenB, with or without the patch? Neither my own builds (without the patch) nor nightly builds (latest tested is 2000100806-M18 on linux) work. Btw, on a nightly build, alt left opens the QA menu, alt right works. On my own build, alt right opens the search menu. I wonder what's different between those nightly builds, mine, and mcafee's / akkana's (which for them work as expected).
jag, without the patch attached here. Trunk, optimized, non-debug, built by myself. Both LeftAlt + LeftArrow and LeftAlt+RightArrow work.
Right, that's what mcafee and akkana are seeing. Hrm... My build is a trunk, optimized, debug one, though I think in optimized non-debug I saw the menus pop up too. Could you see what the nightly I tried does for you?
jag, I don't have that nightly. 2000-10-03: LeftAlt+LeftArrow opens the QA menu, LeftAlt+RightArrow goes forward.
Which glib? 1.2.8 here.
me too.
Ben Bucksch, can you test if my patch makes things worse for you? I think we still need r= and a= noted in this bug before rtm will even be considered.
jag, no change.
patch worksforme, r=mcafee. Note, things are workingforme without the patch too.
If things are working for you without the patch, then I'm inclined to minus this bug. Sairuh, does this problem happen for you?
don, 4 people have commented in this bug that they're seeing this. This patch returns the relevant part of the code back to the way it was before this regressed while leaving intact the fix part of the checkin that caused the regression. Apparantly, some machines behave differently than was expected in that fix, so we should really learn more about that. Until then, this patch provides a "good enough" work around.
The problem is that different versions of gtk have slightly different behavior when it comes to alt keys and unprintable keys, so although Chris and I aren't seeing it, other people will. We should take this change since it fixes it for the people who are seeing the problem, and doesn't hurt those of us who aren't.
Chris, can you get reviewers for this?
Priority: P3 → P2
Whiteboard: [rtm need info]
Chris sez this is r=mcafee Just need the sr now ...
Mind you, I don't consider this a true fix. The bug isn't well understood and this may just be a work around which happens to work (for now).
sr=alecf Let's get this onto the trunk at least.
Whiteboard: [rtm need info] → [rtm+]Fix in hand, reviewed and approved
cc'ing Pavlov. Pavlov, can you tell us what's going on here and whether this patch is the Right Way (tm)?
Keywords: review
I want better assurances given the above (confusing?) comments. Please get a clear explanation, and some feedback from the trunk (regression???) and then renominate. Comments from Pav?? rtm need info
Whiteboard: [rtm+]Fix in hand, reviewed and approved → [rtm need info]Fix in hand, reviewed and approved
checked patch into trunk.
I'm working with syd and Pavlov on a better patch.
Basically, everything above 0xf000 in gdkkeysyms.h is a non-printable, except those dealt with in the switch. Non-printables we want to deal with as keyCodes (so return 0), otherwise we return it. Yes, this code can be slightly optimized, but I'd like to get some comments first from people who know more about this :-)
I checked the latest patch by jag, and it fixes the segfault on solaris, too, see 54694. Axel
How about getting pavlov to r= and blizzard to a=? /be
I sent mail to pavlov asking for review.
looks good to me r=pavlov
I like this new patch MUCH more. Even a gtk-impared person like myself can see this is a major cleanup. The only trick is making sure we didn't just break IME on linux with this. Jag, can you play with the kinput2 and make sure you can still enter japanese characters? I think a good test case would be some HTML/JS that just takes Japanese characters from one <input type="text"> and dumps them into another. I'm thinking: <html> Foo: <input type="text" id="foo" onblur="document.getElementById('bar').value=this.value"><br> Bar: <inputtype="text" id="bar"> </html> All you'd have to do is use kinput2 to enter some characters into the first textfield, and make sure that the same ones appear int the second text field after you leave the first one.
jag is driving this, over to him.
Assignee: mcafee → disttsc
I've no clue at the moment how to get kinput2 up and running on Debian, and need to get some sleep right now. From what I can tell the codepath for IME hasn't changed, but yes, it would be good if someone did regression tests. Can someone help me out here?
r,mo=pavlov
Here's the kinput2 instructions http://www.mozilla.org/quality/intl/kinput2/kinput2-inst.html
URL: n/a
After much struggling: - I've got japanese characters to display in Mozilla -> Yay! - I've got kterm to let me enter japanese characters -> Yay! - Mozilla refuses to let me enter them I'll see if I can get some more help with this :-)
blizzard sr'd the 17284 attachment, I would suggest trying to make a final stab at this and opening a new bug re: kinput2 if needed.
yay, I feel a million times better about it if blizzard reviewed it :) thanks blizzard!
(I hope blizzard & everyone else knows that I was being serious, not sarcastic)
Okay, checked in that patch. This also fixes related bug 55792 (ctrl+arrow) and bug 54694 (crash on ctrl-left).
Marking rtm+, pdt, please read my previous comments and my comment in bug 54694 to understand why I'm marking this rtm+ (hint: fixes a crasher)
Status: NEW → ASSIGNED
Whiteboard: [rtm need info]Fix in hand, reviewed and approved → [rtm+] Fix on trunk, reviewed, approved, fixes crasher 54694
PDT says need info: Is this entire patch necessary in order to just stop the crash? cc bobj && ftang to look into apparently unresolved questions about JA character entry.
Whiteboard: [rtm+] Fix on trunk, reviewed, approved, fixes crasher 54694 → [rtm need info] Fix on trunk, reviewed, approved, fixes crasher 54694
PDT: I can bring this back to an patch (effectively +2 -9, not counting whitespace changes, comment lines or adding/removing '{' and '}'). I'll attach that patch here. That is what's minimally needed to stop the crashing "The Right Way (tm)" with the added (inherent) bonus of fixing the alt/ctrl arrow problems. If that's still too much, please take the last patch (+2 -2) akkana attached to bug 54694 (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=15854). It gets the job done, just less cleanly.
Btw, the patch to the trunk was a bit larger because I had to undo a first-attempt-patch. Also, after analyzing the patch to the branch, I can say (and show the proof if so desired) effectively it's a +1 -5 patch, where the real change is letting the |isprint| checks be done by code more suitable for it (|keysym2ucs|). I now also more confidently dare say that my patch hasn't changed the IME ("JA input") path. ftank, bobj, can you confirm this and put rtm+ back in the whiteboard?
Alec, Thanks for the heads up on potential IME regressions. Cc'ing the Sun guys who have done much of the Mozilla Unix IME work: tajima@eng.Sun.COM, katakai@japan.sun.com Toshi and Masaki, Please review ASAP. Also cc'ing our resident UNIX I18N experts: bstell@netscape.com and erik@netscape.com. Cc'ing our resident Unix IQA expert: ji@netscape.com Xianglan, Will you please you test Linux IME using the trunk build? Can someone provide a local branch build with the patch for Xianglan to test?
Tested 2000101909-Mtrunk build. Japanese IME is not broken there. I used the test case that Alec provided and I also used our own Japanese testing form. It's working fine.
Making this rtm+ again. PDT, we've got confirmation Japanese IME works just fine.
Whiteboard: [rtm need info] Fix on trunk, reviewed, approved, fixes crasher 54694 → [rtm+] Fix on trunk, reviewed, approved, fixes crasher 54694
I had a look at the patch. It is in a very central part of the key event handling code, so it's very sensitive. For the Netscape branch, at this point in time, I would recommend taking the patch that stops the crash (54694). If the PDT believes that Alt-RightArrow is important enough to fix in the branch at this point in time, then we need to get someone more knowledgeable to review the code not only for English and Japanese, but also for German, Russian, etc. Toshi and Masaki, have you had a look at the patch?
For what it's worth, I've attached a new patch to bug 54694, which prevents the crash (dereferencing a null pointer), and only that. Take akkana's patch in that bug if you also want to fix the arrow key problems (the check for keyval<256 does that). Yes, it's that simple, no, it's not really the right fix, but it'll do for branch (low risk, high impact).
rtm-. Since this change is in a sensitive area, and marketing isn't going to shoot us if we don't fix this (and they will shoot us if we slip), we have to minus this for rtm.
Fixed
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
To clarify: Fixed on the trunk, not going in on the branch.
vrfy fixed on linux using a mozilla trunk build i downloaded last week at home [2000.11.08.08]. sorry 'bout the delay. also, as a doublecheck, i vrfy'd that alt+left [back] and alt+right [fwd] also [still] work on winnt, as well as cmd+left and cmd+right on the mac [using 2000.11.13.08 from the comm trunk].
Status: RESOLVED → VERIFIED
It now works right on the machine, I first saw the problem on. now using Build 2000101815
Blocks: 60740
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: