Bug 25898 – [Gtk] object:text-changed events should be emitted for entries and password text

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

At the moment, inserting and deleting entry text causes object:text-caret-moved events to be emitted. (Thanks!) In addition, object:text-changed:{insert,delete} should be emitted to inform ATs of:

* the offset at which the change took place (detail1)
* the length of the inserted/deleted text (detail2)
* the string which was inserted or removed (any_data)

See http://library.gnome.org/devel/atk/unstable/AtkText.html#AtkText-text-changed for more info.

[Mario Sanchez Prada]

I was working on this bug for some time and at the end I manage to find a way that, hopefully, would make sense to allow notifying AT's when a piece of text is modified, by sending the apropriate signals, as described by Joanmarie.

What I did was basically to follow the lead of what it was done with the text-caret-moved signal for GTK, which uses a dummy function in WebCore/editing/SelectionController.h for other platforms, providing a specific implementation for GTK through a WebCore/editing/gtk/SelectionControllerGtk.h file.

Hence, after detecting that for the case of text-changed signal a11y should be notified from four commands (InsertIntoTextNode, DeleteFromTextNode, InsertNodeBefore and AppendNode), I followed the same idea and added a new notifyAccessibility() function to the header files in WebCore/editing for those four commands, implementing them through four new GTK-specific files under WebCore/editing/gtk. Most likely, you'll see some refactorization would be in place as the code in those 4 files is very similar, but at this stage (showing my patches) I preferred to keep everything separated to ease reviewing and understanding the patches.

Last, but not least, I also needed to make a couple of small changes in WebCore/edition/DeleteSelectionCommand.cpp to ensure the DeleteFromTextNodeCommand is executed always when removing a text node, so accessibility could be notified as well in that case (I found that, without this change, at some situations a11y wouldn't have a way to notice when text is deleted, e.g., when deleting all the text in a field at once).

Hence, I'm now attaching 5 patches, the first one implementing the changes in DeleteSelectionCommand.cpp I mentioned above and the other 4 implementing the modifications to each of those 4 commands: InsertIntoTextNode, DeleteFromTextNode, InsertNodeBefore and AppendNode. 

Eager to get some feedback! :-)

PS: I've run all the layout tests with and without my changes and no regressions appeared, so I feel confident this shouldn't break anything, but you know... life is unpredictable :-)

[Mario Sanchez Prada]

Created attachment 60944 [details]
1. Call deleteTextFromNode() when needed while removing text nodes

Do it even when removeNode() is going to be called afterwards, in order to allow accessibility to get properly notified about the text being removed alongside with that node.

[Mario Sanchez Prada]

Created attachment 60945 [details]
2. New InsertIntoTextNodeCommand::notifyAccessibility() private method

Added a dummy version for all platforms but GTK, which comes with a specific implementation to handle the text-changed events

[Mario Sanchez Prada]

Created attachment 60946 [details]
3. New DeleteFromTextNodeCommand::notifyAccessibility() private method

Added a dummy version for all platforms but GTK, which comes with a specific implementation to handle the text-changed events

[Mario Sanchez Prada]

Created attachment 60947 [details]
4. New InsertNodeBeforeCommand::notifyAccessibility() private method

Added a dummy version for all platforms but GTK, which comes with a specific implementation to handle the text-changed events

[Mario Sanchez Prada]

Created attachment 60948 [details]
5. New AppendNodeCommand::notifyAccessibility() private method

Added a dummy version for all platforms but GTK, which comes with a specific implementation to handle the text-changed events

[chris fleizach]

How come the existing methods that handle value changing (you can find by searching for AXValueChange) are not catching these cases?

i think have the notifyAccessibility code in a gtk Text handler is pretty limiting. it should probably work within the existing notification system that AX has, by calling postNotification directly or a specific handler within AXObjectCache

[Mario Sanchez Prada]

Hi Chris, first of all thanks a lot for your comment. I'm pretty new to this WebKit a11y world and I appreciate a lot these kind of comments, really helpful indeed.

(In reply to comment #7)
> How come the existing methods that handle value changing (you can find by 
> searching for AXValueChange) are not catching these cases?

Didn't say those methods are not handling that, I've just didn't know of them so perhaps they're working and nothing of this mess I did was needed after all. 
I'll check it out right now as I'd be more than glad if I could find a better and not so ad-hoce solution to this problem, specially considering it touches the core, and not just the gtk layer.

> i think have the notifyAccessibility code in a gtk Text handler is pretty 
> limiting. it should probably work within the existing notification system 
> that AX has, by calling postNotification directly or a specific handler 
> within AXObjectCache

I'll try this out as I also agree my current implementation it's pretty limiting... but at least it achieved one of its functions: to trigget some discussion and get some feedback to head towards the right direction :-)

Thanks!

[chris fleizach]

 
> (In reply to comment #7)
> > How come the existing methods that handle value changing (you can find by 
> > searching for AXValueChange) are not catching these cases?
> 
> Didn't say those methods are not handling that, I've just didn't know of them so perhaps they're working and nothing of this mess I did was needed after all. 
> I'll check it out right now as I'd be more than glad if I could find a better and not so ad-hoce solution to this problem, specially considering it touches the core, and not just the gtk layer.
>

Great. I think the existing value change notifications are pretty good chokepoints for handling text changes, but there might be missing cases.

Is this to address when you're doing something like <div contenteditable=true> and want to know text changes or does GTK have true cursor support and navigation?

[Mario Sanchez Prada]

(In reply to comment #9)
> Great. I think the existing value change notifications are pretty good 
> chokepoints for handling text changes, but there might be missing cases.

Indeed. Actually I've just found there's a nice function -already GTK specific- called postPlatformNotification() that looks like the way to go, at least for notifying of text insertions (I'm not so sure about deletions as typically, at least in gtk & clutter, those notifications should be emitted *before* the actual deletion happen).

Let's see how it works.

> Is this to address when you're doing something like <div contenteditable=true> 
> and want to know text changes or does GTK have true cursor support and 
> navigation?

Not sure what you mean here. The idea is just to allow ATs get notified about text insertions and deletions (that includes typing new text, deleting individual char, replacing/deleting selections, copying from clipboard...) through the proper signal, specifying the offset and length of the piece of text being inserted or deleted. 

Once this info is emitted, the ATs will use that (offset, length) pair to find the piece of text being inserted/deleted in the platform-specific a11y object (an AtkObject, in GTK port). That's exactly why notifications about deletions should happen before the actual deletion: to find the text which is going to be deleted with the original text + the (offset, length) pair coming with the signal.

Have I answered, at least partially or indirectly, your question?

[Mario Sanchez Prada]

(In reply to comment #10)
> (In reply to comment #9)
> > Great. I think the existing value change notifications are pretty good 
> > chokepoints for handling text changes, but there might be missing cases.
> 
> Indeed. Actually I've just found there's a nice function -already GTK specific- 
> called postPlatformNotification() that looks like the way to go, at least for 
> notifying of text insertions (I'm not so sure about deletions as typically, at 
> least in gtk & clutter, those notifications should be emitted *before* the 
> actual deletion happen).
> 
> Let's see how it works.

Hmm... I found another problem when trying to implement this (I mean, apart from the issue of having to emit the signal before deleting text):

For properly emitting this signal, I need to know both the offset and the length of the piece of text being inserted or deleted, which is a kind of information I do have clearly available when implementing this through the notifyAccessibility() method in InsertIntoTextNodeCommand, DeleteFromTextNodeCommand, InsertNodeBeforeCommand and AppendNodeCommand.

However, when I try to extract this information from AXObjectCache::postPlatformNotification(), I'm having a hard time to do it so since that information seems to have got lost up in the backtrace, far before calling to AXObjectCache::postNotification() in the first place, and I wonder if, given this requirements, perhaps after all the initial approach of doing this more ad-hoc in WebCore/editing could be the right way to do. Or perhaps there's actually a way to know both the offset and length for insertions and deletions from AXObjectCache that I'm missing... don't know yet.

Perhaps, this is the same rationale behind the WebCore/editing/gtk/SelectionControllerGtk.cpp file, used to properly emit the text-caret-moved signal... Xan, could you please comment a bit on this?

Thanks

[chris fleizach]

> Hmm... I found another problem when trying to implement this (I mean, apart from the issue of having to emit the signal before deleting text):
> 
> For properly emitting this signal, I need to know both the offset and the length of the piece of text being inserted or deleted, which is a kind of information I do have clearly available when implementing this through the notifyAccessibility() method in InsertIntoTextNodeCommand, DeleteFromTextNodeCommand, InsertNodeBeforeCommand and AppendNodeCommand.
> 
> However, when I try to extract this information from AXObjectCache::postPlatformNotification(), I'm having a hard time to do it so since that information seems to have got lost up in the backtrace, far before calling to AXObjectCache::postNotification() in the first place, and I wonder if, given this requirements, perhaps after all the initial approach of doing this more ad-hoc in WebCore/editing could be the right way to do. Or perhaps there's actually a way to know both the offset and length for insertions and deletions from AXObjectCache that I'm missing... don't know yet.
> 
> Perhaps, this is the same rationale behind the WebCore/editing/gtk/SelectionControllerGtk.cpp file, used to properly emit the text-caret-moved signal... Xan, could you please comment a bit on this?
> 
> Thanks

On the Mac side, its up to the screen reader to determine what has changed. the screen reader is only notified that the value has changed (that design decision was made a long time ago). 

If however, GTK needs more explicit information then your method of adding accessibility calls in each specific place is a reasonable approach, but I think you should either
a) make specific methods for each type of text iteration in AXObjectCache that handles it
b) make a new kind of AXNotification for each type of thing you need
    if need be, you might have to add a parameter argument to one of the postNotification methods

i'd just like to see the code be in a common WebCore method so that other platforms could use if necessary

[Mario Sanchez Prada]

First of all, sorry for not coming back to you before... I was busy with other matters this day and couldn't come to this earlier

(In reply to comment #12)
> [...]
> On the Mac side, its up to the screen reader to determine what has changed. the screen reader is only notified that the value has changed (that design decision was made a long time ago). 

Thanks for the feedback, now I better understand the whole picture.

> If however, GTK needs more explicit information then your method of adding accessibility calls in each specific place is a reasonable approach, but I think you should either
> a) make specific methods for each type of text iteration in AXObjectCache that handles it
> b) make a new kind of AXNotification for each type of thing you need
>     if need be, you might have to add a parameter argument to one of the postNotification methods
> 
> i'd just like to see the code be in a common WebCore method so that other platforms could use if necessary

I'm not really sure about what you mean with the points above... could you ellaborate a bit more on them?

Thanks in advance

[chris fleizach]

> > If however, GTK needs more explicit information then your method of adding accessibility calls in each specific place is a reasonable approach, but I think you should either
> > a) make specific methods for each type of text iteration in AXObjectCache that handles it
> > b) make a new kind of AXNotification for each type of thing you need
> >     if need be, you might have to add a parameter argument to one of the postNotification methods
> > 
> > i'd just like to see the code be in a common WebCore method so that other platforms could use if necessary
> 
> I'm not really sure about what you mean with the points above... could you ellaborate a bit more on them?
> 

