Bug 153361 – AX: new lines in content editable elements don't notify accessibility

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Doug Russell]

When inserting/deleting a new line in a content editable element, accessibility is not notified.

[Radar WebKit Bug Importer]

<rdar://problem/24302147>

[Doug Russell]

Created attachment 269591 [details]
Patch

[WebKit Commit Bot]

Attachment 269591 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/editing/EditCommand.cpp:195:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/editing/DeleteSelectionCommand.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 4 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Doug Russell]

Created attachment 269595 [details]
patch.patch

[chris fleizach]

Comment on attachment 269595 [details]
patch.patch

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

> Source/WebCore/ChangeLog:10
> +        Post notifications when adding/removing a new line in content/editable

period after this sentence

> Source/WebCore/editing/AppendNodeCommand.cpp:57
> +        simpleNotifyAccessibilityForTextChange(m_node.ptr(), applyEditType());

simple is a strange prefix I feel. i think that could be dropped

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

unnecessary change

> Source/WebCore/editing/DeleteSelectionCommand.cpp:87
> +    , m_shouldSuppressAccessibilityNotifications(shouldSuppressAccessibilityNotifications)

i think the "should" can be dropped from all these names. doesn't seem to add much to the concept

> Source/WebCore/editing/EditCommand.h:117
> +    void simpleNotifyAccessibilityForTextChange(Node *, AXTextEditType);

* in wrong place

[Ryosuke Niwa]

Comment on attachment 269595 [details]
patch.patch

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

> Source/WebCore/editing/EditCommand.cpp:235
> +static VisiblePosition accessibilityVisiblePositionForNode(Node* node)
> +{
> +    if (!node)
> +        return VisiblePosition();
> +    return is<Text>(node) ? Position(downcast<Text>(node), 0) : createLegacyEditingPosition(node, 0);
> +}

Please don't add new code to create a legacy visible position.  Use firstPositionInOrBeforeNode instead.

> Source/WebCore/editing/EditCommand.cpp:242
> +    String text = node->nodeValue();

I don't see why nodeValue could ever be the correct value to use here.
Should this be the visible text? 
Then we should be using TextIterator to get the text because nodeValue contains uncollapsed whitespace.
If we're making that change, then we can't synchronously fire these events as TextIterator requires up-to-date layout
and forcing layout upon each DOM mutation will be too expensive.

