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

Fri Aug 6 04:24:56 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=25898

Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63314|0                           |1
        is obsolete|                            |
  Attachment #63712|                            |review?
               Flag|                            |

--- Comment #63 from Mario Sanchez Prada <msanchez at igalia.com>  2010-08-06 04:24:55 PST ---
Created an attachment (id=63712)
 --> (https://bugs.webkit.org/attachment.cgi?id=63712)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.