306686 - / does not start FAYT anymore

Status: RESOLVED FIXED

(SeaMonkey :: Find In Page, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 1

[Dainis Jonitis]

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.

Comment 2

[Dainis Jonitis]

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.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, then -> widget:gtk, I guess

Comment 4

[Dainis Jonitis]

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?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

modifiers="any"?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

(hmm, does that exist?)

Comment 7

[[e-mail]]

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.

Comment 8

[Dainis Jonitis]

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?

Comment 9

[Dainis Jonitis]

I've meant non letter characters.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Mike, see comment 8.

Comment 11

[Aaron Leventhal]

Does anyone know what caused this regression?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yes, see comment 0 and comment 2

Comment 13

[Dainis Jonitis]

Attachment: Fix for FAYT shortcut keys

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.

Comment 14

[[e-mail]]

Comment on attachment 194568 [details] [diff] [review]
Fix for FAYT shortcut keys

I can't test this but it looks reasonable.

Comment 15

[Aaron Leventhal]

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.

Comment 16

[Dainis Jonitis]

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 17

[Aaron Leventhal]

Comment on attachment 194568 [details] [diff] [review]
Fix for FAYT shortcut keys

Thanks for the explanation. I'll ping XUL Planet with it.

Comment 18

[Dainis Jonitis]

Attachment: Additional patch to allow TAB as separator for XUL nsMenuFrame

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.

Comment 19

[Aaron Leventhal]

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"

Comment 20

[Boris Zbarsky [:bzbarsky]]

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?

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

looks like this was checked in at 2005-09-02 08:54 on trunk

Comment 22

[Mats Palmgren (inactive)]

*** Bug 306639 has been marked as a duplicate of this bug. ***