Closed Bug 1297985 Opened 3 years ago Closed 3 years ago

The letter 'щ' can not be entered when using the Windows 10 Russian - Mnemonic keyboard layout.

Categories

(Core :: Widget: Win32, defect, P2)

48 Branch
x86_64
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: dan.isac.91, Assigned: masayuki)

References

Details

(Keywords: regression, Whiteboard: tpi:+)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160822111414

Steps to reproduce:

1. Use the Russian - Mnemonic keyboar layout on Windows 8/8.1/10.
2. Open Firefox, and try typing the letter 'щ'. This letter should appear after you press the 's' key, and then the 'c' key.



Actual results:

Instead of the letter 'щ' appearing, the result is ’сц’.


Expected results:

One letter 'щ' is expected to be appear, instead of 'сц’ (2 letters).
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Here's regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=29022393615224517283b16adae938128f75e27b&tochange=eb7c36e2ef5d48262bc8566da9ea37623e7d0883

There are 4 bugs but only bug 1137561 touched windows code.
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: Win32
Ever confirmed: true
Product: Firefox → Core
See Also: → 1137561
Attached file log
Odd... We received WM_CHAR messages as is. I guess that I've destroyed dead key handling in bug 1137561.
Blocks: 1137561
Keywords: inputmethod
See Also: 1137561
Okay, perhaps, I understand the cause of this bug. The regression point is this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=8720263&action=diff

In this patch, when mCommittedCharsAndModifiers is longer than 1, we made NativeKey dispatch keypress events without WM_CHAR messages.  However, this hits a hidden bug of KeyboardLayout. KeyboardLayout thinks that the key combination causes 2 characters "сц" but WM_CHAR comes only for "щ" but it's ignored.

So, we need to investigate the cause of this bug in KeyboardLayout class. Fortunately, I hit this assertion:
https://dxr.mozilla.org/mozilla-central/rev/01748a2b1a463f24efd9cd8abad9ccfd76b037b8/widget/windows/KeyboardLayout.cpp#2998

So, there is a bug around here.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Hmm, according to the debugger, KeyboardLayout marks both "s" and "c" are dead key. And when dead key pressed twice, it assumes that the result is always both characters which are produced when each dead key pressed twice. So, KeyboardLayout might not work with the keyboard layout, sigh...
Priority: -- → P2
Whiteboard: tpi:+
Looks like this regressed in 48.  We could still take a patch in 49 in late beta (i.e. very soon) if it seems worth the risk.
Version: 49 Branch → 48 Branch
Yes, these builds work. Thank you very much!
Большое спасибо! :)
Comment on attachment 8788017 [details]
Bug 1297985 part.1 Log KeybordLayout::LoadLayout() to help developers to understand what data is created

https://reviewboard.mozilla.org/r/76548/#review74748
Attachment #8788017 - Flags: review?(m_kato) → review+
Comment on attachment 8788018 [details]
Bug 1297985 part.2 KeyboardLayout should handle a composite character produced by 2 dead keys

https://reviewboard.mozilla.org/r/76550/#review74796
Attachment #8788018 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b1f40b39b2b8
part.1 Log KeybordLayout::LoadLayout() to help developers to understand what data is created r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ecf37ffff913
part.2 KeyboardLayout should handle a composite character produced by 2 dead keys r=m_kato
Comment on attachment 8788017 [details]
Bug 1297985 part.1 Log KeybordLayout::LoadLayout() to help developers to understand what data is created

Approval Request Comment
[Feature/regressing bug #]: Bug 1137561
[User impact if declined]: Users cannot type some characters with specific keyboard layout if the characters are composited by 2 dead keys.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Low, because this only adds logging code of KeyboardLayout class.
[String/UUID change made/needed]: Nothing.
Attachment #8788017 - Flags: approval-mozilla-aurora?
Comment on attachment 8788018 [details]
Bug 1297985 part.2 KeyboardLayout should handle a composite character produced by 2 dead keys

Approval Request Comment
[Risks and why]: Mid. This patch changes dead key handling code. KeyboardLayout hasn't supported such key sequence, 1st key is a dead key and 2nd key is a dead key and the key sequence composites a character instead of inputting two keys' characters. Due to the change of bug 1137561, NativeKey class started to use KeyboardLayout class's mapping table in such cases. So, this was a hidden bug before that.  The risk isn't low (could have some regressions of dead key handling), but the symptom is disaster.  Therefore, I believe that it's worthwhile to uplift these patches.
[String/UUID change made/needed]: Nothing.
Attachment #8788018 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b1f40b39b2b8
https://hg.mozilla.org/mozilla-central/rev/ecf37ffff913
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hello Dan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Comment on attachment 8788017 [details]
Bug 1297985 part.1 Log KeybordLayout::LoadLayout() to help developers to understand what data is created

Recent regression since 48, has stabilized in Nightly for 3 days, Aurora50+
Attachment #8788017 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8788018 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi,
im still having this issue
(In reply to cupermountain from comment #22)
> Hi,
> im still having this issue

Did you tested with Beta, Developer Edition or Nightly?
Flags: needinfo?(dan.isac.91) → needinfo?(cupermountain)
Flags: needinfo?(cupermountain)
You need to log in before you can comment on or make changes to this bug.