If sending out a ValueChange notification alone is not sufficient, I recommend putting

void AppendNodeCommand::notifyAccessibility(bool isUndo)

inside of AXObjectCache

then having the GTK specific code live in either AXObjectCacheGTK or AccessibilityObjectGTK


> Thanks in advance

[Mario Sanchez Prada]

Created attachment 61782 [details]
2. New functions in AXObjectCache to call when text changes in a node

Added two new functions to allow notifying something changed in a text node either through a RenderObject or an AccessibilityObject, making those functions dependant on the platform-specific implementation, provided through a protected function.

Also, provided a GTK implementation of the platform-specific function (as this change was motivated by the GTK port), as well as dummy implementations for the other ports with accessibility support, that is, mac and win ports.

[Mario Sanchez Prada]

Created attachment 61783 [details]
3. Notify accessibility when something changes in a text node

Call to AXObjectCache::nodeTextChangeNotification() to notify when text was inserted/deleted when applying/unapplying a text edition command, along with the offset in the original text where the change took place and the number of characters that got affected.

[Mario Sanchez Prada]

(In reply to comment #14)
> [...]
> 
> If sending out a ValueChange notification alone is not sufficient, I recommend 
> putting
> 
> void AppendNodeCommand::notifyAccessibility(bool isUndo)
> 
> inside of AXObjectCache
> 
> then having the GTK specific code live in either AXObjectCacheGTK or 
> AccessibilityObjectGTK

Thanks for the clarification, Chris. As you can see I've attached two new patches replacing the old 2-5 patches (1st patch would be still valid as it is), hopefully properly addressing your suggestion of centralizing code in AXObjectCache and AXObjectCacheGtk.

I've tried this new way of doign things out and it works exactly the same, passing both the API and Layout tests as well, at least in the gtk build (I couldn't try to compile for other ports but the dummy implementation of nodeTextChangePlatformNotification for win and mac shouldn't be an issue)

[Early Warning System Bot]

Attachment 61783 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3559063

[WebKit Review Bot]

Attachment 61783 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3581053

[Mario Sanchez Prada]

(In reply to comment #18)
> Attachment 61783 [details] did not build on qt:
> Build output: http://webkit-commit-queue.appspot.com/results/3559063

(In reply to comment #19)
> Attachment 61783 [details] did not build on gtk:
> Build output: http://webkit-commit-queue.appspot.com/results/3581053

I was shocked at first with these messages but then I understood what's going on here is just that the WK bot is compiling the three patches separately, so obviously the third one won't compile as it depends on the second one.

I guess there's nothing I could do for this apart of merging both patches in a single one, but I don't think that would be a good idea as those are clearly two different patches IMHO. 

Anyway, if someone knows something better to be done wrt this, please speak up :-)

[WebKit Review Bot]

Attachment 61783 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3577057

[WebKit Review Bot]

Attachment 61783 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3412420

[chris fleizach]

Comment on attachment 61782 [details]
2. New functions in AXObjectCache to call when text changes in a node

This patch is just about there. I'm putting review- just because i'd like to see it one more time before it goes through. thanks!

WebCore/accessibility/AXObjectCache.cpp:473
 +  }
it looks like you don't really need this method, since its only used by the RenderObject version.

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:68
 +  
This looks like a roundabout way to get object->parentObject()->wrapper(). 

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:73
 +  
identation is wrong

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:88
 +  
identation is wrong

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:94
 +  }
looks like you should use a switch statement here which sets a char* or String to "text-change*". then you'd only have to call g_signal* one time.

WebCore/accessibility/win/AXObjectCacheWin.cpp:113
 +  1.7.0.4
extra line removal not needed

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:65
 +  
identation

[chris fleizach]

Comment on attachment 61783 [details]
3. Notify accessibility when something changes in a text node

Adding r=me. I think the changes you need to make are straightforward for this one. I think calling these methods can be mostly reduce to two lines for each instance

if (AXObjectCache::)
    document()->axCache()->nodeTextChange()

WebCore/editing/AppendNodeCommand.cpp:60
 +                                                                    0, len);
indentation

WebCore/editing/AppendNodeCommand.cpp:61
 +          }
do you need the line breaking here in nodeTextChangeNotification?

WebCore/editing/AppendNodeCommand.cpp:78
 +          }
I think the len > 0, node->nodeValue() != "\n" should go into the logic inside AXObjectCache. Different platforms may want to process those, so i think it should be left up to AX code to handle those cases.
it will also make these code snippets much simpler

WebCore/editing/DeleteFromTextNodeCommand.cpp:63
 +  
for one line if statements, no braces needed

WebCore/editing/DeleteFromTextNodeCommand.cpp:82
 +  }
no braces needed

WebCore/editing/InsertIntoTextNodeCommand.cpp:57
 +      }
no braces needed

WebCore/editing/InsertIntoTextNodeCommand.cpp:71
 +  
no braces needed

WebCore/editing/InsertNodeBeforeCommand.cpp:62
 +          }
same comments, let the AX code handle whether it should process this based on length or not

WebCore/editing/InsertNodeBeforeCommand.cpp:78
 +          }
ditto as above

WebCore/editing/InsertNodeBeforeCommand.cpp:84
 +  1.7.0.4
extra line removed, should not be

[chris fleizach]

Comment on attachment 60944 [details]
1. Call deleteTextFromNode() when needed while removing text nodes

Someone more familiar with text editing should take a look at this one

[Mario Sanchez Prada]

Created attachment 61859 [details]
2. New functions in AXObjectCache to call when text changes in a node

First of all, thanks for reviewing and sorry for marking the patch you set as review+ as obsolete, but I had to do it since I couldn't commit the changes by myself anyway (I'm not committer). So I'm now just replacing the old version of the patch with a new one so you could review it and, if it's correct, both mark it as review+ and commit-queue+ :-)

(In reply to comment #23)
> (From update of attachment 61782 [details])
> This patch is just about there. I'm putting review- just because i'd like to see it one more time before it goes through. thanks!
> 
> WebCore/accessibility/AXObjectCache.cpp:473
>  +  }
> it looks like you don't really need this method, since its only used by the RenderObject version.

Sure, you're right. I did it that way following the lead of the AXObject::postNotification() method, thinking that perhaps that was the way to go because of some yet-unknown reason to me... but I normally think needed methods should get in, and if you think the same that's a good reason for me to remove it.

So... done.

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:68
>  +  
> This looks like a roundabout way to get object->parentObject()->wrapper(). 

True. I'm still getting used to this forest of trees in WK and sometimes I get a bit lost... as in this case :-). Thanks for pointing it out, I've tried your suggestion and works also as a charm (but cleaner).

Done.

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:73
>  +  
> identation is wrong
> 
> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:88
>  +  
> identation is wrong

