562195 - Shortcuts do not work in Qt Fennec

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Steffen Imhof]

Attachment: Patch to generate better keypress events

Shortcuts like Ctrl-T to open a new tab do not work in Fennec if built with the Qt backend.

The reason is that the charCode for the keypress event is generated from the text value of the Qt key event, which is non-printable for combinations with Ctrl.

The fallback is to use the keycode as the charcode, but the keycode corresponds to the charcode of the upper case letter which trips up Fennec (Firefox seems to handle the shortcuts differently)

The attached patch, checks the shift state during event generation and upper-/lowercases the fallback charcode so that the generated event matches those from the GTK backend more closely.

Comment 1

[Doug Turner (:dougt)]

this is really terrible.  Is there a Qt bug filed for this issue?

Comment 2

[Jeremias Bosch (:jbos)]

Mhm i don't know if its a bug. Regarding to http://doc.trolltech.com/4.6/qkeyevent.html#key  is meant to work that way. Actually text() could be used instate of key(), but only in cases where no ctrl,... is involved - which it is, in case you want to use a shortcut

Comment 3

[Mike Kristoffersen (:MikeK)]

FWIW: Did a little investigation on this today testing on PC builds, when doing a CTRL-T:

In gtk2 we send:
event.charCode = 0x74
event.keyCode = 0x0

And in Qt:
event.charCode = 0x0
event.keyCode = 0x54
(and what I assume is the CTRL key it self with keyCode 0x11)

I guess this is what the patch tries to fix...

Another interesting thing is that in gtk2 we actually fill out the event.alternativeCharCodes structure - which is documented to contain:

  // OS translated Unicode chars which are used for accesskey and accelkey
  // handling. The handlers will try from first character to last character.

Could it be that we should also fill out this structure in Qt builds?

Comment 4

[Jeremias Bosch (:jbos)]

We should, in order to support non latin text. We had fixes for the key/char code mixup. I'm not sure if they are went as patch already in bugzilla. but this is fixed on our side.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 441956 [details] [diff] [review]
Patch to generate better keypress events

is this really still needed? on my n900 (1.2), i do not see any need.

Comment 6

[Lutz Schönemann]

Attachment: Patch to improve Qt-event.

The attached patch fixes some key event issues and matches the behavior of the Gtk Firefox more closely.

Shortcuts like Ctrl-T work by using the keycode as charcode.

Now the KeyPress event is suppressed for modifier keys (i.e. Shift/Ctrl/Meta/Alt/AltGr). Firefox Gtk tries to implement the KeyPress event described on the MSDN sites but only suppresses the KeyPress events for Shift/Ctrl/Alt/Meta. Should all KeyPress events be suppressed if they don't contain printable characters?

An alternative CharCode is appended with the upper and lower character. If the printable character is a non Latin character the Xkeymap is used to find a Latin character that will be appended as alternative CharCode.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 449598 [details] [diff] [review]
Patch to improve Qt-event. 

masayuki, could you provide your feedback here?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 449598 [details] [diff] [review]
Patch to improve Qt-event. 

# Please use |hg diff -U 8 -p|.

>diff --git a/widget/src/qt/nsWindow.cpp b/widget/src/qt/nsWindow.cpp
>@@ -1400,19 +1415,99 @@
> 
>         nsEventStatus status = DispatchEvent(&downEvent);
> 
>+        // DispatchEvent can Destroy us (bug 378273)
>+        if (status && NS_UNLIKELY(mIsDestroyed)) {
>+            qDebug() << "Returning[" << __LINE__ << "]: " << "Window destroyed";
>+            return status;
>+        }
>+

Probably, this is wrong. You shouldn't check the status. You should check only mIsDestroyed.

>         // If prevent default on keydown, do same for keypress
>         if (status == nsEventStatus_eConsumeNoDefault)
>             setNoDefault = PR_TRUE;
>     }
> 
>+    // Don't pass modifiers as NS_KEY_PRESS events.
>+    //       Instead of selectively excluding some keys from NS_KEY_PRESS events,
>+    //       we instead selectively include (as per MSDN spec
>+    //       ( http://msdn.microsoft.com/en-us/library/system.windows.forms.control.keypress%28VS.71%29.aspx );
>+    //       no official spec covers KeyPress events).
>+    if (aEvent->key() == Qt::Key_Shift
>+        || aEvent->key() == Qt::Key_Control
>+        || aEvent->key() == Qt::Key_Meta
>+        || aEvent->key() == Qt::Key_Alt
>+        || aEvent->key() == Qt::Key_AltGr) { // */
>+/*    if (!( (aEvent->key() >= Qt::Key_Space && aEvent->key() <= Qt::Key_QuoteLeft) // 0x20 <= aEvent->key() <= 0x60
>+        || (aEvent->key() >= Qt::Key_BraceLeft && aEvent->key() >= Qt::Key_AsciiTilde) // 0x7b <= aEvent->key() <= 0x7e
>+        || (aEvent->key() >= Qt::Key_nobreakspace && aEvent->key() >= Qt::Key_ssharp) // 0x0a0 <= aEvent->key() <= 0x0df
>+        || (aEvent->key() == Qt::Key_division) // 0x0f7 == aEvent->key()
>+        || (aEvent->key() == Qt::Key_ydiaeresis) // 0x0ff == aEvent->key()
>+        )) { // */
>+        qDebug() << "Returning[" << __LINE__ << "]: " << "ModifierKeyEvent " << aEvent->key();
>+        if (setNoDefault) {
>+            return nsEventStatus_eConsumeNoDefault;
>+        }
>+        else {
>+            return nsEventStatus_eIgnore;
>+        }

return setNoDefault ?
    nsEventStatus_eConsumeNoDefault :
    nsEventStatus_eIgnore;

>+    }
>+
>     nsKeyEvent event(PR_TRUE, NS_KEY_PRESS, this);
>     InitKeyEvent(event, aEvent);
> 
>-    event.charCode = domCharCode;
>-    event.keyCode = domCharCode ? 0 : domKeyCode;
>+    // If prevent default on keydown, do same for keypress
>+    if (setNoDefault) {
>+        event.flags |= NS_EVENT_FLAG_NO_DEFAULT;
>+    }

Um... I don't think this is correct, but gtk2 also does... So, this should be Okay...

>-    if (setNoDefault)
>-        event.flags |= NS_EVENT_FLAG_NO_DEFAULT;
>+    // If there is not charcode attainable from the text, try to
>+    // generate it from the keycode. Check shift state for case
>+    if( ! domCharCode ) {
>+        if ( QApplication::keyboardModifiers() & Qt::ShiftModifier )
>+            domCharCode = (PRUint32) QChar::toUpper( (uint)aEvent->key() );
>+        else
>+            domCharCode = (PRUint32) QChar::toLower( (uint)aEvent->key() );
>+        if(! QChar((uint)domCharCode).isPrint())
>+            domCharCode = 0;
>+    }

I think that you need to check CapsLock and ShiftLock state. But I'm not sure whether it's possible on qt.

And use { } when a block has only one line.
https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_Structures

I have a question, is this really needed in normal cases? I guess that you don't need to check domCharCode on next if condition, and you should compute the charCode in the block if it's zero.

>+    if(domCharCode) {

I don't think this is correct.

This should be:

if (domCharCode && (isCtrl || isAlt || isMeta) &&
    !(aEvent->text().length() > 0 && aEvent->text()[0].isPrint()))

The alternative char codes are needed only when the key event has modifier keys and that isn't for text input.

>+        event.charCode = domCharCode;
>+        event.keyCode = 0;
>+        nsAlternativeCharCode altCharCode(0, 0);
>+
>+        altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
>+        altCharCode.mShiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );

You need to check the CapsLock and ShiftLock too.

And cannot you get them by low level APIs on Qt? E.g., this is wrong on Hebrew keyboard layout.
http://en.wikipedia.org/wiki/Hebrew_keyboard

And are QChar::toUpper() and QChar::toLower() usable for non-Latin alphabets? E.g., Cyrillic.
# But even if the answer is NO, we cannot solve this problem, probably.

>+        // append alternative char code to event
>+        if (altCharCode.mUnshiftedCharCode || altCharCode.mShiftedCharCode) {
>+            event.alternativeCharCodes.AppendElement(altCharCode);
>+        }

If the key is from a to z and both unshifted char and shifted char is same as original char of aEvent, we don't need to append it. Otherwise, if the unshifted char and shifted char are same as domCharCode, we also don't need to append it.
See nsWindow of Windows.
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5879

