<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: new lines in content editable elements don't notify accessibility"
href="https://bugs.webkit.org/show_bug.cgi?id=153361#c36">Comment # 36</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: new lines in content editable elements don't notify accessibility"
href="https://bugs.webkit.org/show_bug.cgi?id=153361">bug 153361</a>
from <span class="vcard"><a class="email" href="mailto:d_russell@apple.com" title="Doug Russell <d_russell@apple.com>"> <span class="fn">Doug Russell</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=153361#c32">comment #32</a>)
<span class="quote">> Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=272624&action=diff" name="attach_272624" title="Patch">attachment 272624</a> <a href="attachment.cgi?id=272624&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=272624&action=review">https://bugs.webkit.org/attachment.cgi?id=272624&action=review</a>
>
> 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?</span >
To look up the relevant accessibilityObject. I could probably use highestEditableRoot() instead if that makes more sense.
<span class="quote">>
> > 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.</span >
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?
<span class="quote">>
> > 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.</span >
Ok.
<span class="quote">>
> > 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.</span >
The naming there is definitely confusing. I'll try to improve it.
<span class="quote">>
> > 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.</span >
I think that was a holdover I hadn't meant to leave in. I'll check and likely remove it.
<span class="quote">>
> > Source/WebCore/editing/CompositeEditCommand.cpp:516
> > - nodesToRemove.append(*node);
> > + nodesToRemove.append(*node);
>
> Superflous change here.</span >
Ok.
<span class="quote">>
> > 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.</span >
I'll work out clearer names. This logic is quite recent and reflects the flux in it's inconsistent naming.
<span class="quote">>
> Is undoText the text we're going to re-insert or remove?</span >
Re-insert.
<span class="quote">>
> > 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.</span >
These are owned by the EditCommandComposition which I don't think ends up recreating a ReplaceSelectionCommand (could be wrong) to perform the undo.
<span class="quote">>
> > 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.</span >
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.
<span class="quote">>
> > 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.</span >
Will do. Those were just meant as notes before I finalized the patch.
<span class="quote">>
> > 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.</span >
Didn't mean to check that in.
<span class="quote">>
> > Source/WebCore/editing/ReplaceSelectionCommand.h:56
> > + // Only populated when (editingAction() == EditActionPaste && AXObjectCache::accessibilityEnabled) == true
>
> We don't like what comments like this.</span >
Will remove.
<span class="quote">>
> > 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.</span >
Will do.
<span class="quote">>
> > Source/WebCore/editing/ReplaceSelectionCommand.h:116
> > + String m_insertedText = String();
>
> Again, there's no need to initialize String like this.</span >
Ok.
<span class="quote">>
> > 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.</span >
Will add an explanation. Correct editingAction() will primarily effect what we do in undo.
<span class="quote">>
> > 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.</span >
Will do.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>