Fixed all indentation issues in all files.

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:94
>  +  }
> looks like you should use a switch statement here which sets a char* or String to "text-change*". then you'd only have to call g_signal* one time.

Done (I've used a GOwnPtr to avoid using g_free(), hope that's the right thing to do)

> WebCore/accessibility/win/AXObjectCacheWin.cpp:113
>  +  1.7.0.4
> extra line removal not needed

Not sure I understand this one... Isn't it just the end of the patch file?

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:65
>  +  
> identation

Fixed.

Thanks for the review. As I said at the beginning here you have attached a new patch addressing these issues

[Mario Sanchez Prada]

Created attachment 61861 [details]
3. Notify accessibility when something changes in a text node

Now attaching a new patch to replace this one as well...

(In reply to comment #24)
> (From update of attachment 61783 [details])
> Adding r=me. I think the changes you need to make are straightforward for this one. I think calling these methods can be mostly reduce to two lines for each instance
> if (AXObjectCache::)
>     document()->axCache()->nodeTextChange()

As I said in my previous comment, I'm not a committer so if you're ok with this new patch I'd appreciate that, if possible, you set the commit-queue+ flag alongside with the review+ (supposing it's correct!).

> WebCore/editing/AppendNodeCommand.cpp:60
>  +                                                                    0, len);
> indentation

Fixed, alongside with the rest of the indentation issues.

> WebCore/editing/AppendNodeCommand.cpp:61
>  +          }
> do you need the line breaking here in nodeTextChangeNotification?

I'm very used not to trespass the 80 line whenever possible so that's why I broke it. Anyway, I was checking other files and it seems in WK code it's not that strange to find long lines every now and then, so I've just changed this to one line as well.

> WebCore/editing/AppendNodeCommand.cpp:78
>  +          }
> I think the len > 0, node->nodeValue() != "\n" should go into the logic inside AXObjectCache. Different platforms may want to process those, so i think it should be left up to AX code to handle those cases.
> it will also make these code snippets much simpler

Absolutely. The length check should be in the platform specific code (and it actually is already there!), and doing it outside it's just plain useless and unneeded.

However, I need to leave the "\n" check in AppendNodeCommand since that's there to avoid to notify a11y twice, as in that case it happens that the InserNodeBeforeCommand will be always executed whenever a linebreak is done (while AppendNodecommand is only run when making a linebreak at the end of the text), so there's no need to do it also here (and it would even be a mistake IMHO)

> WebCore/editing/DeleteFromTextNodeCommand.cpp:63
>  +  
> for one line if statements, no braces needed

Done.

> WebCore/editing/DeleteFromTextNodeCommand.cpp:82
>  +  }
> no braces needed

Done

> WebCore/editing/InsertIntoTextNodeCommand.cpp:57
>  +      }
> no braces needed

Done

> WebCore/editing/InsertIntoTextNodeCommand.cpp:71
>  +  
> no braces needed

Done

> 
> WebCore/editing/InsertNodeBeforeCommand.cpp:62
>  +          }
> same comments, let the AX code handle whether it should process this based on length or not
> 
> WebCore/editing/InsertNodeBeforeCommand.cpp:78
>  +          }
> ditto as above
> 

Done

> WebCore/editing/InsertNodeBeforeCommand.cpp:84
>  +  1.7.0.4
> extra line removed, should not be

As I said in the previous command, I don't understand this thing :-)

[Early Warning System Bot]

Attachment 61861 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3590065

[WebKit Review Bot]

Attachment 61861 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3584083

[WebKit Review Bot]

Attachment 61861 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3378507

[chris fleizach]

