Closed Bug 817508 Opened 10 years ago Closed 9 years ago

Write cursor not change direction by keyboard layout


(Core :: Widget: Gtk, defect)

Not set





(Reporter: amiadb, Assigned: smontagu)




(2 files, 2 obsolete files)

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.
I use gnome-shell 3.6 on Arch Linux.
Component: Untriaged → General
Can you screenshot this cursor?
I think it might be OS dependent
This is apparently gnome-specific -- I can't reproduce on my KDE system.

> 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.
Gecko listens to the corresponding GTK signal "keys-changed", but doesn't notify layout code.
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #771813 - Flags: review?(karlt)
Assignee: nobody → smontagu
Ever confirmed: true
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?
Flags: needinfo?(smontagu)
Attached patch Simpler patch (obsolete) — Splinter Review
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.
Attachment #771813 - Attachment is obsolete: true
Attachment #771813 - Flags: review?(karlt)
Attachment #771959 - Flags: review?(karlt)
Flags: needinfo?(smontagu)
Comment on attachment 771813 [details] [diff] [review]

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()?
Component: General → Widget: Gtk
Product: Firefox → Core
Attached patch Patch 1 v.2Splinter Review
Back to the first patch, with Karl's comments addressed
Attachment #771959 - Attachment is obsolete: true
Attachment #771959 - Flags: review?(karlt)
Attachment #772553 - Flags: superreview?(roc)
Attachment #772553 - Flags: review?(karlt)
Comment on attachment 772553 [details] [diff] [review]
Patch 1 v.2

>+    if (!sBidiKeyboard) {
>+        nsresult rv = CallGetService(";1", &sBidiKeyboard);
>+        if (NS_FAILED(rv)) {
>+            sBidiKeyboard = nullptr;
>+        }

No need for the NS_FAILED block as sBidiKeyboard is already null.
Attachment #772553 - Flags: review?(karlt) → review+
Attachment #772553 - Flags: superreview?(roc) → superreview+
Version: 19 Branch → Trunk
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 896032
You need to log in before you can comment on or make changes to this bug.