[Webkit-unassigned] [Bug 153361] AX: new lines in content editable elements don't notify accessibility

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 1 23:39:03 PST 2016 

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

--- Comment #36 from Doug Russell <d_russell at apple.com> ---
(In reply to comment #32)
> Comment on attachment 272624 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272624&action=review
> 
> New approach looks a LOT better!
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:135
> > +        cache->postTextReplacementNotification(start.deepEquivalent().anchorNode(), AXTextEditTypeDelete, m_replacedText, type, text, selection.start());
> > +    else
> > +        cache->postTextStateChangeNotification(start.deepEquivalent().anchorNode(), type, text, selection.start());
> 
> Using anchorNode is really bad idea because position could be before/after a
> position.
> e.g. getting any data out of these text nodes might result in very
> unexpected results.
> What are you using these nodes for?

To look up the relevant accessibilityObject. I could probably use highestEditableRoot() instead if that makes more sense.

> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:101
> > +    Node* node = start.deepEquivalent().anchorNode();
> > +    if (!node)
> > +        return;
> > +    if (selection.isRange())
> > +        m_endIndexes.startIndex = indexForVisiblePosition(node, start);
> > +    m_endIndexes.endIndex = indexForVisiblePosition(node, end);
> 
> We should really use the version of indexForVisiblePosition the rest of
> editing code uses.
> The one that takes VisiblePosition as the first argument and the scope as
> the second argument.
> The variant you're using here won't work in many cases.

Good to know, this was some of the code I was hoping to get feedback on.

This version? int indexForVisiblePosition(const VisiblePosition& visiblePosition, RefPtr<ContainerNode>& scope)

How do I determine what to pass to scope?

> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:112
> > +    Node* node = start.deepEquivalent().anchorNode();
> > +    if (!node)
> > +        return;
> > +    m_endIndexes.startIndex = indexForVisiblePosition(node, start);
> 
> Ditto.
> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:131
> > +    VisiblePosition start = visiblePositionForIndexUsingCharacterIterator(node, m_endIndexes.startIndex);
> > +    VisiblePosition end = visiblePositionForIndexUsingCharacterIterator(node, m_endIndexes.endIndex);
> 
> Please use visiblePositionForIndex instead.

Ok.

> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:176
> > +    m_replacedText.setStartingSelection(currentSelection);
> 
> This doesn't make sense. "startingSelection" in editing usually refers to
> the selection before the editing command is being applied.
> What we have here instead is the current frame selection.

The naming there is definitely confusing. I'll try to improve it.

> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:321
> > -        command->m_composition = EditCommandComposition::create(document(), startingSelection(), endingSelection(), editingAction());
> > +        command->m_composition = EditCommandComposition::create(document(), startingSelection(), endingSelection(), frame().selection().selection(), editingAction());
> 
> Why do we need to store the frame selection's visible selection here?
> This should certainly needs to be explained in the change log.

I think that was a holdover I hadn't meant to leave in. I'll check and likely remove it.

> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:516
> > -        nodesToRemove.append(*node);
> > +        nodesToRemove.append(*node); 
> 
> Superflous change here.

Ok.

> 
> > Source/WebCore/editing/CompositeEditCommand.h:53
> > +    String m_undoText = String();
> > +    String m_replacedText = String();
> 
> Here's no need to initialize strings. They're null by default.
> I don't really like these member variable names though.
> It's not clear at all to me what undoText and replacedText refer to.

I'll work out clearer names. This logic is quite recent and reflects the flux in it's inconsistent naming.

> 
> Is undoText the text we're going to re-insert or remove?

Re-insert.

> 
> > Source/WebCore/editing/CompositeEditCommand.h:58
> > +    struct {
> > +        int startIndex = -1;
> > +        int endIndex = -1;
> > +    } m_endIndexes;
> 
> It's strange for a variable named m_endIndexes to contain endIndex. We
> should probably call this selectionRange or something.
> By the way, we shouldn't have to be storing these indices in the object
> since they're only needed to figure out what got deleted / inserted so they
> should be temporary variable
> that only exist during ReplaceSelectionCommand exists.

These are owned by the EditCommandComposition which I don't think ends up recreating a ReplaceSelectionCommand (could be wrong) to perform the undo.

> 
> > Source/WebCore/editing/CompositeEditCommand.h:92
> > +    AccessibilityUndoReplacedText m_replacedText;
> 
> We shouldn't always allocate this object.
> We try to keep EditCommandComposition small because it'll be kept in
> NSUndoManager for a long time.
> We should either put this into std::unique_ptr or create a subclass of
> EditCommandComposition which has this field
> and make ReplaceSelectionCommand create that variant instead.

I'll have to explore doing that. I don't think it can be unique since we need 1 per undo step. I can see about avoiding creating it if AX isn't on. I don't think it's specific to ReplaceSelection either, this logic is used to post all the notifications for unapply and reapply.

> 
> > Source/WebCore/editing/DictationCommand.cpp:112
> > +    // TODO: Test this in the ReplaceSelection case
> > +    // TODO: How do I write a test for this?
> 
> We don't use TODO.  Please use FIXME instead.

Will do. Those were just meant as notes before I finalized the patch.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:929
> > -    if ((selection.start().deprecatedNode()->renderer() && selection.start().deprecatedNode()->renderer()->style().userModify() == READ_WRITE_PLAINTEXT_ONLY)
> > -        && (selection.end().deprecatedNode()->renderer() && selection.end().deprecatedNode()->renderer()->style().userModify() == READ_WRITE_PLAINTEXT_ONLY))
> > +    if ((selection.start().deprecatedNode()->renderer() && selection.start().deprecatedNode()->renderer()->style().userModify() == READ_WRITE_PLAINTEXT_ONLY))
> > +        m_matchStyle = false;
> > +    else if (selection.end().deprecatedNode()->renderer() && selection.end().deprecatedNode()->renderer()->style().userModify() == READ_WRITE_PLAINTEXT_ONLY)
> >          m_matchStyle = false;
> 
> Why are you changing this code?
> It's not great to touch random lines of code like this because it would make
> it harder for us to track changes on svg/git blame.

Didn't mean to check that in.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.h:56
> > +    // Only populated when (editingAction() == EditActionPaste && AXObjectCache::accessibilityEnabled) == true
> 
> We don't like what comments like this.

Will remove.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.h:57
> > +    String insertedText() { return m_insertedText; }
> 
> Instead, rename this member function as well as the instance variable to
> something more descriptive.
> e.g. m_pastedTextForAccessibilityNotification.

Will do.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.h:116
> > +    String m_insertedText = String();
> 
> Again, there's no need to initialize String like this.

Ok.

> 
> > Source/WebCore/editing/TextInsertionBaseCommand.cpp:39
> > -TextInsertionBaseCommand::TextInsertionBaseCommand(Document& document)
> > -    : CompositeEditCommand(document)
> > +TextInsertionBaseCommand::TextInsertionBaseCommand(Document& document, EditAction editingAction)
> > +    : CompositeEditCommand(document, editingAction)
> 
> Is this ever used?
> If so, it should be explained in the change log.

Will add an explanation. Correct editingAction() will primarily effect what we do in undo.

> 
> > Source/WebCore/editing/TypingCommand.cpp:280
> > +        // TODO: Should call postTextStateChangeNotificationForDeletion()
> >          deleteSelection(m_smartDelete);
> >          return;
> >      case DeleteKey:
> > +        // Calls postTextStateChangeNotificationForDeletion()
> >          deleteKeyPressed(m_granularity, m_shouldAddToKillRing);
> >          return;
> >      case ForwardDeleteKey:
> > +        // Calls postTextStateChangeNotificationForDeletion()
> >          forwardDeleteKeyPressed(m_granularity, m_shouldAddToKillRing);
> 
> Please use FIXME instead.

Will do.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160302/29aade15/attachment.html>

More information about the webkit-unassigned mailing list