(In reply to comment #30)
> Attachment 61861 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/3378507

Can you fix this build failure for chromium

[chris fleizach]

(In reply to comment #31)
> (In reply to comment #30)
> > Attachment 61861 [details] [details] did not build on chromium:
> > Build output: http://webkit-commit-queue.appspot.com/results/3378507
> 
> Can you fix this build failure for chromium

can't remember right now if chromium has their own AXObjectCache version that needs to be updated

[chris fleizach]

Comment on attachment 61859 [details]
2. New functions in AXObjectCache to call when text changes in a node

patch looks ok
r=me

[chris fleizach]

Comment on attachment 61861 [details]
3. Notify accessibility when something changes in a text node

patch looks ok
r=me

[chris fleizach]

Comment on attachment 60944 [details]
1. Call deleteTextFromNode() when needed while removing text nodes

Can you add a layout test for this, now that the groundwork is in place?

[WebKit Commit Bot]

Comment on attachment 61859 [details]
2. New functions in AXObjectCache to call when text changes in a node

Rejecting patch 61859 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
/bin/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/AXObjectCacheMac.o /Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm normal i386 objective-c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/3365513

[chris fleizach]

> 
> Full output: http://webkit-commit-queue.appspot.com/results/3365513

/Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm:117: warning: unused parameter 'object'
/Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm:117: warning: unused parameter 'textChange'
/Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm:117: warning: unused parameter 'offset'
/Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm:117: warning: unused parameter 'count'

please submit a new patch

[WebKit Commit Bot]

Comment on attachment 61861 [details]
3. Notify accessibility when something changes in a text node

Rejecting patch 61861 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
ild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/InsertNodeBeforeCommand.o /Users/eseidel/Projects/CommitQueue/WebCore/editing/InsertNodeBeforeCommand.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/DeleteFromTextNodeCommand.o /Users/eseidel/Projects/CommitQueue/WebCore/editing/DeleteFromTextNodeCommand.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(4 failures)


Full output: http://webkit-commit-queue.appspot.com/results/3366511

[Mario Sanchez Prada]

Created attachment 61880 [details]
2. New functions in AXObjectCache to call when text changes in a node

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > Attachment 61861 [details] [details] [details] did not build on chromium:
> > > Build output: http://webkit-commit-queue.appspot.com/results/3378507
> > 
> > Can you fix this build failure for chromium
> 
> can't remember right now if chromium has their own AXObjectCache version that needs to be updated

You're right, sorry for not realizing before:

  $ find . -name "AXObject*"
  ./WebCore/accessibility/gtk/AXObjectCacheAtk.cpp
  ./WebCore/accessibility/mac/AXObjectCacheMac.mm
  ./WebCore/accessibility/chromium/AXObjectCacheChromium.cpp
  ./WebCore/accessibility/win/AXObjectCacheWin.cpp
  ./WebCore/accessibility/AXObjectCache.cpp
  ./WebCore/accessibility/AXObjectCache.h

Added dummy implementation for chromium in the patch being attached now (as the root problem actually belonged to patch 61859, not 61861)

(In reply to comment #37)
> > 
> > Full output: http://webkit-commit-queue.appspot.com/results/3365513
> 
> /Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm:117: warning: unused parameter 'object'
> /Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm:117: warning: unused parameter 'textChange'
> /Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm:117: warning: unused parameter 'offset'
> /Users/eseidel/Projects/CommitQueue/WebCore/accessibility/mac/AXObjectCacheMac.mm:117: warning: unused parameter 'count'
> 
> please submit a new patch

Damm it... I didn't realize about the function header's params, sorry. Fixed in this patch for all dummy implementations (mac, win and chromium), as well as the inline declarations in AXObjectCache.h for those platforms which !HAVE(ACCESSIBILITY)

Sorry for the inconvenience... working till very late yesterday :-)

[Mario Sanchez Prada]

(In reply to comment #35)
> (From update of attachment 60944 [details])
> Can you add a layout test for this, now that the groundwork is in place?

Not sure whether a layout test would be needed, nor what exactly should I test if it was... could you (or anyone else) please give me a tip on this, if possible (so I could work on Monday on it)?

Thanks in advance.

[Mario Sanchez Prada]

(In reply to comment #38)
> (From update of attachment 61861 [details])
> Rejecting patch 61861 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
> Last 500 characters of output:
> ild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/InsertNodeBeforeCommand.o /Users/eseidel/Projects/CommitQueue/WebCore/editing/InsertNodeBeforeCommand.cpp normal i386 c++ com.apple.compilers.gcc.4_2
>     Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/DeleteFromTextNodeCommand.o /Users/eseidel/Projects/CommitQueue/WebCore/editing/DeleteFromTextNodeCommand.cpp normal i386 c++ com.apple.compilers.gcc.4_2
> (4 failures)
> 
> 
> Full output: http://webkit-commit-queue.appspot.com/results/3366511

This error looks to me like "normal" as this third patch couldn't be applied without having applied the second one first (as it was the case cause it got review-).

But if I'm wrong in my guessing just let me know and I'll happily work on fixing whatever is needed.

[Mario Sanchez Prada]

(In reply to comment #40)
> (In reply to comment #35)
> > (From update of attachment 60944 [details] [details])
> > Can you add a layout test for this, now that the groundwork is in place?
> 
> Not sure whether a layout test would be needed, nor what exactly should I test if it was... could you (or anyone else) please give me a tip on this, if possible (so I could work on Monday on it)?
> 
> Thanks in advance.

Btw, this first patch is needed for the other two to work correctly so I'd recommend not setting commit-queue+ on those until we feel this first one is absolutely complete and done.

Now time to start the weekend. See you on Monday

[chris fleizach]

Comment on attachment 61880 [details]
2. New functions in AXObjectCache to call when text changes in a node

r=me

[Mario Sanchez Prada]

(In reply to comment #25)
> (From update of attachment 60944 [details])
> Someone more familiar with text editing should take a look at this one

Just a small question, as I'm seeing this patch is the only one pending to be reviewed to fix this bug... who could I ask for reviewing this patch?

Guess there's not a big hurry for the time being, but I'd like to know whether I need to do something or not in order not to leave this bug stalled :-)

[chris fleizach]

(In reply to comment #44)
> (In reply to comment #25)
> > (From update of attachment 60944 [details] [details])
> > Someone more familiar with text editing should take a look at this one
> 
> Just a small question, as I'm seeing this patch is the only one pending to be reviewed to fix this bug... who could I ask for reviewing this patch?
> 
> Guess there's not a big hurry for the time being, but I'd like to know whether I need to do something or not in order not to leave this bug stalled :-)

good question. maybe xan lopez

[Mario Sanchez Prada]

(In reply to comment #45)
> [...]
> > Guess there's not a big hurry for the time being, but I'd like to know whether I need to do 
> something or not in order not to leave this bug stalled :-)
> 
> good question. maybe xan lopez

Great. I'll annoy him with this issue next week in GUADEC :-)

Thanks Chris for your help

[Ojan Vafai]

Comment on attachment 60944 [details]
1. Call deleteTextFromNode() when needed while removing text nodes

Can you add a test for the new behavior?

[Eric Seidel (no email)]

Comment on attachment 61783 [details]
3. Notify accessibility when something changes in a text node

Cleared Chris Fleizach's review+ from obsolete attachment 61783 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 61859 [details]
2. New functions in AXObjectCache to call when text changes in a node

Cleared Chris Fleizach's review+ from obsolete attachment 61859 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Mario Sanchez Prada]

(In reply to comment #47)
> (From update of attachment 60944 [details])
> Can you add a test for the new behavior?

As I said above, not sure what should I test here... could you please provide some tips?

Thanks in advance

[Mario Sanchez Prada]

(In reply to comment #50)
> (In reply to comment #47)
> > (From update of attachment 60944 [details] [details])
> > Can you add a test for the new behavior?
> 
> As I said above, not sure what should I test here... could you please provide some tips?

I talked to Xan about this in GUADEC and he didn't have a clue either about how to test this, although he agrees that a test could be needed anyway to test the changes done so deep in the WebCore/editing area.

So, I'm now adding Enrica Casucci to CC as recommended by Xan, to see whether she (hopefully) could help me understand which kind of test should be done here (if any) in order to move this stuff forward.

Thanks in advance

[Mario Sanchez Prada]

(In reply to comment #51)
> So, I'm now adding Enrica Casucci to CC as recommended by Xan, to see whether 
> she (hopefully) could help me understand which kind of test should be done here 
> (if any) in order to move this stuff forward.

Looks like I forgot to actually add her to CC :-$. Now doing it so.

[Enrica Casucci]

(In reply to comment #52)
> (In reply to comment #51)
> > So, I'm now adding Enrica Casucci to CC as recommended by Xan, to see whether 
> > she (hopefully) could help me understand which kind of test should be done here 
> > (if any) in order to move this stuff forward.
> 
> Looks like I forgot to actually add her to CC :-$. Now doing it so.

Mario, I would greatly appreciate if you could merge all the patches into one.
I have a hard time understanding now which ones are in an which ones are out.
I'll be happy to review the editing specific part.
I would like to be able to assess the impact on these changes for platforms other than Gtk.

As far as testing, use as reference the folder accessibility under LayoutTests. You can find a lot of examples of tests for accessibility related stuff. Take a look at the source code of dumpRenderTree to get more details on the accessibilityController object and its capabilities.

[Mario Sanchez Prada]

(In reply to comment #53)
> Mario, I would greatly appreciate if you could merge all the patches into 
> one.

I did it in separate patches because I thought it would be better for reviewing them. But I do not have any problem squashing them all into just one patch, I'll upload it as soon as I have it done and tested.

> I have a hard time understanding now which ones are in an which ones are out.

Basically, none of them are in now.

Patches (2) and (3) are currently review+ but not committed yet since they're dependant on patch (1), the one I was actually asking you for reviewing it :-)

> I'll be happy to review the editing specific part.

Great, thanks!

> I would like to be able to assess the impact on these changes for platforms 
> other than Gtk.

Currently I just can test this for the GTK platform. I'll try to somehow measure the performance to see whether there's some regression on that regard.

> As far as testing, use as reference the folder accessibility under 
> LayoutTests. You can find a lot of examples of tests for accessibility 
> related stuff. Take a look at the source code of dumpRenderTree to get more 
> details on the accessibilityController object and its capabilities.

Been there, done that :-), actually my question was more about what kind of test I could do to check whether these changes are correct or not, I mean... whether you could have an idea of what kind of assertions, situations and so forth I should test to make sure the change is correct.

So far I've checked no regressions happened neither in the current Layout tests nor in the unit tests, not sure whether that's enough, though.

Thanks for the feedback, I'll be back soon with the squashed patch

[Mario Sanchez Prada]

Created attachment 63314 [details]
Single patch for fixing this bug

(In reply to comment #54)
> Thanks for the feedback, I'll be back soon with the squashed patch

As promised, attached a single patch for this

[Eric Seidel (no email)]

Comment on attachment 61861 [details]
3. Notify accessibility when something changes in a text node

Cleared Chris Fleizach's review+ from obsolete attachment 61861 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 61880 [details]
2. New functions in AXObjectCache to call when text changes in a node

Cleared Chris Fleizach's review+ from obsolete attachment 61880 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Mario Sanchez Prada]

(In reply to comment #55)
> Created an attachment (id=63314) [details]
> Single patch for fixing this bug
> 
> (In reply to comment #54)
> > Thanks for the feedback, I'll be back soon with the squashed patch
> 
> As promised, attached a single patch for this

I was doing some tests deleting huge amounts of text at once from input text fields (text areas), in order to see if my patch (specially because of the changes in WebCore/editing/DeleteSelectionCommand) degraded at some extent the performance, and the results are good, even quite surprising, I'd say:

 #chars       BEFORE      AFTER
================================
  10000        2 ms        2 ms
  20000        3 ms        1 ms
  30000        2 ms        1 ms
  40000        4 ms        1 ms
  50000        4 ms        2 ms
  60000        6 ms        1 ms
  70000        5 ms        2 ms
  80000        6 ms        2 ms
  90000        7 ms        2 ms
 100000        8 ms        2 ms
  ...          ...         ...
 250000       18 ms        5 ms
  ...          ...         ...
 500000       36 ms        9 ms
  ...          ...         ...
 750000       51 ms        14 ms
  ...          ...         ...
 900000       63 ms        17 ms
 910000       63 ms        17 ms
 920000       64 ms        16 ms
 930000       64 ms        16 ms
 940000       65 ms        17 ms
 950000       66 ms        16 ms
 960000       65 ms        17 ms
 970000       67 ms        17 ms
 980000       67 ms        16 ms
 990000       69 ms        17 ms
1000000       69 ms        17 ms


Explanation:
  - #chars: total number of characters in a text node which is going to be emptied
  - BEFORE: time involved in removing all the text in a row, WITHOUT the patch
  - AFTER: time involved in removing all the text in a row, WITH the patch

Notice that results are shown over 10000 characters being deleted because under that value the differences were practically inexistent.


As you can see there's no performance regression with the patch, but even an improvement, so I guess the patch is fine wrt performance. However, such an improvement is something I hardly can explain at this point, sorry... just let me say that I repeated this tests several times and the results were always similar for each situation. Perhaps, based on this information, someone else with more knowledge in how editing works can drop some light on this... or perhaps my tests are just crap, who knows :-)

Hope this information helps

[chris fleizach]

Comment on attachment 63314 [details]
Single patch for fixing this bug

All the accessibility code looks good. If someone can just review the changes in DeleteSelectionCommmand.cpp, this patch can be r+

[Eric Seidel (no email)]

Comment on attachment 63314 [details]
Single patch for fixing this bug

WebCore/accessibility/AXObjectCache.cpp:463
 +      RefPtr<AccessibilityObject> object = getOrCreate(renderer);
getOrCreate needlessly returns a PassRefPtr, sadly.  Not your fault, but sad that it does here since it holds the reference itself.

WebCore/accessibility/AXObjectCache.h:132
 +      void nodeTextChangeNotification(RenderObject*, AXTextChange, unsigned offset, unsigned count);
Can this be a more specific type than RenderObject*?  We're trying to move to a more strongly typed RenderTree usage.  The AX code is currently the worse offender.

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:62
 +      AccessibilityRenderObject* renderObject = static_cast<AccessibilityRenderObject*>(object);
Do we not have a toAccessibiltyRenderObject() function to do this cast more safely?

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:63
 +      RenderObject* renderer = renderObject->renderer();
This is confusing to names the ax renderer "renderObject".  I would have called it axRenderObject or axRenderer or similar.

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:66
 +      Node* node = renderer->node();
I'm surprised that AccessibilityRenderObject doesn't already have a node() helper method?

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:68
 +      // Calculate global offset (in the whole text, not just in the current node)
Please put this in a helper function.

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:80
 +      // Emit the right signal
Please put this in a helper function too.

WebCore/editing/AppendNodeCommand.cpp:58
 +              document()->axObjectCache()->nodeTextChangeNotification(m_node->renderer(), AXObjectCache::AXTextInserted, 0, len);
Can we pass a more specific type here?

WebCore/editing/AppendNodeCommand.cpp:54
 +      if (AXObjectCache::accessibilityEnabled()) {
It seems this block should be a helper function "sendAXTextChangedIngnoringLineBreaks"
Obviously you'd pass the type of notification.
If you wanted to share with the non-ignoring linebreaks case, you'd use an enum ShouldIgnoreLineBreaks { IgnoreLineBreaks, IncludingLineBreaks } and make "IncludingLineBreaks" the default value.

WebCore/editing/AppendNodeCommand.cpp:68
 +      if (AXObjectCache::accessibilityEnabled()) {
Copy/paste code is work of the devil. :p

WebCore/editing/DeleteSelectionCommand.cpp: 
 +          } else if (m_downstreamEnd.deprecatedEditingOffset() - startOffset > 0) {
Was this an ordered else-if for a reason?

WebCore/editing/DeleteSelectionCommand.cpp:459
 +              // just delete
This comment isn't helpful.

[Enrica Casucci]

I'm not sure I understand why you need to call deleteTextFromNode instead of removing the node as before and make there the notification for the accessibility. Am I missing something?
I also don't fully the get the meaning of the change in the order of the checks.
I'll apply the patch and look at the modified code.

[Mario Sanchez Prada]

(In reply to comment #61)
> I'm not sure I understand why you need to call deleteTextFromNode instead of 
> removing the node as before and make there the notification for the 
> accessibility. Am I missing something?

When I wrote that I wanted to support the needs of the signal to be emmitted (which need the offset and count values for a given deletion) both in the case of actively deleting a piece of text as in the case of undoing such an operation.

Hence, as those pieces of information are inside the the DeleteFromTextNode command there, which also provides both the doApply() and doUnapply() methods, I thought that would be the right place to notify accessibility, which makes it also coherent with the cases of InsertIntoTextNode, AppendNode and InsertNodeBefore commands.

But perhaps there's a way also to implement this, both working with do's and undo's, right there in DeleteSelectionCommand. Not sure yet, though. Any thought on this?

> I also don't fully the get the meaning of the change in the order of the 
> checks.

The change in the order is just to make sure we execute the "else if" branches where deleteFromTextNode() is present, even in the case the main "if" branch would be the way to go when removing all the text from a node (so we'd just remove the node, not the text from it first).

So actually there's no new logic there in the general case, just an extra step in the case when removing the node would be needed (removing the text first).

> I'll apply the patch and look at the modified code.

Thanks a lot for your time, Enrica. I'm not 100% sure either about the editing part (that's why I asked you directly) and I really appreciate you trying to help with this stuff.

[Mario Sanchez Prada]

Created attachment 63712 [details]
Single patch for fixing this bug

(In reply to comment #60)
> (From update of attachment 63314 [details])
> WebCore/accessibility/AXObjectCache.cpp:463
>  +      RefPtr<AccessibilityObject> object = getOrCreate(renderer);
> getOrCreate needlessly returns a PassRefPtr, sadly.  Not your fault, but sad that it does here since it holds the reference itself.

Actually, getOrCreate() is handling and returning a RefPtr, not a PassRefPtr, so I guess the only problem in the code is actually my fault and a matter of changing the following line in AXObjectCache.cpp:

  RefPtr<AccessibilityObject> object = getOrCreate(renderer);

...into something like this:

  AccessibilityObject* object = getOrCreate(renderer);

...which is what it's done all over the place and seems to me like the right way to go (done in current new attachment)
 

> WebCore/accessibility/AXObjectCache.h:132
>  +      void nodeTextChangeNotification(RenderObject*, AXTextChange, unsigned offset, unsigned count);
> Can this be a more specific type than RenderObject*?  We're trying to move to a more strongly typed RenderTree usage.  The AX code is currently the worse offender.

As far as I can see, it can't, because using an specific subclass like RenderText would be not correct, as other subclasses of RenderObject (such as RenderTextControl, which is NOT a subclass of RenderText) would also be valid for this method. So, I guess there's no best option here than accepting a RenderObject and later on checking whether is actually the right kind of object depending on the context (for the gtk port the rule of thumb would be that the associated platform-dependant a11y object (the wrapper) should implement the AtkText interface.

Also, looking for something else in AXObjectCache.h that could help me with this I think I found a similar situation with the AXObjectCache::contentChanged() method:

    // Called by a node when text or a text equivalent (e.g. alt) attribute is changed.
    void contentChanged(RenderObject*);

Looks like it could use a RenderObject because of a similar reason, not 100% sure though.

Anyway, feel free to point out that I'm completely wrong if you think so. I'll appreciate that.

 
> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:62
>  +      AccessibilityRenderObject* renderObject = static_cast<AccessibilityRenderObject*>(object);
> Do we not have a toAccessibiltyRenderObject() function to do this cast more safely?

Added to AccessibilityRenderObject.h, following the lead of other similar ones, such as toRenderText()
 

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:63
>  +      RenderObject* renderer = renderObject->renderer();
> This is confusing to names the ax renderer "renderObject".  I would have called it axRenderObject or axRenderer or similar.

Taken into account (although no longer a problem) in the new patch.


> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:66
>  +      Node* node = renderer->node();
> I'm surprised that AccessibilityRenderObject doesn't already have a node() helper method?

Fixed.

 
> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:68
>  +      // Calculate global offset (in the whole text, not just in the current node)
> Please put this in a helper function.
> 
> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:80
>  +      // Emit the right signal
> Please put this in a helper function too.

Done (both).


> WebCore/editing/AppendNodeCommand.cpp:58
>  +              document()->axObjectCache()->nodeTextChangeNotification(m_node->renderer(), AXObjectCache::AXTextInserted, 0, len);
> Can we pass a more specific type here?

I think we can't in this case. See rationale above.


> WebCore/editing/AppendNodeCommand.cpp:54
>  +      if (AXObjectCache::accessibilityEnabled()) {
> It seems this block should be a helper function "sendAXTextChangedIngnoringLineBreaks"
> Obviously you'd pass the type of notification.

Done that way

> If you wanted to share with the non-ignoring linebreaks case, you'd use an enum ShouldIgnoreLineBreaks { IgnoreLineBreaks, IncludingLineBreaks } and make "IncludingLineBreaks" the default value.

I thought about that, but in this case I think it's more than enough (and even cleaner) to implement it the other way.


> WebCore/editing/AppendNodeCommand.cpp:68
>  +      if (AXObjectCache::accessibilityEnabled()) {
> Copy/paste code is work of the devil. :p

Agreed, copy/paste along with "premature optimizations" are the "root of all evil".
Fixed.


> WebCore/editing/DeleteSelectionCommand.cpp: 
>  +          } else if (m_downstreamEnd.deprecatedEditingOffset() - startOffset > 0) {
> Was this an ordered else-if for a reason?

As explained in comments #1 and #62 (and maybe somewhere else as well), the idea is to make sure the deleteFromTextNode() function gets executed even when removing all the text of a node, so that "else if" must become a normal "if" and moved above the "if (removing-all-text) { removeNode() }" branch, to make sure it gets executed before removing the node.


> WebCore/editing/DeleteSelectionCommand.cpp:459
>  +              // just delete
> This comment isn't helpful.

I missed this one, thanks for the catch.


As you can see I'm now attaching a new patch addressing all the issues commented above but the thing about being more specific with the RenderObject parameter, as explained above.

Hope this new patch gets closer to the good solution, and thanks a lot for your thoroughful review, Eric.

[Eric Seidel (no email)]

This looks OK to me now.  Sounds like you're still waiting for a response from Enrica, but I'm happy to r+ it once that conversation is over (or she's welcome to).

[Mario Sanchez Prada]

(In reply to comment #64)
> This looks OK to me now.  Sounds like you're still waiting for a response from 
> Enrica, but I'm happy to r+ it once that conversation is over (or she's welcome 
> to).

Great, let's wait for Enrica's opinion then.

[Mario Sanchez Prada]

(In reply to comment #65)
> [...]
> Great, let's wait for Enrica's opinion then.

I hate being perhaps too insistent with this so please Enrica accept my apologies in advance with regard to this comment O:-)

Just pointing out that I'll be off for two weeks starting next Monday so it would be great, if possible, to get some feedback on this patch before the end of this week. But please don't take me wrong, I'm not trying to put any extra pressure on this.

Thanks!

[Mario Sanchez Prada]

Created attachment 66367 [details]
Single patch for fixing this bug

(In reply to comment #66)
> [...]
> Just pointing out that I'll be off for two weeks starting next Monday so it would be great, if possible, to get some feedback on this patch before the end of this week. But please don't take me wrong, I'm not trying to put any extra pressure on this.

Attaching new patch rebased against lates version of trunk, as the old one was not applying at this moment.

[Eric Seidel (no email)]

Can someone explain to me (ideally in the ChangeLog) why Gtk is the only platform to need this change?  How do mac/win accomplish similar AX-related feats?

[Enrica Casucci]

Comment on attachment 66367 [details]
Single patch for fixing this bug

> +// Calculate global offset (in the whole text, not just in the current node)
> +static unsigned calculateGlobalOffset(AccessibilityRenderObject* object, unsigned offset)
> +{
> +    Node* node = object->node();
> +    unsigned globalOffset = offset;
> +    for (Node* n = node->parentNode()->firstChild(); n && (n != node); n = n->nextSibling()) {
> +        if (n->isTextNode())
> +            globalOffset += n->nodeValue().length();
> +    }
> +    return globalOffset;
> +}
> +
This is not the right way to do it. You should use the TextIterator class for this purpose. Your function doesn't take into account visibility of the text.

> +void AXObjectCache::nodeTextChangePlatformNotification(AccessibilityRenderObject* object, AXTextChange textChange, unsigned offset, unsigned count)
> +{
> +    // Sanity check
> +    if (!object || count < 1)
> +        return;
> +
> +    unsigned globalOffset = calculateGlobalOffset(object, offset);

There is no need to declare a variable here. You call emitTextChanged calling calculateGlobalOffset.
> +    emitTextChanged(object, textChange, globalOffset, count);
> +}
> +

>  
>  } // namespace WebCore
> diff --git a/WebCore/editing/DeleteSelectionCommand.cpp b/WebCore/editing/DeleteSelectionCommand.cpp
> index e57895c..71595f1 100644
> --- a/WebCore/editing/DeleteSelectionCommand.cpp
> +++ b/WebCore/editing/DeleteSelectionCommand.cpp
> @@ -443,11 +443,7 @@ void DeleteSelectionCommand::handleGeneralDelete()
>          return;
>  
>      if (startNode == m_downstreamEnd.node()) {
> -        // The selection to delete is all in one node.
> -        if (!startNode->renderer() || (startOffset == 0 && m_downstreamEnd.atLastEditingPositionForNode())) {
> -            // just delete
> -            removeNode(startNode);
> -        } else if (m_downstreamEnd.deprecatedEditingOffset() - startOffset > 0) {
> +        if (m_downstreamEnd.deprecatedEditingOffset() - startOffset > 0) {
>              if (startNode->isTextNode()) {
>                  // in a text node that needs to be trimmed
>                  Text* text = static_cast<Text*>(startNode);
> @@ -457,6 +453,10 @@ void DeleteSelectionCommand::handleGeneralDelete()
>                  m_endingPosition = m_upstreamStart;
>              }
>          }
> +
> +        // The selection to delete is all in one node.
> +        if (!startNode->renderer() || (!startOffset && m_downstreamEnd.atLastEditingPositionForNode()))
> +            removeNode(startNode);
>      }
>      else {
>          bool startNodeWasDescendantOfEndNode = m_upstreamStart.node()->isDescendantOf(m_downstreamEnd.node());
> @@ -472,6 +472,9 @@ void DeleteSelectionCommand::handleGeneralDelete()
>              } else {
>                  node = startNode->childNode(startOffset);
>              }
> +        } else if (startNode == m_upstreamEnd.node() && startNode->isTextNode()) {
> +            Text* text = static_cast<Text*>(m_upstreamEnd.node());
> +            deleteTextFromNode(text, 0, m_upstreamEnd.deprecatedEditingOffset());
>          }
>          
This is not clear to me. Could you explain what you're trying to do here? Maybe some additional comments would help.

  }
> diff --git a/WebCore/editing/InsertNodeBeforeCommand.cpp b/WebCore/editing/InsertNodeBeforeCommand.cpp
> index 2ce9846..6f16e08 100644
> --- a/WebCore/editing/InsertNodeBeforeCommand.cpp
> +++ b/WebCore/editing/InsertNodeBeforeCommand.cpp
> @@ -26,6 +26,7 @@
>  #include "config.h"
>  #include "InsertNodeBeforeCommand.h"
>  
> +#include "AXObjectCache.h"
>  #include "htmlediting.h"
>  
>  namespace WebCore {
> @@ -51,6 +52,11 @@ void InsertNodeBeforeCommand::doApply()
>  
>      ExceptionCode ec;
>      parent->insertBefore(m_insertChild.get(), m_refChild.get(), ec);
> +
> +    if (AXObjectCache::accessibilityEnabled()) {
> +        unsigned len = m_insertChild->nodeValue().length();

No need to declare len.
> +        document()->axObjectCache()->nodeTextChangeNotification(m_insertChild->renderer(), AXObjectCache::AXTextInserted, 0, len);
> +    }
>  }
>  
>  void InsertNodeBeforeCommand::doUnapply()
> @@ -58,6 +64,12 @@ void InsertNodeBeforeCommand::doUnapply()
>      if (!m_insertChild->isContentEditable())
>          return;
>          
> +    // Need to notify this before actually deleting the text
> +    if (AXObjectCache::accessibilityEnabled()) {
> +        unsigned len = m_insertChild->nodeValue().length();

Same as above.

> +        document()->axObjectCache()->nodeTextChangeNotification(m_insertChild->renderer(), AXObjectCache::AXTextDeleted, 0, len);
> +    }
> +
>      ExceptionCode ec;
>      m_insertChild->remove(ec);
>  }
> -- 
> 1.7.0.4
>

[Eric Seidel (no email)]

Comment on attachment 66367 [details]
Single patch for fixing this bug

r- based on above.

[Mario Sanchez Prada]

(In reply to comment #68)
> Can someone explain to me (ideally in the ChangeLog) why Gtk is the only platform to need 
> this change?  How do mac/win accomplish similar AX-related feats?

I don't know about win, but in Mac it works completely different so there's no need to emit some values with the signal (count and offset) as just emitting the signal would be enough (see Chris's explanation in comment #12).

But for the GTK port, if we want to provide an ATK-compliant solution, these changes are needed in order to emit that extra info along with the signal, so the AT technologies can be aware of exactly which parts of the text have changed.

[Mario Sanchez Prada]

(In reply to comment #69)
> (From update of attachment 66367 [details])
> > +// Calculate global offset (in the whole text, not just in the current node)
> > +static unsigned calculateGlobalOffset(AccessibilityRenderObject* object, unsigned offset)
> > +{
> > +    Node* node = object->node();
> > +    unsigned globalOffset = offset;
> > +    for (Node* n = node->parentNode()->firstChild(); n && (n != node); n = n->nextSibling()) {
> > +        if (n->isTextNode())
> > +            globalOffset += n->nodeValue().length();
> > +    }
> > +    return globalOffset;
> > +}
> > +
> This is not the right way to do it. You should use the TextIterator class for this purpose. Your function doesn't take into account visibility of the text.

I'm afraid you're right, although I still can't see clearly how to do that in the way you suggest. For instance, how do I get the range associated to the full text in a text entry so I can truly know the offset for a given position, even if that text is internally represented by different text nodes?

If you could ellaborate a bit more on this topic, I'd certainly appreciate it quite a lot.
Thanks in advance.

> > +void AXObjectCache::nodeTextChangePlatformNotification(AccessibilityRenderObject* object, AXTextChange textChange, unsigned offset, unsigned count)
> > +{
> > +    // Sanity check
> > +    if (!object || count < 1)
> > +        return;
> > +
> > +    unsigned globalOffset = calculateGlobalOffset(object, offset);
> 
> There is no need to declare a variable here. You call emitTextChanged calling calculateGlobalOffset.

Yes, I know, I did it here just in order not to get a too long line, that's all. But I don't mind removing that extra variable if getting a long line is not a problem.

> >  } // namespace WebCore
> > diff --git a/WebCore/editing/DeleteSelectionCommand.cpp b/WebCore/editing/DeleteSelectionCommand.cpp
> > index e57895c..71595f1 100644
> > --- a/WebCore/editing/DeleteSelectionCommand.cpp
> > +++ b/WebCore/editing/DeleteSelectionCommand.cpp
> > @@ -443,11 +443,7 @@ void DeleteSelectionCommand::handleGeneralDelete()
> >          return;
> >  
> >      if (startNode == m_downstreamEnd.node()) {
> > -        // The selection to delete is all in one node.
> > -        if (!startNode->renderer() || (startOffset == 0 && m_downstreamEnd.atLastEditingPositionForNode())) {
> > -            // just delete
> > -            removeNode(startNode);
> > -        } else if (m_downstreamEnd.deprecatedEditingOffset() - startOffset > 0) {
> > +        if (m_downstreamEnd.deprecatedEditingOffset() - startOffset > 0) {
> >              if (startNode->isTextNode()) {
> >                  // in a text node that needs to be trimmed
> >                  Text* text = static_cast<Text*>(startNode);
> > @@ -457,6 +453,10 @@ void DeleteSelectionCommand::handleGeneralDelete()
> >                  m_endingPosition = m_upstreamStart;
> >              }
> >          }
> > +
> > +        // The selection to delete is all in one node.
> > +        if (!startNode->renderer() || (!startOffset && m_downstreamEnd.atLastEditingPositionForNode()))
> > +            removeNode(startNode);
> >      }
> >      else {
> >          bool startNodeWasDescendantOfEndNode = m_upstreamStart.node()->isDescendantOf(m_downstreamEnd.node());
> > @@ -472,6 +472,9 @@ void DeleteSelectionCommand::handleGeneralDelete()
> >              } else {
> >                  node = startNode->childNode(startOffset);
> >              }
> > +        } else if (startNode == m_upstreamEnd.node() && startNode->isTextNode()) {
> > +            Text* text = static_cast<Text*>(m_upstreamEnd.node());
> > +            deleteTextFromNode(text, 0, m_upstreamEnd.deprecatedEditingOffset());
> >          }
> >          
> This is not clear to me. Could you explain what you're trying to do here? Maybe some additional comments would help.

See comment #62, but wrapping up what I'm doing here is just ensuring that deleteTextFromNode is going to be called always when deleting text (so I can emit the signal with the needed info -offset and count- for ATK-based AT technologies), even when we're deleting all the text from a node (so just removing the node could be enough).
 
>   }
> > diff --git a/WebCore/editing/InsertNodeBeforeCommand.cpp b/WebCore/editing/InsertNodeBeforeCommand.cpp
> > index 2ce9846..6f16e08 100644
> > --- a/WebCore/editing/InsertNodeBeforeCommand.cpp
> > +++ b/WebCore/editing/InsertNodeBeforeCommand.cpp
> > @@ -26,6 +26,7 @@
> >  #include "config.h"
> >  #include "InsertNodeBeforeCommand.h"
> >  
> > +#include "AXObjectCache.h"
> >  #include "htmlediting.h"
> >  
> >  namespace WebCore {
> > @@ -51,6 +52,11 @@ void InsertNodeBeforeCommand::doApply()
> >  
> >      ExceptionCode ec;
> >      parent->insertBefore(m_insertChild.get(), m_refChild.get(), ec);
> > +
> > +    if (AXObjectCache::accessibilityEnabled()) {
> > +        unsigned len = m_insertChild->nodeValue().length();
> 
> No need to declare len.

Same reason here: not to get too long lines. I'll change it anyway

> > +        document()->axObjectCache()->nodeTextChangeNotification(m_insertChild->renderer(), AXObjectCache::AXTextInserted, 0, len);
> > +    }
> >  }
> >  
> >  void InsertNodeBeforeCommand::doUnapply()
> > @@ -58,6 +64,12 @@ void InsertNodeBeforeCommand::doUnapply()
> >      if (!m_insertChild->isContentEditable())
> >          return;
> >          
> > +    // Need to notify this before actually deleting the text
> > +    if (AXObjectCache::accessibilityEnabled()) {
> > +        unsigned len = m_insertChild->nodeValue().length();
> 
> Same as above.

Likewise.


Thanks a lot for the review Enrica, I think now we're really close to fix this bug once and for all. But first I need to address these issues, specially the one about calculateGlobalOffset() which is where I have more doubts. If you could put some light on that I'd appreciate it quite a lot.

[Enrica Casucci]

> I'm afraid you're right, although I still can't see clearly how to do that in the way you suggest. For instance, how do I get the range associated to the full text in a text entry so I can truly know the offset for a given position, even if that text is internally represented by different text nodes?
> 
> If you could ellaborate a bit more on this topic, I'd certainly appreciate it quite a lot.
> Thanks in advance.

Take a look at the static methods in TextIterator:
    static int rangeLength(const Range*, bool spacesForReplacedElements = false);
    static PassRefPtr<Range> rangeFromLocationAndLength(Element* scope, int rangeLocation, int rangeLength, bool spacesForReplacedElements = false);
    static PassRefPtr<Range> subrange(Range* entireRange, int characterOffset, int characterCount);
Examples of their use can be found in various places in the editing code. Take a look at IndentOutdentCommand.cpp for an example of use of rangeFromLocationAndLength.
CompositeEditCommand.cpp has examples of use of rangeLenght, that should be fairly close to what you need.
>

[Mario Sanchez Prada]

(In reply to comment #73)
> > I'm afraid you're right, although I still can't see clearly how to do that in the way you suggest. For instance, how do I get the range associated to the full text in a text entry so I can truly know the offset for a given position, even if that text is internally represented by different text nodes?
> > 
> > If you could ellaborate a bit more on this topic, I'd certainly appreciate it quite a lot.
> > Thanks in advance.
> 
> Take a look at the static methods in TextIterator:
>     static int rangeLength(const Range*, bool spacesForReplacedElements = false);
>     static PassRefPtr<Range> rangeFromLocationAndLength(Element* scope, int rangeLocation, int rangeLength, bool spacesForReplacedElements = false);
>     static PassRefPtr<Range> subrange(Range* entireRange, int characterOffset, int characterCount);
> Examples of their use can be found in various places in the editing code. Take a look at IndentOutdentCommand.cpp for an example of use of rangeFromLocationAndLength.
> CompositeEditCommand.cpp has examples of use of rangeLenght, that should be fairly close to what you need.
> >

Thanks Enrica, that's just what I needed. As soon as I finish addressing other issues in other patches I'll try this out.

[Mario Sanchez Prada]

Created attachment 67463 [details]
Single patch for fixing this bug

(In reply to comment #74)
> [...] 
> Thanks Enrica, that's just what I needed. As soon as I finish addressing other issues in other patches I'll try this out.

I'm now attaching a new patch addressing all the issues you (Enrica) pointed out so I hope this new patch to be better and, hopefully, closer to help fixing this bug :-)

Wrt the calculateGlobalOffset() function you (Enrica) were definitely right: I was messing too much with a loop iterating over nodes while TextIterator::rangeLength() would basically do everything I needed in a way better and cleaner fashion. Hence I removed that function in this new patch and just added the three-line code in the only place it was needed.

So, now asking for review once again. Hopefully this will be the last or one of the last times I bother you all with this stuff :-) Anyway, don't hesitate to ask whatever you want, of course.

PS: If you review+ it, please set the cq+ flag as well. Thanks!

[Enrica Casucci]

This looks good to me.

[Mario Sanchez Prada]

(In reply to comment #76)
> This looks good to me.

Thanks Enrica. 

It seems this was the last bit of the patch pending on review so, unless someone else wanted to point something else out now and if you all agree, it would be great if someone could add the r+ and cq+ flags (I'm not a committer).

[Martin Robinson]

Comment on attachment 67463 [details]
Single patch for fixing this bug

View in context: https://bugs.webkit.org/attachment.cgi?id=67463&action=prettypatch

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:78
> +    gchar* detailStr = detail.get();
> +    if (detailStr)
> +        g_signal_emit_by_name(wrapper, detailStr, offset, count);
> +}

This is unnecessary, because GOwnPtr has a implicit bool cast.
if (detail)
    g_signal_emit_by_name(wrapper, detail.get(), offset, count);

> WebCore/editing/AppendNodeCommand.cpp:51
> +    if (nodeValue != "\n")

I think this should be an early return.


This look good otherwise, but I'd be more comfortable if someone more familiar with a11y reviewed the platform-independent parts.

[Mario Sanchez Prada]

Created attachment 68105 [details]
Single patch for fixing this bug

(In reply to comment #78)
> (From update of attachment 67463 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67463&action=prettypatch
> 
> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:78
> > +    gchar* detailStr = detail.get();
> > +    if (detailStr)
> > +        g_signal_emit_by_name(wrapper, detailStr, offset, count);
> > +}
> 
> This is unnecessary, because GOwnPtr has a implicit bool cast.
> if (detail)
>     g_signal_emit_by_name(wrapper, detail.get(), offset, count);

Fixed

> > WebCore/editing/AppendNodeCommand.cpp:51
> > +    if (nodeValue != "\n")
> 
> I think this should be an early return.
> 

Done
 
> This look good otherwise, but I'd be more comfortable if someone more familiar with a11y reviewed the platform-independent parts.

I'm attaching a new patch now, asking for review over it, which I guess should be pretty straightforward since now it looks now both Chris and Enrica are ok with the changes in the a11y code and the text editing part, respectively (Besides, I guess both Eric and you will be ok with the current patch, after addressing the issues pointed by you two as well)

[Martin Robinson]

Comment on attachment 68105 [details]
Single patch for fixing this bug

Based on Enrica's previous comment, I'd say this patch seems good.

[WebKit Commit Bot]

Comment on attachment 68105 [details]
Single patch for fixing this bug

Rejecting patch 68105 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
OR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Users/abarth/git/webkit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh

** BUILD FAILED **


The following build commands failed:
WebCore:
	CompileC /Users/abarth/git/webkit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/AppendNodeCommand.o /Users/abarth/git/webkit-queue/WebCore/editing/AppendNodeCommand.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/4094011

[Mario Sanchez Prada]

(In reply to comment #81)
> (From update of attachment 68105 [details])
> Rejecting patch 68105 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
> Last 500 characters of output:
> OR 0320
>     setenv YACC /Developer/usr/bin/yacc
>     /bin/sh -c /Users/abarth/git/webkit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
> 
> ** BUILD FAILED **
> 
> 
> The following build commands failed:
> WebCore:
>     CompileC /Users/abarth/git/webkit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/AppendNodeCommand.o /Users/abarth/git/webkit-queue/WebCore/editing/AppendNodeCommand.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
> (1 failure)
> 
> 
> Full output: http://queues.webkit.org/results/4094011

I don't get this... What does it mean? Is it a problem in my patch or in the commit-queue scripts and does it require manual landing then?

Please let me know if there's anything I need to do from my side. Thanks!

[chris fleizach]

(In reply to comment #82)

build failed because of


cc1plus: warnings being treated as errors
/Users/abarth/git/webkit-queue/WebCore/editing/AppendNodeCommand.cpp:46: warning: unused parameter 'textChange'

[Mario Sanchez Prada]

Created attachment 68201 [details]
Single patch for fixing this bug

(In reply to comment #83)
> (In reply to comment #82)
> 
> build failed because of
> 
> 
> cc1plus: warnings being treated as errors
> /Users/abarth/git/webkit-queue/WebCore/editing/AppendNodeCommand.cpp:46: warning: unused parameter 'textChange'

Argh! Didn't see that, sorry.

Attaching new patch fixing that smal issue (the variable should actually be used, not hardcoded.)

[WebKit Commit Bot]

Comment on attachment 68201 [details]
Single patch for fixing this bug

Rejecting patch 68201 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
VERSION_MINOR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Users/abarth/git/webkit-queue/WebKitBuild/WebKit.build/Debug/WebKit.build/Script-5D88EE6C11407DE800BC3ABC.sh

** BUILD FAILED **


The following build commands failed:
WebKit:
	CompileC /Users/abarth/git/webkit-queue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/x86_64/WebFrame.o /Users/abarth/git/webkit-queue/WebKit/mac/WebView/WebFrame.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/4097013

[Mario Sanchez Prada]

Created attachment 68263 [details]
Single patch for fixing this bug

(In reply to comment #85)
> (From update of attachment 68201 [details])
> Rejecting patch 68201 from commit-queue.
> 
[...]
> 
> Full output: http://queues.webkit.org/results/4097013

I'm definitely doomed with this patch...

Looks the error is this: 

In file included from /Users/abarth/git/webkit-queue/WebKit/mac/WebView/WebFrame.mm:56:
/Users/abarth/git/webkit-queue/WebKitBuild/Debug/WebCore.framework/PrivateHeaders/AXObjectCache.h:29:39: error: AccessibilityRenderObject.h: No such file or directory
In file included from /Users/abarth/git/webkit-queue/WebKit/mac/WebView/WebFrame.mm:56:
[...]

And I bet it's because of this part of my patch:

--- a/WebCore/accessibility/AXObjectCache.h
+++ b/WebCore/accessibility/AXObjectCache.h
@@ -26,7 +26,7 @@
 #ifndef AXObjectCache_h
 #define AXObjectCache_h
 
-#include "AccessibilityObject.h"
+#include "AccessibilityRenderObject.h"
[...]


I think it was a mistake (my mistake) to remove #include "AccessibilityObject.h", even if it was ok to compile it in my local machine, and that's what is causing all that mess in the WebKit/mac/WebView/WebFrame.mm.

Now attaching a new patch that does not remove that line, and just adds "AccessibilityRenderObject.h" instead.

[WebKit Commit Bot]

Comment on attachment 68263 [details]
Single patch for fixing this bug

Rejecting patch 68263 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
AJOR 0300
    setenv XCODE_VERSION_MINOR 0320
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Script-5D88EE6C11407DE800BC3ABC.sh

** BUILD FAILED **


The following build commands failed:
WebKit:
	CompileC /Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Objects-normal/x86_64/WebFrame.o /Projects/CommitQueue/WebKit/mac/WebView/WebFrame.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/4050085

[Mario Sanchez Prada]

(In reply to comment #87)
> (From update of attachment 68263 [details])
> Rejecting patch 68263 from commit-queue.

 [...]

> Full output: http://queues.webkit.org/results/4050085

After the last patch it's obvious the situation improved but still it keeps pouring the following error:

In file included from /Projects/CommitQueue/WebKit/mac/WebView/WebFrame.mm:56:
/Projects/CommitQueue/WebKitBuild/Debug/WebCore.framework/PrivateHeaders/AXObjectCache.h:30:39: error: AccessibilityRenderObject.h: No such file or directory
In file included from /Projects/CommitQueue/WebKit/mac/WebView/WebFrame.mm:56:
/Projects/CommitQueue/WebKitBuild/Debug/WebCore.framework/PrivateHeaders/AXObjectCache.h:140: error: 'AccessibilityRenderObject' has not been declared
PhaseScriptExecution "Check For Framework Include Consistency" /Projects/CommitQueue/WebKitBuild/WebKit.build/Debug/WebKit.build/Script-5D88EE6C11407DE800BC3ABC.sh

At this point, it looks to me like some kind of mac-specific error, as if something were missing in some declaration (AccessibilityRenderObject.h in PrivateHeaders?) or something like that.

Could someone working in the mac port drop some light on this? That would be really helpful. Thanks!

[chris fleizach]

(In reply to comment #88)

AccessibilityRenderObject.cpp is not exported as a header, and I don't think it should be.

Really, anyone using Accessibility in WebKit, should only know about AXObjectCache and AccessibilityObject.

[Mario Sanchez Prada]

Created attachment 68346 [details]
Single patch for fixing this bug

(In reply to comment #89)
> (In reply to comment #88)
> 
> AccessibilityRenderObject.cpp is not exported as a header, and I don't think it should be.
> 
> Really, anyone using Accessibility in WebKit, should only know about AXObjectCache and AccessibilityObject.

That makes sense, and now that I look in AXObjectCache.h, I see the new functions I'm adding there are the only ones requiring AccessibilityRenderObject's as parameters, instead of just AccessibilityObject's.

Hence, attaching a new patch (hope this is the last strike!) where I changed the type of those parameters, letting the platform specific code to do the additional checks to make sure an AccessibilityRenderObject is being passed to nodeTextChangePlatformNotification in the case of GTK.

Sorry for so much hassle with this.

[Martin Robinson]

Comment on attachment 68346 [details]
Single patch for fixing this bug

View in context: https://bugs.webkit.org/attachment.cgi?id=68346&action=review

On further consideration, I'm pretty nervous that this does not hava test attached to it? Is it possible to write one?

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:73
> +    GOwnPtr<gchar> detail;
> +    switch (textChange) {
> +    case AXObjectCache::AXTextInserted:
> +        detail.set(g_strdup("text-changed::insert"));
> +        break;
> +    case AXObjectCache::AXTextDeleted:
> +        detail.set(g_strdup("text-changed::delete"));
> +        break;
> +    }

Please use CString here. Sorry I didn't notice this before.

[Mario Sanchez Prada]

Created attachment 68427 [details]
Single patch for fixing this bug + Unit test

(In reply to comment #91)
> (From update of attachment 68346 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68346&action=review
> 
> On further consideration, I'm pretty nervous that this does not hava test attached to it? Is it possible to write one?

Attaching a new patch with a new unit case in testatk.c to check this works as expected.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:73
> > +    GOwnPtr<gchar> detail;
> > +    switch (textChange) {
> > +    case AXObjectCache::AXTextInserted:
> > +        detail.set(g_strdup("text-changed::insert"));
> > +        break;
> > +    case AXObjectCache::AXTextDeleted:
> > +        detail.set(g_strdup("text-changed::delete"));
> > +        break;
> > +    }
> 
> Please use CString here. Sorry I didn't notice this before.

No problem. Done.

Let's see this time... :-)

[Martin Robinson]

Comment on attachment 68427 [details]
Single patch for fixing this bug + Unit test

Thanks!

[WebKit Commit Bot]

Comment on attachment 68427 [details]
Single patch for fixing this bug + Unit test

Clearing flags on attachment: 68427

Committed r68078: <http://trac.webkit.org/changeset/68078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.