1701288 - Linux: Handling of dead keys in text input fields broken since GTK 3.24.26

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[teruna]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 SeaMonkey/2.53.8

Steps to reproduce:

GTK 3.24.26 was released a month ago and introduced a new way of handling dead keys. Usually, pressing does not show anything and only after pressing e one sees è. Starting with that GTK version, is already displayed, accompanied by a "short _" below the accent, and pressing e afterwards yields è. A visualisation can be seen in this blogpost: https://blog.gtk.org/2021/03/24/input-revisited/

Actual results:

Trying to type è (or any other letters in combination with dead keys) in any text field in SeaMonkey (URL bar, new mail, this bug submission form, ...) yields è`. Namely, the accent is printed twice, once on the letter and once after it.

Expected results:

The input text should read è.

I observed this issue with both the latest version from WG9s (that uses bundled libraries) as well as with my self-build version on Arch Linux (that uses system libraries). A test downgrade to GTK 3.24.25 restored the functionality.

I also tested it with previous official versions: Dead keys work as expected up to version 2.53.4 but are broken starting with 2.53.5b1.

This issue does not occur with neither the current Firefox nor with the latest Waterfox Classic 56.5 (waterfox-classic-2021.02.en-US.linux-x86.tar.bz2).

Comment 1

[teruna]

Bugzilla interpreted ` as a markdown command. The sentence should have been

"Usually, pressing ` (grave accent) does not show anything and only after pressing e one sees è. Starting with that GTK version, ` is already displayed, accompanied by a "short _" below the accent, and pressing e afterwards yields è."

Comment 2

[Dmitry Butskoy (:buc)]

If it is since 2.53.5b1, then probably this commit, and now it is probably affected by the gtk3 changes .

But nothing obvious...

Some similar issues: bug #1505147, bug #1508200, bug #1511752 .

