Closed Bug 306686 Opened 19 years ago Closed 19 years ago

/ does not start FAYT anymore

Categories

(SeaMonkey :: Find In Page, defect)

x86
All
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: aaronlev)

References

Details

(Keywords: helpwanted, regression)

Attachments

(2 files)

checkout finish: Mi Aug 31 22:11:43 CEST 2005 Linux gtk2 seamonkey, trunk Pressing / does not have any effect. That means I have no way to easily start typeaheadfind for text (as opposed to links). It may be worth nothing that / is Shift+7 on my keyboard. So maybe this is caused by bug 295228?
With today's SeaMonkey trunk build on Win32 the / starts the FAYT. Could it be GTK2 widget code that causes problems? For me pressing / the nsXBLPrototypeHandler::KeyEventMatched receives the code 0x2F ('/') and applying ToLowerCase to it does not change anything.
Actually it could be a problem. GTK widget code reports that Shift key is down and match in nsXBLPrototypeHandler::ModifiersMatchMask fails. Now that we take into account Shift key state it becomes important. It is problem of widget code. If modifier keys (Shift, Alt, Ctrl, Meta) together with other key produce different character than in non shifted state then widget code should NOT report this modifier key. In your case Shift+7 should only report '/' and not Shift+'/'. This is very similar to Win32 specific widget problem that I've fixed in bug 287179.
ok, then -> widget:gtk, I guess
Assignee: aaronleventhal → blizzard
Component: Keyboard: Find as you Type → Widget: Gtk
Flags: blocking1.9a1?
QA Contact: keyboard.fayt → gtk
Actually I can recreate the same problem on Windows if I install the German keyboard layout :( In previous note when I was talking about not reporting Shift state I was thinking that you refer to 7 on your numerical keyboard. About '7' in keyboard first row I am not so sure. I guess that Shift have to be reported for backwards compatibility. Is it acceptable to add duplicate rules in XBL that have Shift modifier declared for those shortcuts for which case is not important?
Component: Widget: Gtk → Keyboard: Find as you Type
OS: Linux → All
modifiers="any"?
(hmm, does that exist?)
That exists in XBL but this is a XUL key which doesn't really support it c.f. the Zoom In key which is both Ctrl++ and Ctrl+Shift++ just in case.
When I used: <handler event="keypress" key="/" command="cmd_findTypeText" modifiers="shift any"/> in content/xbl/builtin/browser-base.inc and: <key id="key_findTypeText" key="&findTypeTextCmd.key;" modifiers="shift any"/> <key id="key_findTypeLinks" key="&findTypeLinksCmd.key;" modifiers="shift any"/> in xpfe/communicator/resources/content/win/platformCommunicatorOverlay.xul the FAYT works as expected with German keyboard layout. The problem is that ' modifiers="shift any" ' should be added for both XPFE and Toolkit for all key non character key acceleretors. Is there some centralized place where can I find all shortcut combinations used by both toolkits?
I've meant non letter characters.
Mike, see comment 8.
Flags: blocking1.9a1? → blocking1.9a1+
Assignee: blizzard → aaronleventhal
QA Contact: gtk → keyboard.fayt
Does anyone know what caused this regression?
Keywords: helpwanted
Attached patch Fix for FAYT shortcut keys (deleted) — Splinter Review
Luckily there were not as many non-letter shortcut combinations that are handled by XBL bindings as I thought initially. By inspecting both Firefox and Mozilla help files seems that there are only those combinations: /,' - FAYT Ctrl++, Ctrl--, Cmd++, Cmd+- Cmd+. And only FAYT shortcuts are in XBL. As Neil said the "any" is supported only for XBL and not for XUL. Because bug 295228 changed only XBL behaviour, the XUL shortcuts should work fine. Seems that it is sufficient to change only content/xbl/builtin/browser-base.inc to fix the problem.
Attachment #194568 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 194568 [details] [diff] [review] Fix for FAYT shortcut keys I can't test this but it looks reasonable.
Attachment #194568 - Flags: superreview+
Attachment #194568 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #194568 - Flags: review?(aaronleventhal)
I can't find any docs on the "any" option of the modifiers attribute. It looks like we're saying shift + another modifier is required. Also, I have seen some times there are commas in between modifiers. Can someone point me to a document that explains this or just write it into this bug or to the developer.mozilla.org wiki? We need to get XUL planet to update their docs too. I can't r= this until I understand that.
Have not seen documentation for "any", but you can look at the source code of nsXBLPrototypeHandler::ConstructPrototype http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLPrototypeHandler.cpp#863 Everything that is before "any" is ignored by clearing the mask bits in higher nybble in mKeyMask. See for usage samples in: xpfe\global\resources\content\bindings\listbox.xml xpfe\global\resources\content\bindings\tree.xml Thus "control shift any" means that state of Ctrl and Shift keys is not important, but Alt and Meta should not be pressed. The comma is valid separator at least for XBL - see the nsXBLPrototypeHandler::ConstructPrototype again: char* token = nsCRT::strtok( str, ", \t", &newStr ); I fully agree that XulPlanet docs should be updated to match reality.
Comment on attachment 194568 [details] [diff] [review] Fix for FAYT shortcut keys Thanks for the explanation. I'll ping XUL Planet with it.
Attachment #194568 - Flags: review?(aaronleventhal) → review+
This is strictly speaking not related to the problem of FAYT, but it would be nice to have XUL "modifiers" to be consistent with XBL modifiers and allow not only space and comma as separators, but also the TAB character.
Attachment #194653 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #194653 - Flags: review?(aaronleventhal)
Comment on attachment 194653 [details] [diff] [review] Additional patch to allow TAB as separator for XUL nsMenuFrame I'm not the right person to review this -- try a layout peer. Also, we really need to file a separate bug on this issue and post the patch in there. Something like "Synchronize XUL and XBL handler syntax"
Attachment #194653 - Flags: review?(aaronleventhal)
Attachment #194653 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #194653 - Flags: review?(bzbarsky)
Attachment #194653 - Flags: review?(bzbarsky) → review+
Neil, are you going to check these in? I won't be able to until Tuesday; if they're not landed by then, please ping me?
looks like this was checked in at 2005-09-02 08:54 on trunk
*** Bug 306639 has been marked as a duplicate of this bug. ***
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: 295228
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: