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

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

> Source/WebCore/editing/CompositeEditCommand.cpp:-83
> -Ref<EditCommandComposition> EditCommandComposition::create(Document& document,
> -    const VisibleSelection& startingSelection, const VisibleSelection& endingSelection, EditAction editAction)

This looks like a superfluous change.

> Source/WebCore/editing/CompositeEditCommand.cpp:162
> +    VisiblePosition position = visiblePositionForIndex(m_textInsertedByUnapplyRange.endIndex.value, m_textInsertedByUnapplyRange.endIndex.scope.get());
> +    if (position.isNull())
> +        position = visiblePositionForIndex(m_textInsertedByUnapplyRange.startIndex.value, m_textInsertedByUnapplyRange.startIndex.scope.get());
> +    if (position.isNull())
> +        return;

What is this check for?  Either define a local boolean with a descriptive name or add an explanation in the change log.

> Source/WebCore/editing/CompositeEditCommand.cpp:166
> +    String text = textInsertedByUnapply();

It's not at all clear to me what the difference between text and m_text is.
You should allocate a local variable references (i.e. of type const String&) with descriptive name of what these things are.

> Source/WebCore/editing/CompositeEditCommand.cpp:188
> +    VisiblePosition position = visiblePositionForIndex(m_textDeletedByUnapplyRange.startIndex.value, m_textDeletedByUnapplyRange.startIndex.scope.get());
> +    if (position.isNull())
> +        position = visiblePositionForIndex(m_textDeletedByUnapplyRange.endIndex.value, m_textDeletedByUnapplyRange.endIndex.scope.get());
> +    if (position.isNull())
> +        return;

Ditto.

> Source/WebCore/editing/CompositeEditCommand.cpp:193
> +    String text = textInsertedByReapply();
> +    if (m_text.length() && text.length())

Ditto.

> Source/WebCore/editing/CompositeEditCommand.cpp:213
> +    , m_replacedText(AccessibilityUndoReplacedText())

We shouldn't have to list this in the initialization list since we're just using the default constructor.

> Source/WebCore/editing/CompositeEditCommand.h:60
> +    String m_text;

I think this is really the text to be deleted?  So maybe we should call it m_textToBeDeleted?

> Source/WebCore/editing/CompositeEditCommand.h:88
> -    EditCommandComposition(Document&, const VisibleSelection& startingSelection, const VisibleSelection& endingSelection, EditAction);
> +    EditCommandComposition(Document&, const VisibleSelection&, const VisibleSelection&, EditAction);

I don't think this change is desirable. The fact the first argument is the starting selection and the second argument is the ending selection is useful information.

> Source/WebCore/editing/Editor.cpp:543
> -    applyCommand(ReplaceSelectionCommand::create(document(), fragment, options, editingAction));
> +    RefPtr<ReplaceSelectionCommand> cmd = ReplaceSelectionCommand::create(document(), fragment, options, editingAction);
> +    applyCommand(cmd);

Please don't use abbreviations like cmd. Spell out command.

> Source/WebCore/editing/Editor.cpp:1332
> +        String text = String();

No need to initialize string like this.  "String text;" will do the exact same thing.

> Source/WebCore/editing/TypingCommand.cpp:278
> +        // Calls postTextStateChangeNotificationForDeletion()

This is a what comment, not why comment.  Please remove it.
If anything, we should add some sort of an assertion to ensure the notification had happened instead.

> Source/WebCore/editing/TypingCommand.cpp:282
> +        // Calls postTextStateChangeNotificationForDeletion()

Ditto.

> Source/WebCore/editing/TypingCommand.cpp:418
> +{
> +    AccessibilityReplacedText replacedText(frame().selection().selection());
> +    insertLineBreak();
> +    replacedText.postTextStateChangeNotification(document().existingAXObjectCache(), AXTextEditTypeTyping, "\n", frame().selection().selection());
> +    composition()->setTextInsertedByUnapplyRange(replacedText.replacedRange());
> +}

Instead of modifying each call site of TypingCommand like this, can't we just capture the content
inside CompositeEditCommand::apply() like you're doing for EditCommand::apply?
We can check whether the selection is range there and capture the content if it is.

> Source/WebCore/editing/TypingCommand.cpp:580
> +
> +    postTextStateChangeNotificationForDeletion(selectionToDelete);
> +

Why do we need to manually call postTextStateChangeNotificationForDeletion here?
It's probably worth a why comment here.

> Source/WebCore/editing/TypingCommand.cpp:678
> +
> +    postTextStateChangeNotificationForDeletion(selectionToDelete);
> +

Ditto.

-- 
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/20160330/a3d6fc83/attachment-0001.html>