Write cursor not change direction by keyboard layout

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: amiadb, Assigned: smontagu)

Tracking

Trunk
mozilla25
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
I use gnome-shell 3.6 on Arch Linux.
Component: Untriaged → General

Comment 2

6 years ago
Can you screenshot this cursor?
I think it might be OS dependent
(Reporter)

Comment 3

6 years ago
Created attachment 687668 [details]
screenshot of write cursor
(Assignee)

Comment 4

5 years ago
This is apparently gnome-specific -- I can't reproduce on my KDE system.
Keywords: regressionwindow-wanted
(Assignee)

Comment 5

5 years ago
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.
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
(Assignee)

Comment 7

5 years ago
Created attachment 771813 [details] [diff] [review]
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.
Attachment #771813 - Flags: review?(karlt)
(Assignee)

Updated

5 years ago
Assignee: nobody → smontagu
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → ASSIGNED
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)
(Assignee)

Comment 9

5 years ago
Created attachment 771959 [details] [diff] [review]
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.
Attachment #771813 - Attachment is obsolete: true
Attachment #771813 - Flags: review?(karlt)
Attachment #771959 - Flags: review?(karlt)
Flags: needinfo?(smontagu)
(Assignee)

Updated

5 years ago
Keywords: regressionwindow-wanted
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()?
Component: General → Widget: Gtk
Product: Firefox → Core
(Assignee)

Comment 11

5 years ago
Created attachment 772553 [details] [diff] [review]
Patch 1 v.2

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("@mozilla.org/widget/bidikeyboard;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+
(Assignee)

Updated

5 years ago
Version: 19 Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/1adc72e64db0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.