<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#c42">Comment # 42</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:cfleizach@apple.com" title="chris fleizach <cfleizach@apple.com>"> <span class="fn">chris fleizach</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=250290&action=diff" name="attach_250290" title="value change notifications">attachment 250290</a> <a href="attachment.cgi?id=250290&action=edit" title="value change notifications">[details]</a></span>
value change notifications
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=250290&action=review">https://bugs.webkit.org/attachment.cgi?id=250290&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:5
> + 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.</span >
description should go after the Reviewed by line
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:757
> + // Delegate to the right platform</span >
comment is unnecessary. we only need comments generally to explain why something is happening. what is happening should be self evident from the code
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:874
> + text = obj.passwordFieldOrContainingPasswordField()->passwordFieldValue().substring(position.deepEquivalent().deprecatedEditingOffset(), text.length());</span >
how is this sanitizing the value? is there not a method on HTMLInputType to give you the sanitized version already?
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:893
> + if (sanitizePasswordText(*obj, sanitizedText, position)) {</span >
why do we have this special code for password fields?
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:906
> + AccessibilityObject* obj = getOrCreate(node);</span >
this line is common to both platforms and can be moved above
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:924
> + if (obj) {</span >
if there's no obj can we do an early return?
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:938
> AccessibilityObject* obj = getOrCreate(node);</span >
ditto about AXObject
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1168
> + if (AccessibilityObject *rootObject = this->rootObject()) {</span >
is the root object always a ScrollView? I think it probably is. in that case we can cast to scroll view and call the webArea() method directly instead of relying on that it will be the first object
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.h:192
> + enum AXTextStateChangeType {</span >
what other change types can you envision?
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.h:201
> + AXTextEditTypeTyping, // Insert via typing</span >
how is insert/delete different from typing? why would you use typing instead of insert
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.h:210
> + void postTextReplacementNotification(Node*, AXTextEditType, const String&, AXTextEditType, const String&, const VisiblePosition&);</span >
why are there two AXTextEditTypes?
because there are two we should name them here in the header
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.h:240
> + AXTextDeleted,</span >
can you also have a text moved type? (i.e. you can drag blocks of text on OS X)
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.h:290
> + Timer m_passwordNotificationPostTimer;</span >
still curious why we need special timers for passwords
<span class="quote">> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:568
> + if (AXObjectCache *cache = axObjectCache())</span >
* on wrong side
<span class="quote">> Source/WebCore/accessibility/AccessibilityNodeObject.h:91
> + virtual bool isContainedByPasswordField() const override;</span >
i think we can move this implementation in AccessibilityObject and then we can remove the virtual
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:195
> +static NSDictionary* textReplacementChangeDictionary(AccessibilityObject* obj, AXObjectCache::AXTextEditType type, const String& string, const VisiblePosition& position)</span >
ObjcC stars should be on the other side
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:197
> + NSString* text = (NSString *)string;</span >
ditto about this star.
<span class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:246
> + if (!obj)</span >
is it possible to combine the implementation of this method with the more detailed one (or rather vice versa in this case)
<span class="quote">> Source/WebCore/editing/CutDeleteFromTextNodeCommand.h:44
> + virtual AXObjectCache::AXTextEditType applyEditType() override;</span >
should this be final override
<span class="quote">> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.cpp:53
> + case AXObjectCache::AXTextEditTypePaste:</span >
can you just make the default: case be ASSERT_NOT_REACHED</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>