>+
>+#ifdef MOZ_X11
>+        // check if one of the alternative char codes is latin-1
>+        if ( altCharCode.mUnshiftedCharCode > 0xFF && altCharCode.mShiftedCharCode > 0xFF ) {

&& should be ||.

>+            // find latin char for keycode
>+            KeySym keysym = NoSymbol;
>+            int i = 0;
>+            do {  // find first Latin-Char in XKeyMap
>+                keysym = XKeycodeToKeysym(display, aEvent->nativeScanCode(), i);
>+                if(keysym <= 0xFF)
>+                    break;

Use {}

>+            } while(keysym != NoSymbol && i++ <= 255);
>+
>+            altCharCode.mUnshiftedCharCode = (PRUint32) QChar((uint) keysym).toLower().toLatin1();
>+            altCharCode.mShiftedCharCode = (PRUint32) QChar((uint) keysym).toUpper().toLatin1();
>+
>+            if ( altCharCode.mUnshiftedCharCode || altCharCode.mShiftedCharCode ) {
>+                event.alternativeCharCodes.AppendElement(altCharCode);
>+            }
>+        }
>+#endif
>+    }
>+    else {
>+        event.charCode = domCharCode;
>+        event.keyCode = domCharCode ? 0 : domKeyCode;
>+    }
> 
>     // send the key press event
>     return DispatchEvent(&event);

The event's modifier keys are set real state in InitKeyEvent(). However, this is wrong. If charCode is not zero, i.e., if the keypress event should cause a text input, Ctrl/Alt/Meta flags should be cleared. E.g., AltGr + something. Because nsPlainttextEditor and nsHTMLEditor ignores the keypress event pressed Ctrl or Alt or Meta keys even if it has charCode.

Comment 9

[Lutz Schönemann]

(In reply to comment #8)

> >+    // If there is not charcode attainable from the text, try to
> >+    // generate it from the keycode. Check shift state for case
> >+    if( ! domCharCode ) {
> >+        if ( QApplication::keyboardModifiers() & Qt::ShiftModifier )
> >+            domCharCode = (PRUint32) QChar::toUpper( (uint)aEvent->key() );
> >+        else
> >+            domCharCode = (PRUint32) QChar::toLower( (uint)aEvent->key() );
> >+        if(! QChar((uint)domCharCode).isPrint())
> >+            domCharCode = 0;
> >+    }
> 
> I think that you need to check CapsLock and ShiftLock state. But I'm not sure
> whether it's possible on qt.

This code creates a CharCode from the Keycode. It is needed if the user holds the Ctrl-key while typing. In that case QKeyEvent::text() returns nothing because the result of that event is a non-printable character.
You are right that CapsLock and ShiftLock state must be checked but I haven't found any function in Qt that returns that information.

> I have a question, is this really needed in normal cases? I guess that you
> don't need to check domCharCode on next if condition, and you should compute
> the charCode in the block if it's zero.
> 
> >+    if(domCharCode) {

I think currently it is needed to check domCharCode because some keys produce KeyCodes but no printable CharCodes but are no modification keys (e.g. Backspace, Return)

> 
> I don't think this is correct.
> 
> This should be:
> 
> if (domCharCode && (isCtrl || isAlt || isMeta) &&
>     !(aEvent->text().length() > 0 && aEvent->text()[0].isPrint()))
> 
> The alternative char codes are needed only when the key event has modifier keys
> and that isn't for text input.

What if a website reacts on single key presses to control or navigate the website? Shouldn't we send an corresponding alternativeCharCode in case the pressed key doesn't produce a ASCII/Latin Character?

> 
> >+        event.charCode = domCharCode;
> >+        event.keyCode = 0;
> >+        nsAlternativeCharCode altCharCode(0, 0);
> >+
> >+        altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
> >+        altCharCode.mShiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );
> 
> You need to check the CapsLock and ShiftLock too.
> 
> And cannot you get them by low level APIs on Qt? E.g., this is wrong on Hebrew
> keyboard layout.
> http://en.wikipedia.org/wiki/Hebrew_keyboard

Can you explain in more detail what is wrong on Hebrew keyboards?

> 
> And are QChar::toUpper() and QChar::toLower() usable for non-Latin alphabets?
> E.g., Cyrillic.

From Qt documentation: "[...] toUpper() and toLower() will do case changes when the character has a well-defined uppercase/lowercase equivalent."
So I assume that Qts toUpper/toLower functions are usable for non-Latin characters.

> # But even if the answer is NO, we cannot solve this problem, probably.
> 
> >+        // append alternative char code to event
> >+        if (altCharCode.mUnshiftedCharCode || altCharCode.mShiftedCharCode) {
> >+            event.alternativeCharCodes.AppendElement(altCharCode);
> >+        }
> 
> If the key is from a to z and both unshifted char and shifted char is same as
> original char of aEvent, we don't need to append it. Otherwise, if the
> unshifted char and shifted char are same as domCharCode, we also don't need to
> append it.
> See nsWindow of Windows.
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5879

You're right. If nothing has changed after all the computation we don't need to append the alternativeCharCodes

> 
> >+
> >+#ifdef MOZ_X11
> >+        // check if one of the alternative char codes is latin-1
> >+        if ( altCharCode.mUnshiftedCharCode > 0xFF && altCharCode.mShiftedCharCode > 0xFF ) {
> 
> && should be ||.

I know that gtk2 sets shiftet and unshiftet to Latin chars. But is it needed that both characters are Latin characters? Isn't it sufficient if one of that chars is a Latin code?


> >+            } while(keysym != NoSymbol && i++ <= 255);
> >+
> >+            altCharCode.mUnshiftedCharCode = (PRUint32) QChar((uint) keysym).toLower().toLatin1();
> >+            altCharCode.mShiftedCharCode = (PRUint32) QChar((uint) keysym).toUpper().toLatin1();
> >+
> >+            if ( altCharCode.mUnshiftedCharCode || altCharCode.mShiftedCharCode ) {
> >+                event.alternativeCharCodes.AppendElement(altCharCode);
> >+            }
> >+        }
> >+#endif
> >+    }
> >+    else {
> >+        event.charCode = domCharCode;
> >+        event.keyCode = domCharCode ? 0 : domKeyCode;
> >+    }
> > 
> >     // send the key press event
> >     return DispatchEvent(&event);
> 
> The event's modifier keys are set real state in InitKeyEvent(). However, this
> is wrong. If charCode is not zero, i.e., if the keypress event should cause a
> text input, Ctrl/Alt/Meta flags should be cleared. E.g., AltGr + something.
> Because nsPlainttextEditor and nsHTMLEditor ignores the keypress event pressed
> Ctrl or Alt or Meta keys even if it has charCode.

I don't understand the reason to clear out the modifier keys. Can you explain that in more detail?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI:

See also https://developer.mozilla.org/en/Gecko_Keypress_Event
This is document of "key hell".