It could be fine to know whether the issue is reproducible with various Firefox versions (even starts from FF52 and bisect to the latest one: http://archive.mozilla.org/pub/firefox/releases/ ), in a hope it is reprocudible with Firefox at all...

Comment 3

[Dmitry Butskoy (:buc)]

If it is since gtk3-3.24.26, then the correspond initial gtk3 change seems to be here . But the code is still changed right now, so it is better to wait for gtk3-3.24.28 or try some pre28 devel build to make sure the issue still exists and (if any) behaves the same.

Teruna,
Could you make some test builds yourself under your environment? If so, then:
Try to build SM with the commit reverted (to make sure it triggers the issue).
When gtk3 changes in that area will seem to stop, try to build the latest gtk3 devel snapshot and check with it.
Check whether Firefox-63 (a time of the commit ) is affected too.

Comment 4

[teruna]

I now built GTK from the latest git commit in the gtk-3-24 branch (i.e., this one: https://gitlab.gnome.org/GNOME/gtk/-/commit/71c64e650d4c355fe9597165eeac2bcdde6cb0f8). That's the pre28 devel version that Dmitry was referring to. It did not change anything, dead keys work correctly in 2.53.4 but neither in 2.53.5b1 nor in the current 2.53.8b1 pre.
I also tested Firefox 52-63, no issues. Neither did I observe issues with other GTK applications.

There is, actually, a very simple patch to restore the old dead key behaviour in GTK: https://www.reddit.com/r/linux/comments/lsda2x/gtk_has_changed_how_dead_keys_work_i_didnt_like/ Applying that patch, dead keys indeed again work normally in 2.53.8b1 pre. But this is of course not really a solution.

Dmitry, I will try building the current 2.53.8b1 pre with that specific commit reverted. May I ask how you identified that particular one? If I understand GitLab correctly, this commit was already present in 2.53.1?

Comment 5

[teruna]

If I revert that specific commit, the patch queue does not apply correctly any more. The alternative of first applying the entire queue and then trying to revert the patch also does not work cleanly.

Comment 6

[Dmitry Butskoy (:buc)]

I will try building the current 2.53.8b1 pre with that specific commit reverted. May I ask how you identified that particular one? If I understand GitLab correctly, this commit was already present in 2.53.1?
Yep...

It was kinda fast attempt to guess something. (Considered what gtk3 functions was affected by the change, and tried to find them in SM sources).

Well, the issue seems to be somewhere much deeper.

Teruna,
Could you please describe your (testing) environment a bit more? What locale, keyboard layout, what version of Arch Linux, and maybe the version of system libraries used (those which are "not bundled" in Arch as opposite to official build).

Comment 7

[Dmitry Butskoy (:buc)]

The versions of the system libraries just to know that nothing changed in [system_ver, bundled_ver] or [bundled_ver, system_ver].

Could you also make sure that Firefox > 63 not affected as well? (Technically, something was backported from Fx tree at [2.53.4, 2.53.5b1] time, and this causes the error. Since the reason seems to be in a backported code, it is likely that a source of that code is affected too. So it could be fine to check whether some Firefox version affected too, which allows to locate the issue easier).

Comment 8

[teruna]

Dmitry:
Locale: en_US.UTF-8
Keyboard layout: German (dead grave acute) but the problem also occurs with other layouts that have dead keys: I have tested the regular German and French layouts as well as English (US, intl., with dead keys).
Arch Linux version: Arch is rolling release, thus there are no versions.
System libraries: libffi 3.3, hunspell 1.7.0, cairo 1.17.4, pixman 0.40.0, bzip2 1.0.8, icu 68.2, libjpeg-turbo 2.0.6, libevent 2.1.12, nspr 4.30, nss 3.63, zlib 1.2.11. Please note, however, that the problem also occurs with the official versions that bundle those libraries.

In any case, to rule out any interference with other settings I performed all tests with a newly-created user.

No issues with Firefox up to version 83.0, which appeared in November 2020 and hence after 2.53.1b1 (October 2020).

Comment 9

[Dmitry Butskoy (:buc)]

Reproduced under CentOS-7 with locally built gtk3-3.24.26 .

Comment 10

[teruna]

Maybe this can be helpful: I now built Firefox 60.7.6 out of the SeaMonkey 2.53.6 source and there the dead key issue occurs. By contrast, the official Firefox versions 60.7.2esr and 60.8.0esr work fine (60.7.6 does not seem to have been released).

Comment 11

[Frank-Rainer Grahl (:frg)]

Bug 1443091 would fit the bill I think but the original bug mentioned is not in 2.53. Looking at the patch queue for widget/gtk mostly client side decoration fixes. Nothing which sticks out.

Could someone test 2.57. Only running openSUSE 15.3 on hardware and vms I use for testing are not bleeding edge.

Keyboard and event hadling was heavily refactored and changed after 56. So far I was reluctant to port the stuff for fear of breaking add-ons but mabe the time has come. Some websites now break too because of non spec event handling.

Btw. Firefox built from 2.53 source is like 2.53 a bastard. Based on 56 with tons of fixes up to current Gecko. Just 60 because of stupid website ua sniffung.

Comment 12

[Dmitry Butskoy (:buc)]

Partially reverting the backport of bug #1649005 related to editor/libeditor/TextEditor.cpp fixes the issue.

Teruna,

Could you test whether the reverting of this part fixes the issue for you too?
If yes, pls test that it with gtk3-3.24.27 and any later, to make sure the solution is universal (I've tested with 3.24.26 only).

IOW, the revert of this one:

diff --git a/editor/libeditor/TextEditor.cpp b/editor/libeditor/TextEditor.cpp
index e47ec2b46d..f61b241c4a 100644
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -654,6 +654,10 @@ TextEditor::InsertText(const nsAString& aStringToInsert)
     return NS_ERROR_NOT_INITIALIZED;
   }
 
+  if (aStringToInsert.IsEmpty()) {
+    return NS_OK;
+  }
+
   // Protect the edit rules object from dying
   nsCOMPtr<nsIEditRules> rules(mRules);
 
@@ -1522,6 +1526,10 @@ TextEditor::Rewrap(bool aRespectNewlines)
                           &isCollapsed, current);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  if (current.IsEmpty()) {
+    return NS_OK;
+  }
+
   nsString wrapped;
   uint32_t firstLineOffset = 0;   // XXX need to reset this if there is a selection
   rv = InternetCiter::Rewrap(current, wrapCol, firstLineOffset,
@@ -1544,6 +1552,10 @@ TextEditor::StripCites()
                                    &isCollapsed, current);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  if (current.IsEmpty()) {
+    return NS_OK;
+  }
+
   nsString stripped;
   rv = InternetCiter::StripCites(current, stripped);
   NS_ENSURE_SUCCESS(rv, rv);

Comment 13

[Dmitry Butskoy (:buc)]

Well, it seems only the first hunk is needed (note in reverted mode):

diff --git a/editor/libeditor/TextEditor.cpp b/editor/libeditor/TextEditor.cpp
index e47ec2b46d..f61b241c4a 100644
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -654,6 +654,10 @@ TextEditor::InsertText(const nsAString& aStringToInsert)
     return NS_ERROR_NOT_INITIALIZED;
   }
 
+  if (aStringToInsert.IsEmpty()) {
+    return NS_OK;
+  }
+
   // Protect the edit rules object from dying
   nsCOMPtr<nsIEditRules> rules(mRules);
 

