| Summary: | [Gtk] Regression: text-inserted events lack text inserted and current line | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joanmarie Diggs <jdiggs> | ||||||||
| Component: | Accessibility | Assignee: | Mario Sanchez Prada <mario> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cgarcia, dglazkov, eric, mario, mrobinson, pnormand, webkit.review.bot | ||||||||
| Priority: | P2 | Keywords: | Gtk | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 25531 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Joanmarie Diggs
2011-11-20 11:59:11 PST
Created attachment 116624 [details]
Patch proposal + unit test
Attaching now a patch proposal that basically updates the code to using the new 'text-insert' and 'text-remove' signals (instead of the old, and now deprecated, 'text-changed' one), which allows us to emit the text being actually modified (inserted or removed) along with the signal, instead of just emitting the 'offset' and 'count' values and then relaying on the ATK bridge to get the right information at the right time, which is something we know does not always work fine (and one of the main reasons behind the deprecation of the 'text-changed' signal, AFAIK).
This patch also changes the signature of the nodeTextChangeNotification() and nodeTextChangePlatformNotification() in AXObjectCache, to allow specifying the text being modified from the text editing commands (as we're gonna send the modified text, there's no point in not providing this information to the AXObjectCache if we already know it, right?), and so I needed to update also the signatures in the platform-specific versions of that class for the mac, win and chromium ports too, so they will keep building. Fortunately, this change shouldn't impact those ports at all since they are basically ignoring this functionality and providing an empty implementation for them.
Last, I haven't included a new test with this patch, but just updated the a11y API test we already had in place to use the new signals instead of the old ones. Also, I made the most of this change to implement more careful and detailed checks in that unit test, since it's obvious that the old version of it was not good enough, as it didn't detected that things broke heavily recently (and thus the reason for this bug to be reported).
Adding reviewers to CC Comment on attachment 116624 [details]
Patch proposal + unit test
r=me
Committed r101349: <http://trac.webkit.org/changeset/101349> Mario, using WebKitGtk built from this morning I am seeing this bug only partially fixed: The text inserted is now correct, but the current line seems to only be a single letter which may or may not correspond with the text displayed. Could you please double-check this? Thanks! (In reply to comment #5) > Mario, using WebKitGtk built from this morning I am seeing this bug only partially fixed: The text inserted is > now correct, but the current line seems to only be a single letter which may or may not correspond with the > text displayed. Could you please double-check this? Thanks! Hmmm... I am not sure I understand. I've quickly checked this in the text entries in http://www.igalia.com/contact and all the events I see in accerciser seem to be correct, both in terms of the text displayed and the offset and count values. Perhaps I am not reproducing properly the issue. Could you please point out specific steps for it? In any case, no need to hurry much since I won't be able to work much (if at all) on this month, since I am on holidays since today and so I can't promise much before January. (In reply to comment #6) > Hmmm... I am not sure I understand. I've quickly checked this in the text entries in http://www.igalia.com/contact and all the events I see in accerciser seem to be correct, both in terms of the text displayed and the offset and count values. Yeah, but what about the current line? -- which you would have to use the ipython console for. Or alternatively, you could use my handy (and attached with the opening report) test script. ;) When I type 'Acme Inc.' in the Company field at http://www.igalia.com/contact, here is what my test script prints out in Epiphany: Current line: A All text: A Text inserted: <c> Current line: A <-- WRONG All text: Ac Text inserted: <m> Current line: A <-- WRONG All text: Acm Text inserted: <e> Current line: A <-- WRONG All text: Acme Text inserted: < > Current line: A <-- WRONG All text: Acme Text inserted: <I> Current line: A <-- WRONG All text: Acme I Text inserted: <n> Current line: A <-- WRONG All text: Acme In Text inserted: <c> Current line: A <-- WRONG All text: Acme Inc Text inserted: <.> Current line: A <-- WRONG All text: Acme Inc. Contrast that with what is seen in Epiphany's Gtk+ widget in which you can type the URL to navigate to. Text inserted: <A> Current line: A All text: A Text inserted: <c> Current line: Ac <-- EXPECTED All text: Ac Text inserted: <m> Current line: Acm <-- EXPECTED All text: Acm Text inserted: <e> Current line: Acme <-- EXPECTED All text: Acme Text inserted: <,> Current line: Acme, <-- EXPECTED All text: Acme, Text inserted: < > Current line: Acme, <-- EXPECTED All text: Acme, Text inserted: <I> Current line: Acme, I <-- EXPECTED All text: Acme, I Text inserted: <n> Current line: Acme, In <-- EXPECTED All text: Acme, In Text inserted: <c> Current line: Acme, Inc <-- EXPECTED All text: Acme, Inc Text inserted: <.> Current line: Acme, Inc. <-- EXPECTED All text: Acme, Inc. Given that your response was based on what you were seeing in Accerciser (which does not spit out the current line of text automatically in the event monitor), I am going to assume that this bug is only partially fixed and REOPEN it so that it does not get forgotten. Argh, seems I do not have the super powers required to REOPEN bugs. :-/ So UNCONFIRMED. (In reply to comment #7) >[...] > Given that your response was based on what you were seeing > in Accerciser (which does not spit out the current line of text > automatically in the event monitor), I am going to assume that > this bug is only partially fixed and REOPEN it so that it does > not get forgotten. Ok, I agree with reopening. Will try to tackle it asap then Thanks for reporting the problem, and sorry not to have fixed it completely before (I thought I had) Attachment 116624 [details] was posted by a committer and has review+, assigning to Mario Sanchez Prada for commit.
(In reply to comment #9) > [...] > Ok, I agree with reopening. Will try to tackle it asap then > > Thanks for reporting the problem, and sorry not to have fixed it completely before (I thought I had) JFTR, now working on this... Created attachment 121274 [details]
Patch proposal for the remaining issues + updated unit tests
This patch fixed the problem getting the current line, both for single line and multi-line text controls. It also fixes a problem getting the caret offset for text controls, which was causing trouble when retrieving the current line in multi-line text controls (so that's why it's included in this patch too, instead of a separate bug).
It also updates some caret-related unit tests in testatk.c, so we make sure we don't break this again in the future.
Comment on attachment 121274 [details] Patch proposal for the remaining issues + updated unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=121274&action=review Looks good, but consider the following changes. > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2717 > +AccessibilityObject* focusedObjectAndCaretOffsetUnignored(AccessibilityObject* objectOfReference, int& offset) You can just call objectOfReference referenceObject, if you like. > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2739 > + AccessibilityObject* realObject = focusedObject; I think I would prefer just continuing to use focusedObject here. After this point it seems unused and realObject is a little ambiguous. > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.h:63 > +WebCore::AccessibilityObject* focusedObjectAndCaretOffsetUnignored(WebCore::AccessibilityObject* objectOfReference, int& offset); objectFocusedAndCaretOffsetUnignored might be clearer. You can just omit the parameter name here instead of calling it objectOfReference. Comment on attachment 121274 [details] Patch proposal for the remaining issues + updated unit tests Attachment 121274 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11147137 New failing tests: http/tests/inspector/network/download.html Comment on attachment 121274 [details]
Patch proposal for the remaining issues + updated unit tests
Remove cq bit since I think there are a few style changes I'd like Mario to consider before landing this patch.
Comment on attachment 121274 [details] Patch proposal for the remaining issues + updated unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=121274&action=review Thanks for the review, Martin. I'm applying your suggestions to the patch and committing after that. See my comments below. >> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2717 >> +AccessibilityObject* focusedObjectAndCaretOffsetUnignored(AccessibilityObject* objectOfReference, int& offset) > > You can just call objectOfReference referenceObject, if you like. Ok. Changed. >> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2739 >> + AccessibilityObject* realObject = focusedObject; > > I think I would prefer just continuing to use focusedObject here. After this point it seems unused and realObject is a little ambiguous. Sounds fine to me. Just used realObject since it was what we had before this patch, but always using focusedObject sounds clearer to me too. >> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.h:63 >> +WebCore::AccessibilityObject* focusedObjectAndCaretOffsetUnignored(WebCore::AccessibilityObject* objectOfReference, int& offset); > > objectFocusedAndCaretOffsetUnignored might be clearer. You can just omit the parameter name here instead of calling it objectOfReference. Ok. Changed Committed r104446: <http://trac.webkit.org/changeset/104446> |