(In reply to comment #9)
> You are right that CapsLock and ShiftLock state must be checked but I haven't
> found any function in Qt that returns that information.

Sigh... It's very serious for i18n.

> > I have a question, is this really needed in normal cases? I guess that you
> > don't need to check domCharCode on next if condition, and you should compute
> > the charCode in the block if it's zero.
> > 
> > >+    if(domCharCode) {
> 
> I think currently it is needed to check domCharCode because some keys produce
> KeyCodes but no printable CharCodes but are no modification keys (e.g.
> Backspace, Return)

You should domCharCode if its value is not printable like BackSpace and Return. Such keypress events' charCode should be zero. And at that time, current code doesn't set domCharCode if the character isn't printable.

> > I don't think this is correct.
> > 
> > This should be:
> > 
> > if (domCharCode && (isCtrl || isAlt || isMeta) &&
> >     !(aEvent->text().length() > 0 && aEvent->text()[0].isPrint()))
> > 
> > The alternative char codes are needed only when the key event has modifier keys
> > and that isn't for text input.
> 
> What if a website reacts on single key presses to control or navigate the
> website? Shouldn't we send an corresponding alternativeCharCode in case the
> pressed key doesn't produce a ASCII/Latin Character?

Oops, the condition is wrong, I meant:

if ((isCtrl || isAlt || isMeta) &&
    !(aEvent->text().length() > 0 && aEvent->text()[0].isPrint()))

And you should move

>+    // If there is not charcode attainable from the text, try to
>+    // generate it from the keycode. Check shift state for case
>+    if( ! domCharCode ) {
>+        if ( QApplication::keyboardModifiers() & Qt::ShiftModifier )
>+            domCharCode = (PRUint32) QChar::toUpper( (uint)aEvent->key() );
>+        else
>+            domCharCode = (PRUint32) QChar::toLower( (uint)aEvent->key() );
>+        if(! QChar((uint)domCharCode).isPrint())
>+            domCharCode = 0;
>+    }

to this block.

alternativeCharCode is only needed for finding a registered accesskey or shortcut key *in* Gecko (script cannot access it). All accesskeys need Alt key. And all shortcut keys should need one or more modifier keys. So, If a keypress event is without any modifier keys or a keypress event doesn't cause text input, alternativeCharCode isn't needed.

> > >+        event.charCode = domCharCode;
> > >+        event.keyCode = 0;
> > >+        nsAlternativeCharCode altCharCode(0, 0);
> > >+
> > >+        altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
> > >+        altCharCode.mShiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );
> > 
> > You need to check the CapsLock and ShiftLock too.
> > 
> > And cannot you get them by low level APIs on Qt? E.g., this is wrong on Hebrew
> > keyboard layout.
> > http://en.wikipedia.org/wiki/Hebrew_keyboard
> 
> Can you explain in more detail what is wrong on Hebrew keyboards?

Hebrew keyboard layout switches Hebrew characters and Upper Latin alphabets by shift key. So, event if domCharCode is 'A', the actual unshifted character is Hebrew's.

> > >+#ifdef MOZ_X11
> > >+        // check if one of the alternative char codes is latin-1
> > >+        if ( altCharCode.mUnshiftedCharCode > 0xFF && altCharCode.mShiftedCharCode > 0xFF ) {
> > 
> > && should be ||.
> 
> I know that gtk2 sets shiftet and unshiftet to Latin chars. But is it needed
> that both characters are Latin characters? Isn't it sufficient if one of that
> chars is a Latin code?

Yes, it's needed. The Latin characters might match to accesskeys (both UI and web pages). E.g., An user who use non-Latin keyboard layout may want to access both <button accesskey="A"> and <button accesskey="native character">.

> > >     // send the key press event
> > >     return DispatchEvent(&event);
> > 
> > The event's modifier keys are set real state in InitKeyEvent(). However, this
> > is wrong. If charCode is not zero, i.e., if the keypress event should cause a
> > text input, Ctrl/Alt/Meta flags should be cleared. E.g., AltGr + something.
> > Because nsPlainttextEditor and nsHTMLEditor ignores the keypress event pressed
> > Ctrl or Alt or Meta keys even if it has charCode.
> 
> I don't understand the reason to clear out the modifier keys. Can you explain
> that in more detail?

bug 414130 is the example. Some characters can be inputted only with Ctrl or Alt key on some keyboard layout. If you set such modifier keys to the keypress event, the event will be ignored by editors.

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsPlaintextEditor.cpp#389
> 363 NS_IMETHODIMP nsPlaintextEditor::HandleKeyPress(nsIDOMKeyEvent* aKeyEvent)
> 364 {
> 
> 389     if (character && !altKey && !ctrlKey && !metaKey)
> 390     {
> 391       aKeyEvent->PreventDefault();
> 392       nsAutoString key(character);
> 393       return TypedText(key, eTypedText);
> 394     }

Comment 11

[Lutz Schönemann]

Attachment: Patch to improve Qt-event.

The new patch only works correctly with X11. It now checks the CapsLock/ShiftLock state. Also Hebrew layout should work.

The patch copies two files from widgets/src/gtk2 to widgets/src/qt. These sources provide a mapping from X11s KeySym to Unicode that is needed to lookup CharCodes

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 451529 [details] [diff] [review]
 Patch to improve Qt-event.

>diff --git a/widget/src/gtk2/keysym2ucs.c b/widget/src/qt/keysym2ucs.c
>copy from widget/src/gtk2/keysym2ucs.c
>copy to widget/src/qt/keysym2ucs.c
>diff --git a/widget/src/gtk2/keysym2ucs.h b/widget/src/qt/keysym2ucs.h
>copy from widget/src/gtk2/keysym2ucs.h
>copy to widget/src/qt/keysym2ucs.h

Cannot share the files? E.g., cannot move them to xpwidget and only builds them at gtk2 or qt?

I think roc should decide what is best.

>diff --git a/widget/src/qt/nsWindow.cpp b/widget/src/qt/nsWindow.cpp
>--- a/widget/src/qt/nsWindow.cpp
>+++ b/widget/src/qt/nsWindow.cpp
>@@ -60,16 +60,21 @@
>+#include "keysym2ucs.h"

Shouldn't be in #ifdef MOZ_X11?

>@@ -1365,59 +1372,326 @@
>+#ifdef MOZ_X11
>+    // get keymap and modifier map from the Xserver
>+    Display *display = QX11Info::display();
>+    int x_min_keycode = 0, x_max_keycode = 0, xkeysyms_per_keycode;
>+    XDisplayKeycodes(display, &x_min_keycode, &x_max_keycode);
>+    XModifierKeymap *xmodmap = XGetModifierMapping(display);
>+    KeySym *xkeymap = XGetKeyboardMapping(display, x_min_keycode, x_max_keycode - x_min_keycode,
>+                                          &xkeysyms_per_keycode);
>+    
>+    // create modifier masks
>+    qint32 shift_lock_mask = 0, caps_lock_mask = 0, num_lock_mask = 0;
>+    
>+    for(int i = 0; i < 8 * xmodmap->max_keypermod; ++i) {
>+        qint32 maskbit = 1 << (i / xmodmap->max_keypermod);
>+        KeyCode modkeycode = xmodmap->modifiermap[i];
>+        if(modkeycode == NoSymbol) {
>+            continue;
>+        }
>+        
>+        quint32 mapindex = (modkeycode - x_min_keycode) * xkeysyms_per_keycode;
>+        for(int j = 0; j < xkeysyms_per_keycode; ++j) {
>+            KeySym modkeysym = xkeymap[mapindex + j];
>+            switch(modkeysym) {
>+                case XK_Num_Lock:
>+                    num_lock_mask |= maskbit;
>+                    break;
>+                case XK_Caps_Lock:
>+                    caps_lock_mask |= maskbit;
>+                    break;
>+                case XK_Shift_Lock:
>+                    shift_lock_mask |= maskbit;
>+                    break;
>+            }
>+        }
>+    }

Unfortunately, I'm not familiar with X11 programing, so, you should request review to another person. (maybe, karlt?)

>+    // now we can update the shift_state
>+    shift_state = shift_state ^ (bool)((caps_lock_mask | shift_lock_mask) & aEvent->nativeModifiers()) ?
>+        PR_TRUE :
>+        PR_FALSE;

I think that this is wrong. When CapsLock is locked, it's affected only to alphabet keys, not all keys.

>+    // try to find a keysym that we can translate to a DOMKeyCode
>+    // this is needed because some of Qt's keycodes cannot be translated
>+    // TODO: use US keyboard keymap instead of localised keymap
>+    if(! domKeyCode &&
>+      aEvent->nativeScanCode() >= (quint32)x_min_keycode &&
>+      aEvent->nativeScanCode() <= (quint32)x_max_keycode) {
>+        int index = (aEvent->nativeScanCode() - x_min_keycode) * xkeysyms_per_keycode;

strange whitespaces...

>if (!domKeyCode &&
>    aEvent->nativeScanCode() >= (quint32)x_min_keycode &&
>    aEvent->nativeScanCode() <= (quint32)x_max_keycode) {
>    int index = (aEvent->nativeScanCode() - x_min_keycode) * xkeysyms_per_keycode;

>+        if (status == nsEventStatus_eConsumeNoDefault) {
>             setNoDefault = PR_TRUE;
>+        }

FYI: I'm not sure whether this is right thing. But gtk2's nsWindow behaves as same. So, it's not matter for now.

>+    // Don't pass modifiers as NS_KEY_PRESS events.
>+    //       Instead of selectively excluding some keys from NS_KEY_PRESS events,
>+    //       we instead selectively include (as per MSDN spec
>+    //       ( http://msdn.microsoft.com/en-us/library/system.windows.forms.control.keypress%28VS.71%29.aspx );
>+    //       no official spec covers KeyPress events).
>+    if (aEvent->key() == Qt::Key_Shift   ||
>+        aEvent->key() == Qt::Key_Control ||
>+        aEvent->key() == Qt::Key_Meta    ||
>+        aEvent->key() == Qt::Key_Alt     ||
>+        aEvent->key() == Qt::Key_AltGr) { // */
>+/*    if (!( (aEvent->key() >= Qt::Key_Space && aEvent->key() <= Qt::Key_QuoteLeft) // 0x20 <= aEvent->key() <= 0x60
>+        || (aEvent->key() >= Qt::Key_BraceLeft && aEvent->key() >= Qt::Key_AsciiTilde) // 0x7b <= aEvent->key() <= 0x7e
>+        || (aEvent->key() >= Qt::Key_nobreakspace && aEvent->key() >= Qt::Key_ssharp) // 0x0a0 <= aEvent->key() <= 0x0df
>+        || (aEvent->key() == Qt::Key_division) // 0x0f7 == aEvent->key()
>+        || (aEvent->key() == Qt::Key_ydiaeresis) // 0x0ff == aEvent->key()
>+        )) { // */
>+
>+        return setNoDefault ?
>+            nsEventStatus_eConsumeNoDefault :
>+            nsEventStatus_eIgnore;
>+    }
>+//    else {
>+//        qDebug() << "No key down event [" << __LINE__ << "]: isAutoRepeat(" << aEvent->isAutoRepeat() << ") IsKeyDown(" << IsKeyDown(domKeyCode) << ")";
>+//    }
>+

Please remove the commented out lines from final version of your patch.

>+    if( (! domCharCode) &&
>+        (QApplication::keyboardModifiers() & (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier)) ) {

Please use our coding style here too.

>+        // first try: get a character from keyCode
>+        if ( shift_state ) {
>+            domCharCode = (PRUint32) QChar::toUpper( (uint)aEvent->key() );
>+        } else {
>+            domCharCode = (PRUint32) QChar::toLower( (uint)aEvent->key() );
>+        }

You can check whether the upper character and the lower character are same character or not. If they are different, you can honor the CapsLock state if it's locked.

>+        // if Ctrl is pressed and domCharCode is not a ASCII character
>+        if( domCharCode > 0xFF && (QApplication::keyboardModifiers() & Qt::ControlModifier) ) {
>+            qDebug() << "Ctrl is pressed so we need a latin character";
>+            // replace Unicode character
>+            int index = (aEvent->nativeScanCode() - x_min_keycode) * xkeysyms_per_keycode;
>+            for( int i = 0; i < xkeysyms_per_keycode; ++i ) {
>+                if( xkeymap[index + i] <= 0xFF ) {
>+                    domCharCode = (PRUint32) QChar::toLower((uint) xkeymap[index + i]);

You should check shift key state here.

>+                    break;
>+                }
>+            }
>+            qDebug() << "domCharCode is now: " << domCharCode;
>+            qDebug();
>+        }
>+#endif
>+    } else { // character is printable so we have to unset modifiers
>+        //event.isShift = PR_FALSE;
>+        event.isControl = PR_FALSE;
>+        event.isAlt = PR_FALSE;
>+        event.isMeta = PR_FALSE;
>+    }
>+    
>+    // append alternativeCharCodes if modifier is pressed
>+    // append an additional alternativeCharCodes if domCharCode is not a Latin character
>+    // and if one of these modifiers is pressed (i.e. Ctrl, Alt, Meta)
>+    if(domCharCode &&
>+        (QApplication::keyboardModifiers() & (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier))) {
>+        
>+        QChar originChar(domCharCode);
>+    
>+        event.charCode = domCharCode;
>+        event.keyCode = 0;
>+        nsAlternativeCharCode altCharCode(0, 0);
>+
>+        if(originChar.isUpper() || originChar.isLower()) { // if character has a lower and upper representation
>+            if( shift_state ) {
>+                altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );
>+                altCharCode.mShiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
>+            } else {
>+                altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
>+                altCharCode.mShiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );
>+            }

I worry about the condition. If the originalChar is an alphabet but the other shift state character of the key is different character like Hebrew keyboard layout, this sets wrong charcodes.


This patch is really complicated. Would you separate the X11 part to a simple class? E.g., nsQtKeyMap. You can create the class in nsWindow.cpp. And it has:

PRUint32 TranslateToChar(QKeyEvent *aEvent, PRUint32 aModifierKeys);

aModifierKeys is combination of kShift, kCtrl, kAlt, kMeta, kCapsLock, kShiftLock, kNumLock.

The implementation of the class can have #ifdefs. Then, the users don't need to have #ifdefs.

And you should create the instance of the class on stack, not heap. How about this idea?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Roc:

If we need to share some files between gtk2 and qt, where is a best place of the files? xpwidgets?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> PRUint32 TranslateToChar(QKeyEvent *aEvent, PRUint32 aModifierKeys);

TranslateToCharCode is better?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about widget/src/shared/x11?

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #15)
> How about widget/src/shared/x11?

Sounds good. And I think that /shared is useful for some common methods which are in all platforms, e.g., pref related methods.

Comment 17

[Jeremias Bosch (:jbos)]

Any changes on that?

Comment 18

[Jeremias Bosch (:jbos)]

Attachment: Unrotten Version

We really want this in the qt build. So whats holding this up?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I already reviewed it at comment 12-16, but it's not been updated yet.

Comment 20

[James Cook]

(In reply to comment #18)
> Created attachment 460651 [details] [diff] [review]
> Unrotten Version
> 
> We really want this in the qt build. So whats holding this up?

There are still many things wrong with this patch.  I'm working on it.

Comment 21

[Jeremias Bosch (:jbos)]

I must say that I'm not fully into that patch topic. But Lutz does not seem to update this anymore but we (Maemo/Meego) are really interested to get it landed soon. 

The Problem i have here that this topic off of my exp. so i may miss things and require a bit of your help in order to get it fixed and landed soon. Thanks for this.

Updated the patch as much as I could.


(In reply to comment #12)
> Comment on attachment 451529 [details] [diff] [review]
>  Patch to improve Qt-event.
> 
> >diff --git a/widget/src/gtk2/keysym2ucs.c b/widget/src/qt/keysym2ucs.c
> >copy from widget/src/gtk2/keysym2ucs.c
> >copy to widget/src/qt/keysym2ucs.c
> >diff --git a/widget/src/gtk2/keysym2ucs.h b/widget/src/qt/keysym2ucs.h
> >copy from widget/src/gtk2/keysym2ucs.h
> >copy to widget/src/qt/keysym2ucs.h
> 
> Cannot share the files? E.g., cannot move them to xpwidget and only builds them
> at gtk2 or qt?
> 
> I think roc should decide what is best.
> 


Duno who should do it.







> >+    // now we can update the shift_state
> >+    shift_state = shift_state ^ (bool)((caps_lock_mask | shift_lock_mask) & aEvent->nativeModifiers()) ?
> >+        PR_TRUE :
> >+        PR_FALSE;
> 
> I think that this is wrong. When CapsLock is locked, it's affected only to
> alphabet keys, not all keys.
>


Does this mater? how would you fix it here?

> >+        // first try: get a character from keyCode
> >+        if ( shift_state ) {
> >+            domCharCode = (PRUint32) QChar::toUpper( (uint)aEvent->key() );
> >+        } else {
> >+            domCharCode = (PRUint32) QChar::toLower( (uint)aEvent->key() );
> >+        }
> 
> You can check whether the upper character and the lower character are same
> character or not. If they are different, you can honor the CapsLock state if
> it's locked.
>

Duno what to do here with that?

  
> >+                    break;
> >+                }
> >+            }
> >+            qDebug() << "domCharCode is now: " << domCharCode;
> >+            qDebug();
> >+        }
> >+#endif
> >+    } else { // character is printable so we have to unset modifiers
> >+        //event.isShift = PR_FALSE;
> >+        event.isControl = PR_FALSE;
> >+        event.isAlt = PR_FALSE;
> >+        event.isMeta = PR_FALSE;
> >+    }
> >+    
> >+    // append alternativeCharCodes if modifier is pressed
> >+    // append an additional alternativeCharCodes if domCharCode is not a Latin character
> >+    // and if one of these modifiers is pressed (i.e. Ctrl, Alt, Meta)
> >+    if(domCharCode &&
> >+        (QApplication::keyboardModifiers() & (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier))) {
> >+        
> >+        QChar originChar(domCharCode);
> >+    
> >+        event.charCode = domCharCode;
> >+        event.keyCode = 0;
> >+        nsAlternativeCharCode altCharCode(0, 0);
> >+
> >+        if(originChar.isUpper() || originChar.isLower()) { // if character has a lower and upper representation
> >+            if( shift_state ) {
> >+                altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );
> >+                altCharCode.mShiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
> >+            } else {
> >+                altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
> >+                altCharCode.mShiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );
> >+            }
> 
> I worry about the condition. If the originalChar is an alphabet but the other
> shift state character of the key is different character like Hebrew keyboard
> layout, this sets wrong charcodes.
>

Same here, duno how to fix it. Can you make it right?

Comment 22

[Jeremias Bosch (:jbos)]

Attachment: Unrotten Version V2

Comment 23

[Jeremias Bosch (:jbos)]

Comment on attachment 461344 [details] [diff] [review]
Unrotten Version V2

Did I forget to add you as review?

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #21)
> (In reply to comment #12)
> > Comment on attachment 451529 [details] [diff] [review] [details]
> >  Patch to improve Qt-event.
> > 
> > >diff --git a/widget/src/gtk2/keysym2ucs.c b/widget/src/qt/keysym2ucs.c
> > >copy from widget/src/gtk2/keysym2ucs.c
> > >copy to widget/src/qt/keysym2ucs.c
> > >diff --git a/widget/src/gtk2/keysym2ucs.h b/widget/src/qt/keysym2ucs.h
> > >copy from widget/src/gtk2/keysym2ucs.h
> > >copy to widget/src/qt/keysym2ucs.h
> > 
> > Cannot share the files? E.g., cannot move them to xpwidget and only builds them
> > at gtk2 or qt?
> > 
> > I think roc should decide what is best.
> 
> Duno who should do it.

I think that you should do it. roc suggested widget/src/shared/x11, you should create the new directory and move keysym2ucs.* from gtk2.

> > >+    // now we can update the shift_state
> > >+    shift_state = shift_state ^ (bool)((caps_lock_mask | shift_lock_mask) & aEvent->nativeModifiers()) ?
> > >+        PR_TRUE :
> > >+        PR_FALSE;
> > 
> > I think that this is wrong. When CapsLock is locked, it's affected only to
> > alphabet keys, not all keys.
> 
> Does this mater? how would you fix it here?

Yes, CapsLock shouldn't affect non-alphabet keys.

I think:

shift_state =
  (shift_state ^ ((shift_lock_mask & aEvent->nativeModifiers()) != 0));
capslock_state = ((caps_lock_mask & aEvent->nativeModifiers()) != 0);

So, you must not include CapsLock state in Shift state because their meaning are really different and you should swap the computed key values by CapsLock state only when the values are alphabet.

> > >+        // first try: get a character from keyCode
> > >+        if ( shift_state ) {
> > >+            domCharCode = (PRUint32) QChar::toUpper( (uint)aEvent->key() );
> > >+        } else {
> > >+            domCharCode = (PRUint32) QChar::toLower( (uint)aEvent->key() );
> > >+        }
> > 
> > You can check whether the upper character and the lower character are same
> > character or not. If they are different, you can honor the CapsLock state if
> > it's locked.
> >
> 
> Duno what to do here with that?

Ah, if the key value isn't an alphabet, the results of toUpper and toLower are same...
but you should separate capslock_state as above, so, the new code should be:

if (shift_state ^ capslock_state) {
  ...
} else {
  ...
}

> > >+    // append alternativeCharCodes if modifier is pressed
> > >+    // append an additional alternativeCharCodes if domCharCode is not a Latin character
> > >+    // and if one of these modifiers is pressed (i.e. Ctrl, Alt, Meta)
> > >+    if(domCharCode &&
> > >+        (QApplication::keyboardModifiers() & (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier))) {
> > >+        
> > >+        QChar originChar(domCharCode);
> > >+    
> > >+        event.charCode = domCharCode;
> > >+        event.keyCode = 0;
> > >+        nsAlternativeCharCode altCharCode(0, 0);
> > >+
> > >+        if(originChar.isUpper() || originChar.isLower()) { // if character has a lower and upper representation
> > >+            if( shift_state ) {
> > >+                altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );
> > >+                altCharCode.mShiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
> > >+            } else {
> > >+                altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toLower( (uint)domCharCode );
> > >+                altCharCode.mShiftedCharCode = (PRUint32) QChar::toUpper( (uint)domCharCode );
> > >+            }
> > 
> > I worry about the condition. If the originalChar is an alphabet but the other
> > shift state character of the key is different character like Hebrew keyboard
> > layout, this sets wrong charcodes.
> >
> 
> Same here, duno how to fix it. Can you make it right?

First, you need to get actual the other shift state char code.

unshiftedChar = ...
shiftedChar = ...

if ((unshiftedChar.isUpper() || unshiftedChar.isLower()) && (shiftedChar.isUpper() || shiftedChar.isLower())) {
  if (shift_state ^ capslock_state) {
  } else {
  }
} else {
}

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> if ((unshiftedChar.isUpper() || unshiftedChar.isLower()) && (shiftedChar.isUpper() || shiftedChar.isLower())) {

er, maybe,

if ((unshiftedChar.isUpper() || unshiftedChar.isLower()) &&
    (unshiftedChar.lower() == shiftedChar.lower()) {

is better. CapsLock should affect only when the characters are same alphabet.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

please keep bug 583783 in mind while fixing this bug.

Comment 27

[Jeremias Bosch (:jbos)]

Attachment: Updated Version 3

issues adressed, this should be it. we should concentrate to push that patch since it fixes more than it actually has issues - if some are left - .  We should adress the remaining issues in another bug and not try to fix everything in one patch.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 462604 [details] [diff] [review]
Updated Version 3

>--- a/widget/src/qt/nsWindow.cpp
>+++ b/widget/src/qt/nsWindow.cpp
>@@ -60,16 +60,21 @@
> 
> #include <QtCore/QDebug>
> #include <QtCore/QEvent>
> #include <QtCore/QVariant>
> #if (QT_VERSION >= QT_VERSION_CHECK(4, 6, 0))
> #include <QPinchGesture>
> #endif // QT version check
> 
>+#ifdef MOZ_X11
>+    #include <QX11Info>
>+    #include <X11/Xlib.h>
>+#endif
>+

Doesn't need the indentation. And // comment should be at the end of the #endif line. I.e.,

#endif // MOZ_X11

>+#ifdef MOZ_X11
>+    #include "keysym2ucs.h"
>+#endif

same.

>@@ -1354,58 +1363,284 @@ nsWindow::OnKeyPressEvent(QKeyEvent *aEv
>+    // If there is no charcode attainable from the text, try to
>+    // generate it from the keycode. Check shift state for case
>+    // Also replace the charcode if ControlModifier is the only
>+    // pressed Modifier
>+    if ((!domCharCode) &&
>+        (QApplication::keyboardModifiers() & 
>+        (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier))) {
>+
>+        // first try: get a character from keyCode
>+        if (shift_state ^ capslock_state) {
>+            domCharCode = (PRUint32) QChar::toUpper((uint)aEvent->key());
>+        } else {
>+            domCharCode = (PRUint32) QChar::toLower((uint)aEvent->key());
>+        }
>+
>+#ifdef MOZ_X11
>+        // second try: get a character from X11 key map
>+        if (!QChar((uint)domCharCode).isPrint()) {
>+            KeySym keysym = aEvent->nativeVirtualKey();
>+            if (keysym) {
>+                domCharCode = (PRUint32) keysym2ucs(keysym);
>+                if (domCharCode == -1 || ! QChar((quint32)domCharCode).isPrint()) {
>+                    domCharCode = 0;
>+                }
>+            }
>+        }
>+
>+        // if Ctrl is pressed and domCharCode is not a ASCII character
>+        if (domCharCode > 0xFF && (QApplication::keyboardModifiers() & Qt::ControlModifier)) {
>+            // replace Unicode character
>+            int index = (aEvent->nativeScanCode() - x_min_keycode) * xkeysyms_per_keycode;
>+            for (int i = 0; i < xkeysyms_per_keycode; ++i) {
>+                if (xkeymap[index + i] <= 0xFF && !shift_state) {
>+                    domCharCode = (PRUint32) QChar::toLower((uint) xkeymap[index + i]);
>+                    break;
>+                }
>+            }
>+        }
>+#endif

Um... Why don't you use X11 code first? I think that only when the domCharCode is zero after that (or only in #else block?), we should use the keyCode hack.

And please use:

#endif MOZ_X11

>+    } else { // character is printable so we have to unset modifiers

I'd like you to add the reason. "because nsEditor will not accept the key event for text input if one or more modifiers are set." Of course, my English isn't good, please rewrite it :-)

>+    // append alternativeCharCodes if modifier is pressed
>+    // append an additional alternativeCharCodes if domCharCode is not a Latin character
>+    // and if one of these modifiers is pressed (i.e. Ctrl, Alt, Meta)
>+    if (domCharCode &&
>+        (QApplication::keyboardModifiers() & 
>+        (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier))) {
>+
>+        QChar unshiftedChar(domCharCode);
>+        QChar shiftedChar = unshiftedChar.isUpper() ?
>+                            QChar::toLower((uint)domCharCode) : QChar::toUpper((uint)domCharCode);

No, I meant that you should get the unshifted charCode and the shifted charCode of aEvent from X11 API. Then, the next condition of the "if" will work fine. E.g., Hebrew keyboard layout.

>+        event.charCode = domCharCode;
>+        event.keyCode = 0;
>+        nsAlternativeCharCode altCharCode(0, 0);
>+        // if character has a lower and upper representation
>+        if (unshiftedChar.isUpper() || unshiftedChar.isLower() &&
>+            unshiftedChar.lower() == shiftedChar.lower()) {
>+            if (shift_state ^ capslock_state) {
>+                altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toUpper((uint)domCharCode);
>+                altCharCode.mShiftedCharCode = (PRUint32) QChar::toLower((uint)domCharCode);
>+            } else {
>+                altCharCode.mUnshiftedCharCode = (PRUint32) QChar::toLower((uint)domCharCode);
>+                altCharCode.mShiftedCharCode = (PRUint32) QChar::toUpper((uint)domCharCode);
>+            }
>+#ifdef MOZ_X11
>+        } else { // try to find a other character mapped to that key
>+            KeySym keysym = NoSymbol;
>+            int index = (aEvent->nativeScanCode() - x_min_keycode) * xkeysyms_per_keycode;
>+            for (int i = 0; i < xkeysyms_per_keycode; ++i) {
>+                if (xkeymap[index + i] == aEvent->nativeVirtualKey()) {
>+                    if ((i % 2) == 0) { // shifted char
>+                        keysym = xkeymap[index + i + 1];
>+                        break;
>+                    } else { // unshifted char
>+                        keysym = xkeymap[index + i - 1];
>+                        break;
>+                    }
>+                }
>+                if (xkeysyms_per_keycode - 1 == i) {
>+                    qWarning() << "Symbol '" << aEvent->nativeVirtualKey() << "' not found";
>+                }
>+            }
>+            altCharCode.mUnshiftedCharCode = (PRUint32) domCharCode;
>+            long ucs = keysym2ucs(keysym);
>+            ucs = ucs == -1 ? 0 : ucs;
>+            altCharCode.mShiftedCharCode = (PRUint32) ucs;
>+#endif

#endif // MOZ_X11

>+        }
>+
>+        // append alternative char code to event
>+        if ((altCharCode.mUnshiftedCharCode || altCharCode.mShiftedCharCode) &&
>+            ((altCharCode.mUnshiftedCharCode != domCharCode) ||
>+            (altCharCode.mShiftedCharCode != domCharCode))) {
>+            event.alternativeCharCodes.AppendElement(altCharCode);
>+        }

nit,

if ((altCharCode.mUnshiftedCharCode && altCharCode.mUnshiftedCharCode != domCharCode) ||
    (altCharCode.mShiftedCharCode && altCharCode.mShiftedCharCode != domCharCode)) {

is strictly correct.

Otherwise, looks ok for me except the specific part of X11 platform.

Comment 29

[Jeremias Bosch (:jbos)]

(In reply to comment #28)
> Um... Why don't you use X11 code first? I think that only when the domCharCode
> is zero after that (or only in #else block?), we should use the keyCode hack.

Dont get that, the code is building on top of another.

> 
> No, I meant that you should get the unshifted charCode and the shifted charCode
> of aEvent from X11 API. Then, the next condition of the "if" will work fine.
> E.g., Hebrew keyboard layout.
>

This is already happening in else path, i moved it up and let it "unsolved" in the non x11 path. But i added a todo, to notice about this missing part. We might should create a bug about that issue. but fennec/xulrunner qt without x11 is really far away and there are more problems than just a not working hebrew keyboard, like a proper rendersystem...

Comment 30

[Jeremias Bosch (:jbos)]

Attachment: Updated Version 4

Comment 31

[James Cook]

(In reply to comment #26)
> please keep bug 583783 in mind while fixing this bug.

I'd like to hear opinions about bug 577630 too.

Comment 32

[Joe Drew (not getting mail)]

I don't think this blocks mozilla 2.0, but it almost certainly blocks fennec 2.0.

Comment 33

[Jeremias Bosch (:jbos)]

Mark, shortcuts in qt fennec on meego works with this also in e10s case

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #33)
> Mark, shortcuts in qt fennec on meego works with this also in e10s case

Great! and odd at the same time. Does this mean Gtk has a problem that Qt doesn't? :)

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #29)
> (In reply to comment #28)
> > Um... Why don't you use X11 code first? I think that only when the domCharCode
> > is zero after that (or only in #else block?), we should use the keyCode hack.
> 
> Dont get that, the code is building on top of another.

Sorry, I don't understand, what did you mean?

> > No, I meant that you should get the unshifted charCode and the shifted charCode
> > of aEvent from X11 API. Then, the next condition of the "if" will work fine.
> > E.g., Hebrew keyboard layout.
> >
> 
> This is already happening in else path, i moved it up and let it "unsolved" in
> the non x11 path. But i added a todo, to notice about this missing part. We
> might should create a bug about that issue. but fennec/xulrunner qt without x11
> is really far away and there are more problems than just a not working hebrew
> keyboard, like a proper rendersystem...

O.K.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 462714 [details] [diff] [review]
Updated Version 4

>+    } else { // nsEditor don't accept input when modifiers are set
>+        event.isControl = PR_FALSE;
>+        event.isAlt = PR_FALSE;
>+        event.isMeta = PR_FALSE;
>+    }

Would you append the reason which I said in my previous comment?

>+#ifdef MOZ_X11
>+    KeySym keysym = NoSymbol;
>+    int index = (aEvent->nativeScanCode() - x_min_keycode) * xkeysyms_per_keycode;
>+    for (int i = 0; i < xkeysyms_per_keycode; ++i) {
>+        if (xkeymap[index + i] == aEvent->nativeVirtualKey()) {
>+            if ((i % 2) == 0) { // shifted char
>+                keysym = xkeymap[index + i + 1];
>+                break;
>+            } else { // unshifted char
>+                keysym = xkeymap[index + i - 1];
>+                break;
>+            }
>+        }
>+        if (xkeysyms_per_keycode - 1 == i) {
>+            qWarning() << "Symbol '" << aEvent->nativeVirtualKey() << "' not found";
>+        }
>+    }
>+    QChar unshiftedChar(domCharCode);
>+    long ucs = keysym2ucs(keysym);
>+    ucs = ucs == -1 ? 0 : ucs;
>+    QChar shiftedChar(ucs);
>+#else
>+    //:TODO: Fix hebrew keyboard for qt without x11
>+    QChar unshiftedChar(domCharCode);
>+    QChar shiftedChar = nshiftedChar.isUpper() ?
>+                        QChar::toLower((uint)domCharCode) : QChar::toUpper((uint)domCharCode);
>+#endif //MOZ_X11

Thanks!

>+        // append alternative char code to event
>+if ((altCharCode.mUnshiftedCharCode && altCharCode.mUnshiftedCharCode != domCharCode) ||
>+    (altCharCode.mShiftedCharCode && altCharCode.mShiftedCharCode != domCharCode)) {
>+            event.alternativeCharCodes.AppendElement(altCharCode);
>+        }

Please indent the if condition.

>@@ -1416,16 +1656,39 @@ nsWindow::OnKeyReleaseEvent(QKeyEvent *a

>+#ifdef MOZ_X11
>+    if (!domKeyCode) {
>+        // get keymap from the Xserver
>+        Display *display = QX11Info::display();
>+        int x_min_keycode = 0, x_max_keycode = 0, xkeysyms_per_keycode;
>+        XDisplayKeycodes(display, &x_min_keycode, &x_max_keycode);
>+        KeySym *xkeymap = XGetKeyboardMapping(display, x_min_keycode, x_max_keycode - x_min_keycode,
>+                                          &xkeysyms_per_keycode);
>+
>+        if (aEvent->nativeScanCode() >= (quint32)x_min_keycode &&
>+            aEvent->nativeScanCode() <= (quint32)x_max_keycode) {
>+            int index = (aEvent->nativeScanCode() - x_min_keycode) * xkeysyms_per_keycode;
>+            for(int i = 0; (i < xkeysyms_per_keycode) && (domKeyCode == (quint32)NoSymbol); ++i) {
>+                domKeyCode = QtKeyCodeToDOMKeyCode(xkeymap[index + i]);
>+            }
>+        }
>+
>+        if (xkeymap) {
>+            XFree(xkeymap);
>+        }
>+    }
>+#endif

#endif // MOZ_X11

I'll +'ing with you change the above nits and you answer to my previous comment.

Comment 37

[Jeremias Bosch (:jbos)]

Attachment: Updated Version 5

Thanks, so now finally fixed that.

Comment 38

[Jeremias Bosch (:jbos)]

Comment on attachment 463709 [details] [diff] [review]
Updated Version 5

Added Lutz for review, too since its his patch original.

Comment 39

[Jeremias Bosch (:jbos)]

(In reply to comment #35)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > Um... Why don't you use X11 code first? I think that only when the domCharCode
> > > is zero after that (or only in #else block?), we should use the keyCode hack.
> > 
> > Dont get that, the code is building on top of another.
> 
> Sorry, I don't understand, what did you mean?

I meant that I can't move the code because the code before this is needed for this. It must stay there in order to keep the functionality.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I still don't understand...

+    // If there is no charcode attainable from the text, try to
+    // generate it from the keycode. Check shift state for case
+    // Also replace the charcode if ControlModifier is the only
+    // pressed Modifier
+    if ((!domCharCode) &&
+        (QApplication::keyboardModifiers() & 
+        (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier))) {
+
+#ifdef MOZ_X11
+        // Get a character from X11 key map
+        KeySym keysym = aEvent->nativeVirtualKey();
+        if (keysym) {
+            domCharCode = (PRUint32) keysym2ucs(keysym);
+            if (domCharCode == -1 || ! QChar((quint32)domCharCode).isPrint()) {
+                domCharCode = 0;
+            }
+        }
+
+        // if Ctrl is pressed and domCharCode is not a ASCII character
+        if (domCharCode > 0xFF && (QApplication::keyboardModifiers() & Qt::ControlModifier)) {
+            // replace Unicode character
+            int index = (aEvent->nativeScanCode() - x_min_keycode) * xkeysyms_per_keycode;
+            for (int i = 0; i < xkeysyms_per_keycode; ++i) {
+                if (xkeymap[index + i] <= 0xFF && !shift_state) {
+                    domCharCode = (PRUint32) QChar::toLower((uint) xkeymap[index + i]);
+                    break;
+                }
+            }
+        }
 #else
+        // Get a character from keyCode
+        if (shift_state ^ capslock_state) {
+            domCharCode = (PRUint32) QChar::toUpper((uint)aEvent->key());
+        } else {
+            domCharCode = (PRUint32) QChar::toLower((uint)aEvent->key());
+        }
+#endif //MOZ_X11


What is broken by this?

And capslock_state is defined in #ifdef MOZ_X11 block, you need to define capslock_state variable for non-X11 too.

Comment 41

[Jeremias Bosch (:jbos)]

(In reply to comment #40)
+        // Get a character from keyCode
+        if (shift_state ^ capslock_state) {
+            domCharCode = (PRUint32) QChar::toUpper((uint)aEvent->key());
+        } else {
+            domCharCode = (PRUint32) QChar::toLower((uint)aEvent->key());
+        }
here the domCharCode might get changed/adjusted

> +    if ((!domCharCode) &&
here we ask if its not valid. What if its not valid because of the "Upper/Lower" reported 0 ?


> +
> +#ifdef MOZ_X11
> +        // Get a character from X11 key map
> +        KeySym keysym = aEvent->nativeVirtualKey();
> +        if (keysym) {
> +            domCharCode = (PRUint32) keysym2ucs(keysym);
> +            if (domCharCode == -1 || ! QChar((quint32)domCharCode).isPrint())
> {
> +                domCharCode = 0;
> +            }
> +        }
> +
> +        // if Ctrl is pressed and domCharCode is not a ASCII character
> +        if (domCharCode > 0xFF && (QApplication::keyboardModifiers() &
> Qt::ControlModifier)) {
> +            // replace Unicode character
> +            int index = (aEvent->nativeScanCode() - x_min_keycode) *
> xkeysyms_per_keycode;
> +            for (int i = 0; i < xkeysyms_per_keycode; ++i) {
> +                if (xkeymap[index + i] <= 0xFF && !shift_state) {
> +                    domCharCode = (PRUint32) QChar::toLower((uint)
> xkeymap[index + i]);
> +                    break;
> +                }
> +            }
> +        }
>  #else
> +        // Get a character from keyCode
> +        if (shift_state ^ capslock_state) {
> +            domCharCode = (PRUint32) QChar::toUpper((uint)aEvent->key());
> +        } else {
> +            domCharCode = (PRUint32) QChar::toLower((uint)aEvent->key());
> +        }
> +#endif //MOZ_X11
> 
> 
> What is broken by this?
> 
> And capslock_state is defined in #ifdef MOZ_X11 block, you need to define
> capslock_state variable for non-X11 too.

Comment 42

[Jeremias Bosch (:jbos)]

Attachment: Updated Version 6

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #41)
> (In reply to comment #40)
> +        // Get a character from keyCode
> +        if (shift_state ^ capslock_state) {
> +            domCharCode = (PRUint32) QChar::toUpper((uint)aEvent->key());
> +        } else {
> +            domCharCode = (PRUint32) QChar::toLower((uint)aEvent->key());
> +        }
> here the domCharCode might get changed/adjusted

This just _guesses_ the domCharCode. The X11 part computes actual domCharCode on the current keyboard layout.

Comment 44

[Jeremias Bosch (:jbos)]

ok i see i did some tests and it seems right. Ok fixed it thanks for this good input.

Comment 45

[Jeremias Bosch (:jbos)]

Attachment: Updated Version 7

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 464477 [details] [diff] [review]
Updated Version 7

>+    // indicate whether is down or not (will be updated when we know if shift lock or caps lock is active)
>+    PRBool shift_state = QApplication::keyboardModifiers() & Qt::ShiftModifier;
>+    // there is no indicator about capslock in plain qt. Its in Shift.
>+    PRBool capslock_state = QApplication::keyboardModifiers() & Qt::ShiftModifier;

capslock_state should be PR_FALSE if we cannot know the actual state.

>+    // If there is no charcode attainable from the text, try to
>+    // generate it from the keycode. Check shift state for case
>+    // Also replace the charcode if ControlModifier is the only
>+    // pressed Modifier
>+    if ((!domCharCode) &&
>+        (QApplication::keyboardModifiers() & 
>+        (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier))) {
>+
>+#ifdef MOZ_X11
>+        // get a character from X11 key map
>+        if (!QChar((uint)domCharCode).isPrint()) {
>+            KeySym keysym = aEvent->nativeVirtualKey();
>+            if (keysym) {
>+                domCharCode = (PRUint32) keysym2ucs(keysym);
>+                if (domCharCode == -1 || ! QChar((quint32)domCharCode).isPrint()) {
>+                    domCharCode = 0;
>+                }
>+            }
>+        }

domCharCode must be zero at the next line of |#ifdef MOZ_X11|.
So, you can remove the |if (!QChar((uint)domCharCode).isPrint()) {|.

>+#endif //MOZ_X11
>+    } else { //because nsEditor will not accept the key event
>+             //for text input if one or more modifiers are set.

// The key event should cause a character input.
// At that time, we need to reset the modifiers
// because nsEditor will not accept a key event
// for text input if one or more modifiers are set.

>+    // append alternativeCharCodes if modifier is pressed
>+    // append an additional alternativeCharCodes if domCharCode is not a Latin character
>+    // and if one of these modifiers is pressed (i.e. Ctrl, Alt, Meta)
>+    if (domCharCode &&
>+        (QApplication::keyboardModifiers() & 
>+        (Qt::ControlModifier | Qt::AltModifier | Qt::MetaModifier))) {
>+
>+        event.charCode = domCharCode;
>+        event.keyCode = 0;
>+        nsAlternativeCharCode altCharCode(0, 0);
>+        // if character has a lower and upper representation
>+        if (unshiftedChar.isUpper() || unshiftedChar.isLower() &&
>+            unshiftedChar.lower() == shiftedChar.lower()) {

if ((unshiftedChar.isUpper() || unshiftedChar.isLower()) &&
    unshiftedChar.lower() == shiftedChar.lower()) {

And please add the moved files to next patch.

Comment 47

[Jeremias Bosch (:jbos)]

Attachment: Patch to Move Files

this patch move the files and adjust the makefiles

Comment 48

[Jeremias Bosch (:jbos)]

Attachment: Updated Version 8

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 464721 [details] [diff] [review]
Updated Version 8

>+#ifdef MOZ_X11
>+    PRBool capslock_state = PR_FALSE;
>+#else
>+    // there is no indicator about capslock in plain qt. Its in Shift.
>+    PRBool capslock_state = QApplication::keyboardModifiers() & Qt::ShiftModifier;
>+#endif //MOZ_X11

No, you can always initialize capslock_state with PR_FALSE.

>+    capslock_state = (bool)(caps_lock_mask & aEvent->nativeModifiers());

because you will set a correct value by this line.

And if the comment (capslock state is in shift state of Qt) is true, 

>+    shift_state = shift_state ^ (bool)(shift_lock_mask & aEvent->nativeModifiers());

is this correct?

The states should be:

+-------+----------+-----------+------------+-------------+----------------+
| Shift | Capslock | ShiftLock | Qt's state | shift_state | capslock_state |
+-------+----------+-----------+------------+-------------+----------------+
|  NO   |    NO    |     NO    |    FALSE   |     FALSE   |     FALSE      |
+-------+----------+-----------+------------+-------------+----------------+
|  YES  |    NO    |     NO    |    TRUE    |     TRUE    |     FALSE      |
+-------+----------+-----------+------------+-------------+----------------+
|  NO   |    YES   |     NO    |    TRUE?   |     FALSE   |     TRUE       |
+-------+----------+-----------+------------+-------------+----------------+
|  NO   |    NO    |     YES   |    TRUE?   |     TRUE    |     FALSE      |
+-------+----------+-----------+------------+-------------+----------------+
|  YES  |    YES   |     NO    |    TRUE?   |     TRUE    |     TRUE       |
+-------+----------+-----------+------------+-------------+----------------+
|  YES  |    NO    |     YES   |    TRUE?   |     FALSE   |     FALSE      |
+-------+----------+-----------+------------+-------------+----------------+

But looks like the actual shift_state value isn't so. I guess that

>+    shift_state = shift_state ^ (bool)(shift_lock_mask & aEvent->nativeModifiers());

Should be:

+    qint32 shift_mask = 0, shift_lock_mask = 0, caps_lock_mask = 0, num_lock_mask = 0;

...

+        quint32 mapindex = (modkeycode - x_min_keycode) * xkeysyms_per_keycode;
+        for (int j = 0; j < xkeysyms_per_keycode; ++j) {
+            KeySym modkeysym = xkeymap[mapindex + j];
+            switch (modkeysym) {
+                case XK_Num_Lock:
+                    num_lock_mask |= maskbit;
+                    break;
+                case XK_Caps_Lock:
+                    caps_lock_mask |= maskbit;
+                    break;
+                case XK_Shift_Lock:
+                    shift_lock_mask |= maskbit;
+                    break;
                 case XK_Shift:
                     shift_mask |= maskbit;
                     break;
+            }
+        }

...

>+    shift_state = ((shift_mask & aEvent->nativeModifiers()) != 0) ^
                      ((shift_lock_mask & aEvent->nativeModifiers()) != 0);

# I'm not sure whether the switch statement works as expected.

Comment 50

[Jeremias Bosch (:jbos)]

Mhm, alright, somehow i overlooked one thing.

if ((unshiftedChar.isUpper() || unshiftedChar.isLower()) &&
    unshiftedChar.lower() == shiftedChar.lower()) {

isn't valid qt.

It must be either:
if ((unshiftedChar.isUpper() || unshiftedChar.isLower()) &&
    unshiftedChar.isLower() == shiftedChar.isLower()) {

or

if ((unshiftedChar.isUpper() || unshiftedChar.isLower()) &&
    unshiftedChar.toLower() == shiftedChar.toLower()) {


I dont know what you wanted to achieve. 

I also wonder if we cant cut down the patch. For now meego (the only user of qt) only care to get shortcuts working, and our timeframe is basically next week for this. The patch fixes way more issues than it left. 

The qt port is for now only used in qt mobile platform - meego - which does not have any hebrew keyboard support or support for japanese. 
I totaly agree with you that those things must get fixed - but cant we get this patch within 1 more iteration in a shape to have it pushed and follow up the issues later on?

Comment 51

[Oleg Romashin (:romaxa)]

Comment on attachment 464721 [details] [diff] [review]
Updated Version 8

> 
>+#ifdef MOZ_X11
>+    }
>+#endif //MOZ_X11
>+
>+#ifdef MOZ_X11
>+        // get a character from X11 key map
>+#else
>+        }
>+#endif //MOZ_X11

I think all these ifdefs are quite crazy... and I even don't know yet any working environment with Qt and non-X11....
I think we should just don't care about qt-non-x11 path and drop it from patch, because it is not possible to test.

Also put all this functionality under X11.

if someday we decide to make support for Qt-X11, then we can create another bug and take non-X11 patch from previous patch and make that works.



+    // and if one of these modifiers is pressed (i.e. Ctrl, Alt, Meta)
+    if (domCharCode &&
+        (QApplication::keyboardModifiers() & 
                                             ^ - some white spaces, kill them

+        KeySym *xkeymap = XGetKeyboardMapping(display, x_min_keycode, x_max_keycode - x_min_keycode,
+                                          &xkeysyms_per_keycode);
some indentation problems.


I think we should fix this, findings from comment#49, and take it...
and then take care about other things, if they are available.

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #50)
> The qt port is for now only used in qt mobile platform - meego - which does not
> have any hebrew keyboard support or support for japanese. 

Okay. If there isn't French-AZERTY keyboard layout, you don't need to fix the shift lock key state issue for now. What's keyboard layout used in France?

(In reply to comment #51)
> >+#ifdef MOZ_X11
> >+    }
> >+#endif //MOZ_X11
> >+
> >+#ifdef MOZ_X11
> >+        // get a character from X11 key map
> >+#else
> >+        }
> >+#endif //MOZ_X11
> 
> I think all these ifdefs are quite crazy... and I even don't know yet any
> working environment with Qt and non-X11....
> I think we should just don't care about qt-non-x11 path and drop it from patch,
> because it is not possible to test.
> 
> Also put all this functionality under X11.

If current qt widget cannot build without X11, I agree.

Comment 53

[Jeremias Bosch (:jbos)]

Attachment: Updated Version 9

i got rid of the non x11 path and kept for this the already implemented version.

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 464862 [details] [diff] [review]
Updated Version 9

>diff --git a/widget/src/qt/nsWindow.cpp b/widget/src/qt/nsWindow.cpp
>+    // indicate whether is down or not (will be updated when we know if shift lock or caps lock is active)
>+    PRBool shift_state = PR_FALSE;
>+    PRBool capslock_state = PR_FALSE;
>+

>+    // now we can update the shift_state
>+    shift_state = ((shift_mask & aEvent->nativeModifiers()) != 0) ^
>+                    (bool)(shift_lock_mask & aEvent->nativeModifiers());
>+    capslock_state = (bool)(caps_lock_mask & aEvent->nativeModifiers());

You can define the variable at the latter block. I.e.,

>+    PRBool shift_state = ((shift_mask & aEvent->nativeModifiers()) != 0) ^
>+                           (bool)(shift_lock_mask & aEvent->nativeModifiers());
>+    PRBool capslock_state = (bool)(caps_lock_mask & aEvent->nativeModifiers());

With this change, r=masayuki.

Thank you for your work.

Comment 55

[Jeremias Bosch (:jbos)]

Attachment: Last bits fixed, patch to push

Thanks for your reviews.

Comment 56

[Jeremias Bosch (:jbos)]

Attachment: Final Version.

/local/Workspace/Fennec/mozilla/widget/src/qt/nsWindow.cpp: In member function ‘virtual nsEventStatus nsWindow::OnKeyPressEvent(QKeyEvent*)’:
/local/Workspace/Fennec/mozilla/widget/src/qt/nsWindow.cpp:1423: error: ‘XK_Shift’ was not declared in this scope
/local/Workspace/Fennec/mozilla/widget/src/qt/nsWindow.cpp:1511: warning: comparison between signed and unsigned integer expressions
/local/Workspace/Fennec/mozilla/widget/src/qt/nsWindow.cpp:1556: error: call of overloaded ‘QChar(long int&)’ is ambiguous
/local/Workspace/Fennec/mozilla/widget/src/qt/nsWindow.cpp:1579: error: invalid cast from type ‘QChar’ to type ‘PRUint32’
/local/Workspace/Fennec/mozilla/widget/src/qt/nsWindow.cpp:1580: error: invalid cast from type ‘QChar’ to type ‘PRUint32’



Fixed in this patch. Compiled on plain qt version.

Comment 57

[Lutz Schönemann]

Tested this new patch and all tests of mine work perfect.

Thanks for the good follow up work

Comment 58

[Oleg Romashin (:romaxa)]

> /local/Workspace/Fennec/mozilla/widget/src/qt/nsWindow.cpp:1423: error:
> ‘XK_Shift’ was not declared in this scope
> Fixed in this patch. Compiled on plain qt version.

Jeremias, please attach only small patch, which is fixing compilation problem.

Comment 59

[Jeremias Bosch (:jbos)]

the other patch is not yet landed, cant we not just use the final version?

Comment 60

[Oleg Romashin (:romaxa)]

we should assign small change on review, and it is better to review only that small change instead of whole patch.

Comment 61

[Jeremias Bosch (:jbos)]

ok undestand. will do.

Comment 62

[Jeremias Bosch (:jbos)]

Attachment: patch on top of "Updated Version 9" one.

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Why don't you land the patch?

Comment 64

[Jeremias Bosch (:jbos)]

I think it was hold because of fennec alpha, I cant land it, i dont have the rights. So since this here is on our top 10 list, can someone please push it?

Comment 65

[Oleg Romashin (:romaxa)]

dougt: is it beta time now can we push it now?

Comment 66

[Doug Turner (:dougt)]

yup, follow tree rules.

Comment 67

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/3b7fdb70ac2b

Comment 68

[Robert Strong (they/them - no direct email)]

Followup to fix Makefile syntax errors that busted pymake
http://hg.mozilla.org/mozilla-central/rev/2595e3710c6f

Comment 69

[ISHIKAWA, Chiaki]

Hi,

I got the following error during the build.


I posted analysis to the newsgroup mozilla.dev.builds
http://groups.google.co.jp/group/mozilla.dev.builds/browse_thread/thread/0b046a5dc1d814b7?hl=ja#

No rule to make target `../shared/x11/libwidget_shared_x11.a', needed by
`libxpwidgets_s.a'.  Stop.

My conclusion:

So I suspect that the new addition to Makefile.in either
   - gets the path name incorrect (extra /x11/ component?), OR
   - gets the extension incorrect (should have used ".so" instead of
   ".a")???

Will you kindly check it?

(I will post my MOZCONFIG on request.)

Comment 70

[Oleg Romashin (:romaxa)]

>No rule to make target `../shared/x11/libwidget_shared_x11.a', needed by
>`libxpwidgets_s.a'.  Stop.

see 591017

Comment 71

[Daniel Holbert [:dholbert]]

(In reply to comment #68)
> Followup to fix Makefile syntax errors that busted pymake
> http://hg.mozilla.org/mozilla-central/rev/2595e3710c6f

Pushed another followup to fix other instances of this syntax error, which was triggering this warning:
> Makefile:165: Extraneous text after `endif' directive
http://hg.mozilla.org/mozilla-central/rev/dfb6ec3de702