229438 - Support the "Find" and "Help" keys on Sun keyboard

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Simford]

Steps to reproduce:
1) Press the "Find" key on a Sun keyboard that has that key
2) Press the "Help" key on a Sun keyboard that has that key

Actual reesults:
Nothing

Expected results:
1) Expected the result to be the same as when pressing Ctrl-F (Find in This
Page...".
2) expected the result to be the same as when pressing F1

Comment 1

[Simford]

Attachment: patch

Comment 2

[Simford]

Hi Timeless:

could you please take a look on this patch?
Thanks!

Comment 3

[timeless]

Comment on attachment 137978 [details] [diff] [review]
patch

it might be the case that the f19 stuff should be in a platform localized file,
but ...

Comment 4

[Kyle Yuan]

patch checked into 1.7b (trunk).

Comment 5

[Scott MacGregor]

I think this VKF19 stuff needs to use a solaris ifdef using the xul
pre-processor...It shouldn't end up in the chrome used by windows, osx, etc. my
two cents. 

Comment 6

[Kyle Yuan]

mscott, could you give us an example? 

As far as I know, the content and locale directory under xpfe/browser/resource
have 3 platform specific directories - mac, win, unix. Did you mean that we
should put all solaris related changes into the unix directory?

Comment 7

[Roland Mainz]

Scott MacGregor wrote:
> I think this VKF19 stuff needs to use a solaris ifdef using the xul
> pre-processor...

This assumes that both X client (Mozilla) and Xserver (Xsun/Solaris) reside on
the same machine - which is NOT true in such cases.
The correct solution would be to enable such stuff for all X11 platforms, not
only Solaris (just think about a Linux/x86 Mozilla displaying on a Xsun/Solaris
Xserver... then those keys should work, too. Using a Solaris-specific |ifdef|
would break that).

Comment 8

[Scott MacGregor]

This should either be moved to a platform specific XUL overlay file where we put
platform specific bindings or it needs to use the xul pre-processor..

i.e.

#ifdef XP_SOLARIS
  xul for 2nd key binding
#endif 

We shouldn't put platform specific key bindings in xul files shared by all
platforms like we did here I think.

Hopefully that clarifies the comment I was trying to make.

:)

Comment 9

[Roland Mainz]

Scott MacGregor wrote:
> This should either be moved to a platform specific XUL overlay file where we 
> put platform specific bindings or it needs to use the xul pre-processor..
> i.e.
>
> #ifdef XP_SOLARIS
>  xul for 2nd key binding
> #endif 

mscott:
Did you read my previous comment (comment #7) ?

|XP_SOLARIS| is __WRONG__.

The keys are specific to the Xserver being used. A mozilla binary running on
Linux can display on a Solaris Xserver and should therefore be able to use this.
This |ifdef| should ONLY disable the code if no X11 code is being build (e.g.
GTK+/Xlib/Qt etc.-ports should have this enabled).

Comment 10

[Scott MacGregor]

*sigh* I was just illustrating an example of HOW to use the xul pp which was
what I was asked about. "mscott, could you give us an example?"

I don't care what the ifdef is, just as long as these platform key bindings
aren't showing up in windows and OSX versions of the product. Or moved to a unix
bindings file.

Comment 11

[Roland Mainz]

mscott:
bryner said on IRC that the XUL preprocessor can use all defined from
"mozilla-config.h". If that works then a simple
|ifdef MOZ_X11| should be the correct way to |ifdef|-out all X11-specific stuff
(again, the Sun keys should be available for all X11 builds, not only for the
Solaris (see examples above - X11 applications are network transparent and
windows can be displayed between different platforms)).

Comment 12

[Roland Mainz]

Scott MacGregor wrote:
> *sigh*

... my comment was not thought as offense to anyone... I am just fearing of new
bugs in this area. I am hoping that most of these bugs have been fixed now and I
don't want new ones.

> I was just illustrating an example of HOW to use the xul pp which was
> what I was asked about. "mscott, could you give us an example?"

Those examples sometimes end up 1:1 in the patches... and I don't have my eyes
eveywhere...

Comment 13

[Kyle Yuan]

reopen to address mscott's comments

Comment 14

[Kyle Yuan]

->me

Comment 15

[Kyle Yuan]

Attachment: make keybindings platform specific

Comment 16

[Benjamin Smedberg]

Comment on attachment 142017 [details] [diff] [review]
make keybindings platform specific

I'm obviously not a peer of this code, so mscott has to review it, but the
preprocessor stuff looks fine.

Comment 17

[Scott MacGregor]

Comment on attachment 142017 [details] [diff] [review]
make keybindings platform specific

thanks for going the extra distance on this one!

I'm assuming roland is happy with using MOZ_X11 as the ifdef qualifier with my
sr.

Comment 18

[Kyle Yuan]

fix checked in.

Comment 19

[[e-mail]]

Oh no, I'm going to have to start making chrome to test trivial patches :-(

Comment 20

[Scott MacGregor]

Neil, would you prefer seamonkey to use an overlay to add this command instead
of leveraging the xul pre-processor for the effected files?

I suspect you are going to eventually be forced to do this anyway as more and
more seamonkey files are going to leverage the xul pre-processor over time...

Comment 21

[Boris Zbarsky [:bzbarsky]]

This patch almost certainly caused bug 236700.

As for the preprocessor, I still maintain that until we have build-time validity
checking working (sending the tree red if the post-processed file is not
well-formed, for example) we should keep its use to a minimum.  Oh, well.

Comment 22

[[e-mail]]

So, would anyone object if I wrote a patch that moved this keybinding into
unix/platformMailnewsOverlay.xul?

Comment 23

[Kyle Yuan]

I'll be glad to see that.

Comment 24

[[e-mail]]

Attachment: Move into platform overlay

Comment 25

[[e-mail]]

The keybinding is now in the platform overlay.

Comment 26

[[e-mail]]

Damn, I hadn't noticed that you'd also patched editorOverlay.xul all along :-/