Closed Bug 399740 Opened 17 years ago Closed 17 years ago

FAYT (find as you type) fails

Categories

(Core :: Widget: Cocoa, defect)

PowerPC
macOS
defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: phiw2, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.)
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.)
(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.)
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
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.
Attached patch Patch rv1.0 (obsolete) — Splinter Review
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.
Assignee: joshmoz → masayuki
Status: NEW → ASSIGNED
Attachment #285250 - Flags: review?(joshmoz)
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?
(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.)
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.
Attached file GetGeckoKeyCodeFromChar suggestion 1 (obsolete) —
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.
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.
s/out/output
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.
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+
Attached patch Patch rv1.1Splinter Review
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+
Comment on attachment 285723 [details] [diff] [review]
Patch rv1.1

This is a regression from previous milestone.
Attachment #285723 - Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Attachment #285723 - Flags: approvalM9?
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+
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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|.
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.

Attachment

General

Created:
Updated:
Size: