GNOME Bugzilla – Bug 722220
Incorrect string reported in accessible text-changed events when text is removed
Last modified: 2014-01-17 12:54:17 UTC
Created attachment 266315 [details] accessible-event listener Steps to reproduce: 1. Launch the attached accessible-event listener 2. Get into gnome-shell's run dialog by pressing Alt+F2 3. Type abcde 4. Left arrow once to move to the left of e 5. Press Backspace to remove d, c, b, and a Expected results: The character removed would be provided as the event's any_data. (See results from Gedit below as example.) Actual results: The character at the caret is reported as the event's any_data. Results from Gedit Text removed: <d>. Current text: <abce> Text removed: <c>. Current text: <abe> Text removed: <b>. Current text: <ae> Text removed: <a>. Current text: <e> Results from performing the steps above: Text removed: <e>. Current text: <abce> Text removed: <e>. Current text: <abe> Text removed: <e>. Current text: <ae> Text removed: <e>. Current text: <e>
(In reply to comment #0) > Created an attachment (id=266315) [details] > accessible-event listener > > Steps to reproduce: > 1. Launch the attached accessible-event listener > 2. Get into gnome-shell's run dialog by pressing Alt+F2 > 3. Type abcde > 4. Left arrow once to move to the left of e > 5. Press Backspace to remove d, c, b, and a FWIW, it also happens if you remove text with Supr. Will try to take a look to this one today.
After some checking, this is a regression. With clutter-1.18 this doesn't happens. The problem is not on the atk implementation, but on a change on when ClutterText::delete-text is emitted. Change that I think that was not intentional. ClutterText::delete-text documentation says that should be emitted before the text changes [1]. But on clutter 1.10 ClutterTextBuffer was added, and a lot of ClutterText code ClutterText was moved there. ClutterTextBuffer::deleted-text documentation says that it should be emitted after the text changes [2]. Right now ClutterText::delete-text implementation is mostly a forward of ClutterTextBuffer::deleted-text, so right now is being emitted after the text changes. What I don't know is this bug is a bug on ClutterText::delete-text documentation (so we would be changing the original behaviour of that signal) or on his implementation. In any case, current ATK implementation follows ClutterText documentation, so I'm moving this bug to clutter:ClutterText. [1] https://developer.gnome.org/clutter/unstable/ClutterText.html#ClutterText-delete-text [2] https://developer.gnome.org/clutter/unstable/ClutterTextBuffer.html#ClutterTextBuffer-deleted-text
ouch, sorry about that. it's definitely a regression, since the documentation was there before the introduction of ClutterTextBuffer. we should emit ClutterText::delete-text, then call clutter_text_buffer_delete_text(). the main issue is clutter_text_buffer_delete_text() would need to cause the emission of ClutterText::delete-text as well, but that would only happen after ClutterTextBuffer::delete-text has been emitted, which in turn happens after the text has been removed. I'm open to ideas on how to fix this regression.
(In reply to comment #3) > we should emit ClutterText::delete-text, then call > clutter_text_buffer_delete_text(). the main issue is > clutter_text_buffer_delete_text() would need to cause the emission of > ClutterText::delete-text as well, but that would only happen after > ClutterTextBuffer::delete-text has been emitted, which in turn happens after > the text has been removed. Yes I briefly thought about this, and it is complicated (assuming that is possible) to emit a signal before the text changes, if you are using as base a signal emitted after the text changes. > I'm open to ideas on how to fix this regression. TL;DR; Emit ClutterTextBuffer::deleted-text before text changes (update documentation properly). It would be needed to do the same with ClutterTextBuffer::inserted-text Explanation: because is the straightforward solution, what I was hoping is to just conclude that having a signal emitted after the text changes is an error. Usually you want this signal for a given purpose, not just to know that the text changes. In our case we need it to report that the text change and to get what text was removed/added. As the text removed/added is not included on the signal, we need to ask as part of the callback a subrange of the text, so the text can't be modified yet. Looking at gtk [1], in some cases the signal could be also used to modify the insertion/removal (something we don't need in this case). In fact, this is the first case I see a text-change signal emitted after the text changes. Obviously this is a problem if there are too many components using that signal assuming that will be emitted after the text changes (so we would get similar regressions to the current one). Second option would be add a new text-changed related signal on ClutterTextBuffer, to be emitted before the text changes. [1] https://developer.gnome.org/gtk3/3.9/GtkEditable.html#GtkEditable-delete-text
I have realized something. For the case of ClutterTextBuffer::inserted-text[1], it includes the text inserted. Having the same for ClutterTextBuffer::deleted-text would solve our problem. Anyway, that would mean changing the signature of the signal, something that would mean an API change to anyone connected to that signal. [1] https://developer.gnome.org/clutter/unstable/ClutterTextBuffer.html#ClutterTextBuffer-inserted-text
we kinda mutuated the ClutterTextBuffer from GtkEntryBuffer, so we have the same behaviour. how do you handle that one? I'd agree that we ought to change ClutterTextBuffer::deleted-text to be emitted before the text deletion takes place, but I'm not entirely sure this will work well in case you're subclassing ClutterTextBuffer, where you need to call clutter_text_buffer_emit_deleted_text() yourself. we definitely cannot change the signature of the ::deleted-text signal: it'd be an ABI break for the signal, and an API break for the emitter function.
(In reply to comment #6) > we kinda mutuated the ClutterTextBuffer from GtkEntryBuffer, so we have the > same behaviour. how do you handle that one? Good point. On Gtk we just use GtkEditable that have the signal we want, so I didn't realize that we have there the same situation we have here (buffer emitting after, public interface emitting before). After checking, this is how it works on GtkEntry: * GtkEditable has two virtual methods, delete_text for the signal, and do_delete_text for the method gtk_editable_delete_text. * GtkEntry reimplement both. * do_delete_text implementation just emit the signal "delete-text" * delete_text implementation calls gtk_entry_buffer_delete_text * <snarky> Really easy to follow code: iface->do_delete_text = gtk_entry_delete_text; iface->delete_text = gtk_entry_real_delete_text; </snarky> * GtkEntry implementation uses mostly gtk_editable_delete_text. So this would call ->do_delete_text. This will emit the signal "delete-text" before the changes of the text. As the signal is emitted, it will execute the virtual method for that signal, so ->delete_text, that as I said, calls gtk_entry_buffer_delete_text. Atk connects with g_signal_connect, so at that point the text didn't change. I guess that using g_signal_connect_after would mean that would fail. * GtkEntry also connects to GtkEntryBuffer::deleted-text, but the callback just do some internal updates, without firing any signal. * Taking into account this logic, it is really likely that if you delete text calling directly gtk_entry_buffer_delete_text you are not going to get the equivalent signals from GtkEditable. So you need to be really careful to use GtkEditable API, even on the object implementing it, in order to get the proper signals. Somehow, the "main issue" you mentioned on comment 3 is not covered by gtk. > I'd agree that we ought to change ClutterTextBuffer::deleted-text to be emitted > before the text deletion takes place, but I'm not entirely sure this will work > well in case you're subclassing ClutterTextBuffer, where you need to call > clutter_text_buffer_emit_deleted_text() yourself. Well, we could try to do what Gtk does, and just not worry about what happens when you modify directly the buffer. Not sure how wise is that.
(In reply to comment #7) > Well, we could try to do what Gtk does, and just not worry about what happens > when you modify directly the buffer. Not sure how wise is that. well, since there is no documentation detailing the fact that modifying the TextBuffer will result in signals being emitted by the ClutterText actor(s) using it, I think we found a nice grey area to exploit at our advantage. so: • clutter_text_delete_text() should: ‣ emit ClutterText::delete-text ‣ call clutter_text_buffer_delete_text() and we remove the ClutterText::delete-text emission from the ClutterTextBuffer::deleted-text handler inside ClutterText. this does suck a bit, but at least we're consistent with GTK+; you should probably add a comment pointing to this bug inside clutter_text_delete_text().
Created attachment 266489 [details] [review] Emit ClutterText::delete-text before changing the text As clutter_text_buffer_delete_text was used in two different places, and both would need a ClutterText::delete-text emission, I create a new method to do both things. In that way the explanatory comments are places just once, as would apply in both cases. Using all the power of my imagination I called that method clutter_text_real_delete_text (taken from gtk), but it is not a really good name.
Created attachment 266492 [details] [review] Emit ClutterText::delete-insert before changing the text Documentation of that signal also says that should be emitted before. It is not causing any bug now, but as we are solving one, it would be good to solve both, and avoid one signal being emitted at the proper moment, and the other at the wrong moment.
Review of attachment 266492 [details] [review]: looks good to me. have you checked that the test suite still passes?
Review of attachment 266489 [details] [review]: looks good. same question as above: did the test suite pass?
(In reply to comment #10) > Created an attachment (id=266492) [details] [review] > Emit ClutterText::delete-insert before changing the text typo, this should be ::insert-text, not ::delete-insert :-)
(In reply to comment #13) > (In reply to comment #10) > > Created an attachment (id=266492) [details] [review] [details] [review] > > Emit ClutterText::delete-insert before changing the text > > typo, this should be ::insert-text, not ::delete-insert :-) Yep, but luckily enough, was a typo on the patch description, not on the commit description. (In reply to comment #11) > Review of attachment 266492 [details] [review]: > > looks good to me. have you checked that the test suite still passes? Yes. (In reply to comment #12) > Review of attachment 266489 [details] [review]: > > looks good. same question as above: did the test suite pass? And yes. Patches committed. Closing bug.
Probably a stupid question, but is there any chance this fix will be present in the version used in gnome 3:10? Thanks.
(In reply to comment #15) > Probably a stupid question, but is there any chance this fix will be present in > the version used in gnome 3:10? GNOME 3.10 uses Clutter 1.16. distributions will need to pick up the patch until I make a new 1.16 release (will probably do so soon anyway).