Closed
Bug 817508
Opened 12 years ago
Closed 11 years ago
Write cursor not change direction by keyboard layout
Categories
(Core :: Widget: Gtk, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: amiadb, Assigned: smontagu)
References
Details
Attachments
(2 files, 2 obsolete files)
6.86 KB,
image/png
|
Details | |
7.92 KB,
patch
|
karlt
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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
Comment 2•12 years ago
|
||
Can you screenshot this cursor? I think it might be OS dependent
Assignee | ||
Comment 4•11 years ago
|
||
This is apparently gnome-specific -- I can't reproduce on my KDE system.
Keywords: regressionwindow-wanted
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 5•11 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.
Comment 6•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
Assignee: nobody → smontagu
Assignee | ||
Updated•11 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
Keywords: regressionwindow-wanted
Comment 10•11 years ago
|
||
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()?
Updated•11 years ago
|
Component: General → Widget: Gtk
Product: Firefox → Core
Assignee | ||
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
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 | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1adc72e64db0
Assignee | ||
Updated•11 years ago
|
Version: 19 Branch → Trunk
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1adc72e64db0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•