Closed
Bug 399740
Opened 17 years ago
Closed 17 years ago
FAYT (find as you type) fails
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: phiw2, Assigned: masayuki)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
masayuki
:
review+
roc
:
superreview+
beltzner
:
approvalM9+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
Load any page.
Type '/' followed by some characters (e.g a word you see near top of a page).
Nothing happens.
Works correctly: Version 2007101000 (2.0a1pre)
Fails: 2007101101 (2.0a1pre)
Clean profile. Nothing to see in Console.log.10.4.10 ppc.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-10+00%3A00%3A00&maxdate=2007-10-11+01%3A00%3A00&cvsroot=%2Fcvsroot
Not sure what could have triggered this.
Minefield works (much) better although there is bug 399471 which seems a focus problem.
Maybe the one of the a11y bugs? I agree, nothing in that list looks very promising. :(
(If you enable autostart, nothing happens either, which sort-of rules out any dead-key handling issues.)
Reporter | ||
Comment 2•17 years ago
|
||
This is a regression from bug 385292.
I backed out that patch locally, and FAYT is working perfectly fine, both starting with '/' and with autostart turned on.
(hmmm, the about box says 2007101101, but that build landed on the download server at 11-Oct-2007 02:31.)
Blocks: 385292
Flags: blocking1.9?
(That build started at 01:42 11 Oct, according to tinderbox. We must just take the current hour and use it for the build ID/about box, no matter what point in that hour the build began.)
Assignee | ||
Comment 4•17 years ago
|
||
Why this is caused by bug 385292? Does Camino use the key codes for FAYT? If so, the implementation might be wrong.
So, FAYT works in Mac SeaMonkey trunk 2007-10-11-01 and is broken in 2007-10-12-01; this is clearly a Widget:Cocoa bug unless anyone has evidence of FAYT being broken in SeaMonkey on another platform.
Note that in SeaMonkey, the info pops up in the status bar ("Starting -- find text as you type") when you hit /, but any further key presses make FAYT stop ("Find stopped"). We don't even get the status bar info in Camino.
(SeaMonkey and Camino both use suitetypeaheadfind, fwiw.)
Assignee: nobody → joshmoz
Component: General → Widget: Cocoa
Product: Camino → Core
QA Contact: general → cocoa
Assignee | ||
Comment 6•17 years ago
|
||
http://lxr.mozilla.org/mozilla/source/widget/src/windows/nsWindow.cpp#3395
> 3395 DispatchKeyEvent(NS_KEY_PRESS, uniChars [cnt], 0, aKeyData, extraFlags);
> 3396 }
> 3397 } else
> 3398 DispatchKeyEvent(NS_KEY_PRESS, 0, DOMKeyCode, aKeyData, extraFlags);
hmm... o.k., on Windows, we are only sending the keycode if the characters is not printable. But cocoa doesn't do so.
Assignee | ||
Comment 7•17 years ago
|
||
This fixes this bug, and maybe, this is correct.
keyCode should be set non-zero value only at the key being "non-printable" key (e.g., enter and tab). And charCode should be zero then.
And some characters are not handled in GetGeckoKeyCodeFromChar, I'm adding them.
Comment on attachment 285250 [details] [diff] [review]
Patch rv1.0
+ return (0x20 <= aChar && aChar <= 0x7E) || 0xA0 <= aChar;
Please put the constants (0x20 and 0x7E) on the right side of the comparison.
Is it possible to arrange GetGeckoKeyCodeFromChar so that instead of listing out all of the characters we report 0 for they end up that way by default? Maybe put the code in the default case on top of the switch, then go through the ones that return a special gecko key code in the switch and default to 0?
Seems strange that we don't include the keycode for printable characters. Do we assume that for any printable character the event receiver can derive the keycode from the character?
Assignee | ||
Comment 9•17 years ago
|
||
(In reply to comment #8)
> Seems strange that we don't include the keycode for printable characters. Do we
> assume that for any printable character the event receiver can derive the
> keycode from the character?
If the event receiver uses the keycode for printable character, it might make intl bugs. It seems that the rule is a prevention of it. (So, I think that the event receiver should not guess the keycode from the printable character.)
Comment 10•17 years ago
|
||
http://developer.mozilla.org/en/docs/DOM:event.keyCode#Notes describes the way keycode and charcode are defined to interact in Gecko for keydown/up/press. Not doing that will definitely cause breakage. In particular, note that keydown and keyup don't behave the same way as keypress.
Comment 11•17 years ago
|
||
Is there any reason GetGeckoKeyCodeFromChar can't be implemented like this? This is a modified version of that function from your patch. This seems cleaner.
Assignee | ||
Comment 12•17 years ago
|
||
Josh:
If I did so, we cannot find bug 400355.
For, unknown cases, we should out the WARNING.
# But I agree to make simpler code.
Assignee | ||
Comment 13•17 years ago
|
||
s/out/output
Comment 14•17 years ago
|
||
Could the fix for bug 385292 have triggered another bug, where pressing enter causes the associated code to be executed twice? In the Browser, if I search (cmd+F) for text that doesn't appear in the document and hit enter (instead of clicking the Find button) I get the "Text not found" sheet twice. If I search in History and hit enter (instead of clicking the Find button) I get the "Search Results" window twice.
If you want I can file a new bug for it, but I'm kinda hoping this patch will fix it.
Comment 15•17 years ago
|
||
Hrm, I just tested, and this patch doesn't fix the "enter ''submits'' twice" problem.
Attachment #285250 -
Flags: superreview?(roc)
Attachment #285250 -
Flags: review?(joshmoz)
Attachment #285250 -
Flags: review+
Assignee | ||
Comment 16•17 years ago
|
||
oops, bad timing...
Josh:
this patch includes your sugestions, please check it.
# if it has some worngs, please change to r-.
the r+ is carried over, because the acutal logic is not changed.
Attachment #285250 -
Attachment is obsolete: true
Attachment #285618 -
Attachment is obsolete: true
Attachment #285723 -
Flags: superreview?(roc)
Attachment #285723 -
Flags: review+
Attachment #285250 -
Flags: superreview?(roc)
Attachment #285723 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 17•17 years ago
|
||
Comment on attachment 285723 [details] [diff] [review]
Patch rv1.1
This is a regression from previous milestone.
Attachment #285723 -
Flags: approval1.9?
Attachment #285723 -
Flags: approvalM9?
Comment 18•17 years ago
|
||
Comment on attachment 285723 [details] [diff] [review]
Patch rv1.1
a=endgame drivers for M9 landing
Attachment #285723 -
Flags: approvalM9?
Attachment #285723 -
Flags: approvalM9+
Attachment #285723 -
Flags: approval1.9?
Attachment #285723 -
Flags: approval1.9+
Assignee | ||
Comment 19•17 years ago
|
||
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 20•17 years ago
|
||
Comment on attachment 285723 [details] [diff] [review]
Patch rv1.1
switch (aChar)
{
...
+ case '-': return NS_VK_SUBTRACT;
+ case '+': return NS_VK_ADD;
default:
+ if (!IsPrintableChar(aChar))
+ NS_WARNING("GetGeckoKeyCodeFromChar has failed.");
+ return 0;
+ }
Nit: this is overindented. It should line up with the opening brace of the |switch|.
Reporter | ||
Comment 21•17 years ago
|
||
v.
Camino Version 2007102500 (2.0a1pre)
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•