> Source/WebCore/editing/EditCommand.cpp:250
> +    // Don't consider linebreaks
> +    if (text == AccessibilityLineBreakValue)
> +        return "";
> +
> +    // We do want line breaks for BR and P tags
> +    if (!text.length()) {

This logic doesn't make any sense.  Whether \n matters or not depends on the computed style of the surrounding text.
\n could be a user visible line break with white-space: pre.
P tag could be turned into a inline element in which case it doesn't denote a line break all.
r- because of all those issues.

> Source/WebCore/html/HTMLDivElement.h:44
> +
> +    bool m_isParagraphSeparator = false;

We certainly don't want to grow every div element by eight bytes.
r- because this will regress WebCore's memory usage.

[Doug Russell]

Created attachment 272234 [details]
Patch

[chris fleizach]

Comment on attachment 272234 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:121
> +    if (AXObjectCache::accessibilityEnabled() && selection.isRange())

if ax is not enabled it seems like we should not do anything

> Source/WebCore/accessibility/AXObjectCache.cpp:124
> +        m_replacedText = String();

this seems unnecessary. i think this is what m_replacedText will be initialized to regardless

> Source/WebCore/editing/CompositeEditCommand.cpp:637
> +    default:

feel like we should ASSERT_NOT_REACHED for default: and expect someone will pass in Cut or Delete

> Source/WebCore/editing/DictationCommand.cpp:113
> +    notifyAccessibilityForTextChange(AXTextEditTypeDictation, m_textToInsert);

check Internals.cpp for whether dictation has some stubs to direct data through

> Source/WebCore/editing/Editor.cpp:543
> +    applyCommand(cmd);

this change seems unneeded

> Source/WebCore/editing/Editor.cpp:557
> +    if (AXObjectCache::accessibilityEnabled() && editingAction == EditActionPaste)

seems like we should cleanup memory when not being used

> Source/WebCore/editing/Editor.cpp:1283
> +    cache->postTextStateChangeNotification(node, AXTextEditTypeCut, text, start);

do we need to check for node == nil here? can postTextState changes handle the nil node case instead?

> Source/WebCore/editing/InsertTextCommand.cpp:143
> +        // FIXME: Need to capture the deleted selection and post replacement notification after insertion

any fixme: should probably have a webkit bug tied to it

> LayoutTests/accessibility/mac/value-change-userinfo.html:195
> +//             execExtendSelectionRightByCharacterCommand();

should this be deleted?

[Doug Russell]

(In reply to comment #8)
> Comment on attachment 272234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272234&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:121
> > +    if (AXObjectCache::accessibilityEnabled() && selection.isRange())
> 
> if ax is not enabled it seems like we should not do anything

Meaning we shouldn't initialize m_replacedText?

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:124
> > +        m_replacedText = String();
> 
> this seems unnecessary. i think this is what m_replacedText will be
> initialized to regardless

I'll have to check.

> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:637
> > +    default:
> 
> feel like we should ASSERT_NOT_REACHED for default: and expect someone will
> pass in Cut or Delete

Will do.

> 
> > Source/WebCore/editing/DictationCommand.cpp:113
> > +    notifyAccessibilityForTextChange(AXTextEditTypeDictation, m_textToInsert)
> 
> check Internals.cpp for whether dictation has some stubs to direct data
> through

Will do.

> 
> > Source/WebCore/editing/Editor.cpp:543
> > +    applyCommand(cmd);
> 
> this change seems unneeded

I need it for cmd->replacedText()

> 
> > Source/WebCore/editing/Editor.cpp:557
> > +    if (AXObjectCache::accessibilityEnabled() && editingAction == EditActionPaste)
> 
> seems like we should cleanup memory when not being used

I'm inclined to reclaim the cmd->replacedText() memory after this if regardless since it's not used again (although it might be in the incomplete undo logic.

> 
> > Source/WebCore/editing/Editor.cpp:1283
> > +    cache->postTextStateChangeNotification(node, AXTextEditTypeCut, text, start);
> 
> do we need to check for node == nil here? can postTextState changes handle
> the nil node case instead?

In the nil node case postTextStateChangeNotification ends up using the web area for the notification (on OS X) so I could drop that check.

> 
> > Source/WebCore/editing/InsertTextCommand.cpp:143
> > +        // FIXME: Need to capture the deleted selection and post replacement notification after insertion
> 
> any fixme: should probably have a webkit bug tied to it

That's out of date. I'll remove it.

> 
> > LayoutTests/accessibility/mac/value-change-userinfo.html:195
> > +//             execExtendSelectionRightByCharacterCommand();
> 
> should this be deleted?

Yes.

[Build Bot]

Comment on attachment 272234 [details]
Patch

Attachment 272234 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/883255

New failing tests:
fast/parser/strip-script-attrs-on-input.html
accessibility/mac/selection-value-changes-for-aria-textbox.html
editing/pasteboard/select-element-1.html

[Build Bot]

Created attachment 272238 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272234 [details]
Patch

Attachment 272234 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/883261

New failing tests:
accessibility/mac/selection-value-changes-for-aria-textbox.html
fast/parser/strip-script-attrs-on-input.html
editing/pasteboard/select-element-1.html

[Build Bot]

Created attachment 272239 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272234 [details]
Patch

Attachment 272234 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/883292

New failing tests:
accessibility/mac/selection-value-changes-for-aria-textbox.html
fast/parser/strip-script-attrs-on-input.html
editing/pasteboard/select-element-1.html

[Build Bot]

Created attachment 272244 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Doug Russell]

Created attachment 272359 [details]
Patch

[Doug Russell]

(In reply to comment #16)
> Created attachment 272359 [details]
> Patch

Fixed the build failures on GTK & Windows
Fixed the failing mac test (updated expect)

Could still use some guidance on building a useful VisibleSelection for notifying on undo.

[Build Bot]

Comment on attachment 272359 [details]
Patch

Attachment 272359 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/887305

New failing tests:
fast/parser/strip-script-attrs-on-input.html
editing/pasteboard/select-element-1.html

[Build Bot]

Created attachment 272364 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Doug Russell]

Created attachment 272369 [details]
Patch

[Doug Russell]

(In reply to comment #20)
> Created attachment 272369 [details]
> Patch

Fixed crashing text
Don't notify on cut unless accessibilityEnabled == true

[Doug Russell]

Created attachment 272496 [details]
Patch

[Doug Russell]

Created attachment 272612 [details]
Patch

[Doug Russell]

(In reply to comment #23)
> Created attachment 272612 [details]
> Patch

Partial work on undo. Getting unreliable string building on Paste and on undoing Paste.

[Doug Russell]

Created attachment 272613 [details]
Patch

[Doug Russell]

(In reply to comment #25)
> Created attachment 272613 [details]
> Patch

Fix patch not applying

[Doug Russell]

Created attachment 272624 [details]
Patch

[Build Bot]

Comment on attachment 272624 [details]
Patch

Attachment 272624 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/909127

New failing tests:
accessibility/mac/value-change-userinfo.html

[Build Bot]

Created attachment 272627 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272624 [details]
Patch

Attachment 272624 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/909141

New failing tests:
accessibility/mac/value-change-userinfo.html

[Build Bot]

Created attachment 272629 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

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?

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

[Doug Russell]

Created attachment 272638 [details]
Patch

[Doug Russell]

(In reply to comment #33)
> Created attachment 272638 [details]
> Patch

Just fixing tests (disabling one for wk1 for now), no review updates yet

[Ryosuke Niwa]

Comment on attachment 272638 [details]
Patch

Okay, clearing r? flag in that case (not to confuse other reviewers).

[Doug Russell]

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

[Doug Russell]

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

I was misreading the method signature. Good on this.

[Doug Russell]

Created attachment 273379 [details]
Patch

[WebKit Commit Bot]

Attachment 273379 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/ReplaceSelectionCommand.h:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Doug Russell]

Created attachment 273381 [details]
Patch

[Build Bot]

Comment on attachment 273381 [details]
Patch

Attachment 273381 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/945155

New failing tests:
accessibility/mac/value-change/value-change-user-info-textarea.html
accessibility/mac/value-change/value-change-user-info-textfield.html

[Build Bot]

Created attachment 273387 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

It looks like it's failing a bunch of tests on Mac and failing to build on GTK+?

[Build Bot]

Comment on attachment 273381 [details]
Patch

Attachment 273381 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/945150

New failing tests:
accessibility/mac/value-change/value-change-user-info-textarea.html
accessibility/mac/value-change/value-change-user-info-textfield.html

[Build Bot]

Created attachment 273388 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Doug Russell]

Created attachment 274262 [details]
Patch

[Doug Russell]

Created attachment 274264 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 274264 [details]
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.

[Doug Russell]

Created attachment 275266 [details]
Patch

[Doug Russell]

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

I'm not sure how I'd do that and have the correct context for if I should be capturing and what the user intent is.

[Ryosuke Niwa]

Comment on attachment 275266 [details]
Patch

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

r=me with comments.

> Source/WebCore/editing/CompositeEditCommand.cpp:88
> +void AccessibilityUndoReplacedText::confgureTextToBeDeletedByUnapplyIndexesWithEditCommandEndingSelection(const VisibleSelection& selection)

This is an overly long function name.  We don't normally use "configure" prefix these things.
I think something along the line of setTextDeletedByUnapplyRangeFromEndingSelection will suffice.

> Source/WebCore/editing/CompositeEditCommand.cpp:98
> +void AccessibilityUndoReplacedText::confgureTextToBeDeletedByUnapplyStartIndexWithEditCommandStartingSelection(const VisibleSelection& selection)

Ditto about the function name.

> Source/WebCore/editing/CompositeEditCommand.cpp:752
> +static EditAction deleteSelectionEditingActionForEditingAction(EditAction editingAction)

This function name doesn't tell us what kind of conversion we're doing.
This might be a rare case in which inlining the code below as in:
editingAction() == EditActionCut ? EditActionCut : EditActionDelete
might be more clear.

[Ryosuke Niwa]

Comment on attachment 275266 [details]
Patch

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

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

Again, we should call this variable m_deletedText or something to make it semantics clear.

[Doug Russell]

(In reply to comment #52)
> Comment on attachment 275266 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275266&action=review
> 
> > Source/WebCore/editing/CompositeEditCommand.h:60
> > +    String m_text;
> 
> Again, we should call this variable m_deletedText or something to make it
> semantics clear.

Renamed to m_replacedText; to match AccessibilityReplacedText

[Ryosuke Niwa]

http://trac.webkit.org/changeset/199030