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

[bugzilla-daemon at webkit.org]

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





--- Comment #72 from Mario Sanchez Prada <msanchez at igalia.com>  2010-09-08 04:14:16 PST ---
(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.

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