817508 - Write cursor not change direction by keyboard layout

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Amiad]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20121202 Firefox/19.0
Build ID: 20121202042013

Steps to reproduce:

I change keyboard layout from English to Hebrew.


Actual results:

Write cursor not change its direction by keyboard layout.


Expected results:

Write cursor will change its direction by keyboard layout.

Comment 1

[Amiad]

I use gnome-shell 3.6 on Arch Linux.

Comment 2

[Jesper Hansen]

Can you screenshot this cursor?
I think it might be OS dependent

Comment 3

[Amiad]

Attachment: screenshot of write cursor

Comment 4

[Simon Montagu :smontagu]

This is apparently gnome-specific -- I can't reproduce on my KDE system.

Comment 5

[Simon Montagu :smontagu]

https://bugzilla.gnome.org/show_bug.cgi?id=695018#c10:

> In most applications the caret's shape is just a vertical line.
> In Firefox, the caret has a special shape with a little arrow above the
> vertical line. The arrow should point right when typing in English, and
> point left when typing in Hebrew.
> 
> However, the shape of the caret doesn't change when switching language.
> Its shape is set according to the language that was selected before
> launching Firefox, and doesn't change. (see video)

So that's a Firefox bug then. It only seems to check the layout to use this
special caret when it starts up. It needs to do it everytime the XKB
configuration changes, i.e. listen to XkbNewKeyboardNotify and XkbMapNotify.

Comment 6

[Karl Tomlinson (:karlt)]

Gecko listens to the corresponding GTK signal "keys-changed", but doesn't notify layout code.
http://hg.mozilla.org/mozilla-central/annotate/17fe59f6c54a/widget/gtk2/nsGtkKeyUtils.cpp#l532

Comment 7

[Simon Montagu :smontagu]

Attachment: Patch

The same bug presumably exists in other OSs, but I don't know if there is a parallel notification for them -- I looked in MSDN but couldn't find anything similar for Windows.

Comment 8

[Karl Tomlinson (:karlt)]

Simon, do you know why the result of IsLangRTL() depends on what gdk_keymap_have_bidi_layouts() returns?
i.e. Why not use gdk_keymap_get_direction() regardless of what gdk_keymap_have_bidi_layouts() returns?

mHaveBidiKeyboards is used only in IsLangRTL().

Even if gdk_keymap_have_bidi_layouts() is necessary, then it could be efficiently called on each invocation of IsLangRTL() because GDK caches the results until the keyboard map changes.

How does the editor know to call IsLangRTL() again when the layout changes?

Comment 9

[Simon Montagu :smontagu]

Attachment: Simpler patch

IsLangRTL gets called every time the caret is drawn. If mHaveBidiKeyboards is false and it returns NS_ERROR_FAILURE, then no bidi hook is drawn on the caret, on the assumption that users without any bidi keyboards installed don't need the hook and will only be confused by it. If it is true, the hook is drawn on the left or right side of the caret depending on the value returned in aIsRTL.

However, if we don't need to cache mHaveBidiKeyboards for efficiency, we can do this much more simply without changing the .idl, like so.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 771813 [details] [diff] [review]
Patch

Sorry for putting you wrong Simon, but I rechecked the gdk_keymap_have_bidi_layouts() implementation and there is one part that is not cached - it's get_num_groups() does two round trips with the X server fetching much of the information that its caches were meant to save.

So, I think your first approach was a better approach.  Thanks for the explanation.

>   /**
>+   * Inspects the installed keyboards and initializes the bidi keyboard state
>+   */
>+  void Init();

Can you clarify that this can be called multiple times, and doesn't need to be called the first time, please?
Perhaps rename to "reinit" or "update".  AIUI IDL methods are usually not capitalized.  I guess roc or mats should sr.

KeymapWrapper::OnKeysChanged() only sets KeymapWrapper::mInitialized to false and doesn't directly call Init().  That means updating the nsBidiKeyboard won't happen until something else uses the KeymapWrapper.  Could the nsBidiKeyboard be updated directly from OnKeysChanged()?

Comment 11

[Simon Montagu :smontagu]

Attachment: Patch 1 v.2

Back to the first patch, with Karl's comments addressed

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 772553 [details] [diff] [review]
Patch 1 v.2

>+    if (!sBidiKeyboard) {
>+        nsresult rv = CallGetService("@mozilla.org/widget/bidikeyboard;1", &sBidiKeyboard);
>+        if (NS_FAILED(rv)) {
>+            sBidiKeyboard = nullptr;
>+        }

No need for the NS_FAILED block as sBidiKeyboard is already null.

Comment 13

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1adc72e64db0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1adc72e64db0