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: