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

[bugzilla-daemon at webkit.org]

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

--- Comment #32 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 272624
  --> https://bugs.webkit.org/attachment.cgi?id=272624
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?

> 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.

> 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.

> 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.

> 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.

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

Superflous change here.

> 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.

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

> 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.

> 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.

> 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.

> 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.

> Source/WebCore/editing/ReplaceSelectionCommand.h:56
> +    // Only populated when (editingAction() == EditActionPaste && AXObjectCache::accessibilityEnabled) == true

We don't like what comments like this.

> 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.

> Source/WebCore/editing/ReplaceSelectionCommand.h:116
> +    String m_insertedText = String();

Again, there's no need to initialize String like this.

> 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.

> 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.

-- 
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/b9f9c24d/attachment.html>