Thus the first hunk for editor/libeditor/TextEditor.cpp in the backport of bug #1649005 was not needed, the second seems needed (some "::Rewrite" exists in the original Fx commit), and no idea about third. So the whole backport should be reconsidered.

Comment 14

[Dmitry Butskoy (:buc)]

Well, dropping the first hunk does not lead to crash described in https://bugzilla.mozilla.org/show_bug.cgi?id=1649005#c3, so this hunk was not needed.

Comment 15

[teruna]

frg, today's 2.57 from WG9s is also affected.

buc, reverting the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1701288#c13 worked. I tested it with the 2.53.7 source. There was an offset by -12 lines, though. Thanks a lot for spotting the relevant patch!

Comment 16

[Frank-Rainer Grahl (:frg)]

Will be fixed for a future 2.53.8b1 and up. Just doing some other things first and want to take a closer look. The code was heavily refactored after 2.57 / 60 so just want to make sure that the backed out stuff can not cause a crash elsewhere.

FRG

Comment 17

[Frank-Rainer Grahl (:frg)]

Attachment: 1701288-partialbackout-25371.patch

Still unable to reproduce the original crash with the partial backout so lets go with it. I would be fine with it for 2.53.7.1

[Approval Request Comment]
Regression caused by (bug #): Backport of bug 1649005
User impact if declined: Problems entering dead keys
Testing completed (on m-c, etc.): 2.53.8b1 pre
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: --

Comment 18

[Dmitry Butskoy (:buc)]

I'm worry about the first hunk...

The original Fx commit has this first hunk, plus ::Rewrap one.

Initial SM one adds also ::InsertText and ::StripCites. It is OK to remove these two (esp. ::InsertText is the reason for the issue).
But ::InsertTextWithQuota seems spoil nothing for gtk3, and since it is exactly present in the original Fx commit, probably it is needed to fix the crash properly under some circumstances. Even if under SM code it is not so obvious...

Comment 19

[Frank-Rainer Grahl (:frg)]

Attachment: 1701288-partialbackout-v1_1-25371.patch

Message to myself: Don't do patches at 4:00am.

I am flying a bit blind here. Currently don't have a Linux with the affected gtk version at hand. Could you test this patch. I still think the backported checks are basically correct just not in insertText.

Comment 20

[Dmitry Butskoy (:buc)]

Comment on attachment 9214232 [details] [diff] [review]
1701288-partialbackout-v1_1-25371.patch

Works for me.

But saying about "::InsertTextWithQuota" I mean the hunk for "HTMLEditor::InsertTextWithQuotations" from editor/libeditor/HTMLEditorDataTransfer.cpp, not "TextEditor::InsertTextWithQuotations" from editor/libeditor/TextEditor.cpp ...

The way with only one hunk for "TextEditor::InsertText" fixes gtk3 issue, and do not lead to crash.

Comment 21

[Dmitry Butskoy (:buc)]

According to bug #1489939 comment #5, the only presence of InsertTextWithQuotations in the comm code belongs to HTMLEditor rather than TextEditor.

The TextEditor::InsertTextWithQuotations is considered "never used" in bug #1476897 .

IOW, looks like the hunk with it does not needed.

Comment 22

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9214232 [details] [diff] [review]
1701288-partialbackout-v1_1-25371.patch

[Approval Request Comment]
Regression caused by (bug #): Backport of bug 1649005
User impact if declined: Problems entering dead keys
Testing completed (on m-c, etc.): 2.53.8b1 pre
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: --

buc I bookmarked Bug 1476897 for a possible backport later. Thanks for tracking it further down. But too complex for 2.53.7.1 Given that the current patch seems to be not wrong lets just take it to fix the bug fast.

Comment 23

[Ian Neal]

Comment on attachment 9214232 [details] [diff] [review]
1701288-partialbackout-v1_1-25371.patch

>+++ b/editor/libeditor/TextEditor.cpp
>@@ -1365,18 +1361,21 @@ TextEditor::OutputToStream(nsIOutputStre
>   if (NS_WARN_IF(!encoder)) {
>     return NS_ERROR_FAILURE;
>   }
> 
>   return encoder->EncodeToStream(aOutputStream);
> }
> 
> NS_IMETHODIMP
>-TextEditor::InsertTextWithQuotations(const nsAString& aStringToInsert)
>-{
>+TextEditor::InsertTextWithQuotations(const nsAString& aStringToInsert) {
I don't think moving the "{" was part of the original commit :)

r/a=me with that sorted.

Comment 24

[Frank-Rainer Grahl (:frg)]

https://gitlab.com/seamonkey-project/seamonkey-2.53-mozilla/-/commit/dae420b4d24e0696d9019c67ac3f91712d76ae00
Check for empty string only when insert quoted tesxt. r=IanN a=IanN