<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: richer text change notifications"
href="https://bugs.webkit.org/show_bug.cgi?id=142719#c102">Comment # 102</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: richer text change notifications"
href="https://bugs.webkit.org/show_bug.cgi?id=142719">bug 142719</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=251507&action=diff" name="attach_251507" title="Update from review">attachment 251507</a> <a href="attachment.cgi?id=251507&action=edit" title="Update from review">[details]</a></span>
Update from review
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=251507&action=review">https://bugs.webkit.org/attachment.cgi?id=251507&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:2238
> +2015-04-14 Doug Russell <<a href="mailto:d_russell@apple.com">d_russell@apple.com</a>>
> +
> + AX: richer text change notifications
> + <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: richer text change notifications"
href="show_bug.cgi?id=142719">https://bugs.webkit.org/show_bug.cgi?id=142719</a>
> +
> + Richer accessibility value change notifications. Introduce AXTextEditType, postTextStateChangeNotification and postTextReplacementNotification to give assistive tech apps more reliable context for responding to changes in web content. Also implement a mechanism to post value changes in password form fields in coalesced ticks to thwart analyzing the cadence of changes.
> +
> +2015-04-14 Doug Russell <<a href="mailto:d_russell@apple.com">d_russell@apple.com</a>>
> +
> + AX: richer text change notifications
> + <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: richer text change notifications"
href="show_bug.cgi?id=142719">https://bugs.webkit.org/show_bug.cgi?id=142719</a>
> +
> + New EditCommand subclasses to give the context that accessibility needs to post more detailed value and selection change notifications. This commit is all infrastructure and doesn't contain any accessibility specific changes.
> +>>>>>>> <rdar://problem/20169058> AX: richer text change notifications (142719)
> +</span >
Extra change log entry here due to merging problems I assume.
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:339
> + NSAccessibilityPostNotificationWithUserInfo(webArea->wrapper(), NSAccessibilityValueChangedNotification, userInfo);
> + // Used by DRT to know when notifications are posted.
> + [webArea->wrapper() accessibilityPostedNotification:NSAccessibilityValueChangedNotification userInfo:userInfo];</span >
Use AXPostNotificationWithUserInfo.
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:343
> + NSAccessibilityPostNotificationWithUserInfo(object->wrapper(), NSAccessibilityValueChangedNotification, userInfo);
> + // Used by DRT to know when notifications are posted.
> + [object->wrapper() accessibilityPostedNotification:NSAccessibilityValueChangedNotification userInfo:userInfo];</span >
Use AXPostNotificationWithUserInfo.
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:386
> +#if PLATFORM(COCOA) && !PLATFORM(IOS)</span >
#if PLATFORM(MAC)
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:400
> + NSMutableArray *mutableArray = [array mutableCopy];</span >
A shame that this function makes a mutable copy of the array in the almost 100% common case where it does contain only JSON types.
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:419
> + NSMutableDictionary *mutableDictionary = [dictionary mutableCopy];</span >
A shame that this function makes a mutable copy of the dictionary in the almost 100% common case where it does contain only JSON types.
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.h:36
> +- (id)textMarkerRangeFromVisiblePositions:(WebCore::VisiblePosition)startPosition endPosition:(WebCore::VisiblePosition)endPosition;</span >
Shouldn’t the arguments be const VisiblePosition& like in the method just below?
<span class="quote">> Source/WebCore/editing/CompositeEditCommand.cpp:179
> + default:
> + break;</span >
Normally we omit the default so we get a warning when we add a new enum value and don’t yet handle it in this switch statement.
<span class="quote">> Source/WebCore/editing/CompositeEditCommand.h:36
> +class DeleteFromTextNodeCommand;</span >
Why is this needed? I don’t see it used below.
<span class="quote">> Source/WebCore/editing/CompositeEditCommand.h:63
> + AXTextEditType unapplyEditType();</span >
Should be a const member function.
<span class="quote">> Source/WebCore/editing/DeleteFromTextNodeCommand.cpp:53
> +const String& DeleteFromTextNodeCommand::deletedText()
> +{
> + return m_text;
> +}</span >
Should probably be inlined in the header. Just add "inline" and put it at the bottom of the header.
<span class="quote">> Source/WebCore/editing/EditCommand.cpp:133
> + default:
> + break;</span >
Same comment about default as above.
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:1035
> +AXTextStateChangeIntent inline FrameSelection::TextSelectionIntent(EAlteration alter, SelectionDirection direction, TextGranularity granularity)</span >
Why inline? That doesn’t seem like a good idea. Also, we normally put inline to the left of the type.
<span class="quote">> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56
> +const String& InsertIntoTextNodeCommand::insertedText()
> +{
> + return m_text;
> +}</span >
Inline in header?
<span class="quote">> Source/WebCore/editing/InsertIntoTextNodeCommand.h:44
> + const String& insertedText();
> +protected:
> + InsertIntoTextNodeCommand(PassRefPtr<Text> node, unsigned offset, const String& text, EditAction editingAction);</span >
Missing blank line before protected. Also, should just make this private again. I believe the reason to make it protected is obsolete.
<span class="quote">> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:33
> +class Text;</span >
Not needed.
<span class="quote">> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:35
> +class ReplaceDeleteFromTextNodeCommand : public DeleteFromTextNodeCommand {</span >
Should mark this class final.
<span class="quote">> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:37
> + static Ref<ReplaceDeleteFromTextNodeCommand> create(PassRefPtr<Text> node, unsigned offset, unsigned count)</span >
New code should not use PassRefPtr. See <<a href="https://www.webkit.org/coding/RefPtr.html">https://www.webkit.org/coding/RefPtr.html</a>>.
<span class="quote">> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:43
> + ReplaceDeleteFromTextNodeCommand(PassRefPtr<Text>, unsigned, unsigned);</span >
New code should not use PassRefPtr. See <<a href="https://www.webkit.org/coding/RefPtr.html">https://www.webkit.org/coding/RefPtr.html</a>>.
This should be private, not protected.
<span class="quote">> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:33
> +class Text;</span >
Not needed.
<span class="quote">> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:35
> +class ReplaceInsertIntoTextNodeCommand : public InsertIntoTextNodeCommand {</span >
Should mark this class final.
<span class="quote">> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:37
> + static Ref<ReplaceInsertIntoTextNodeCommand> create(PassRefPtr<Text> node, unsigned offset, const String& text, const String& deletedText, EditAction editingAction)</span >
New code should not use PassRefPtr. See <<a href="https://www.webkit.org/coding/RefPtr.html">https://www.webkit.org/coding/RefPtr.html</a>>.
<span class="quote">> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:43
> + ReplaceInsertIntoTextNodeCommand(PassRefPtr<Text>, unsigned, const String&, const String&, EditAction);</span >
New code should not use PassRefPtr. See <<a href="https://www.webkit.org/coding/RefPtr.html">https://www.webkit.org/coding/RefPtr.html</a>>.
This should be private, not protected.
<span class="quote">> Source/WebCore/editing/ReplaceSelectionCommand.h:124
> + AXTextEditType m_applyEditType;</span >
This looks uninitialized and unused. Please omit it.</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>