<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#c43">Comment # 43</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: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=142719#c42">comment #42</a>)
<span class="quote">> 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>
>
> > 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.
>
> description should go after the Reviewed by line</span >
Will do
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.cpp:757
> > + // Delegate to the right platform
>
> 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 >
Will do
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.cpp:874
> > + text = obj.passwordFieldOrContainingPasswordField()->passwordFieldValue().substring(position.deepEquivalent().deprecatedEditingOffset(), text.length());
>
> how is this sanitizing the value? is there not a method on HTMLInputType to
> give you the sanitized version already?</span >
It's using passwordFieldValue() if the element is a password field or contained by a password field
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.cpp:893
> > + if (sanitizePasswordText(*obj, sanitizedText, position)) {
>
> why do we have this special code for password fields?</span >
To make sure that we're not posting password characters in notifications and to make sure we're posting password notifications at regular ticks so they can't be analyzed
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.cpp:906
> > + AccessibilityObject* obj = getOrCreate(node);
>
> this line is common to both platforms and can be moved above</span >
Will do
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.cpp:924
> > + if (obj) {
>
> if there's no obj can we do an early return?</span >
The Mac logic handles cases where obj == nullptr
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.cpp:938
> > AccessibilityObject* obj = getOrCreate(node);
>
> ditto about AXObject</span >
Will do
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.cpp:1168
> > + if (AccessibilityObject *rootObject = this->rootObject()) {
>
> 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 >
Sounds reasonable, I'll check.
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.h:192
> > + enum AXTextStateChangeType {
>
> what other change types can you envision?</span >
One I've considered is an explicit type for find and replace, especially in the multiple find and replace case where echoing the result might be complex. To answer a later review question, possibly a move type if there's a need to discern that from a delete followed by an insert.
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.h:201
> > + AXTextEditTypeTyping, // Insert via typing
>
> how is insert/delete different from typing? why would you use typing instead
> of insert</span >
A good example of insert not being typing is using javascript to set a form value
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.h:210
> > + void postTextReplacementNotification(Node*, AXTextEditType, const String&, AXTextEditType, const String&, const VisiblePosition&);
>
> why are there two AXTextEditTypes?
> because there are two we should name them here in the header</span >
For the deletion and insert, I'll add variable names
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.h:240
> > + AXTextDeleted,
>
> can you also have a text moved type? (i.e. you can drag blocks of text on OS
> X)</span >
Answered above
<span class="quote">>
> > Source/WebCore/accessibility/AXObjectCache.h:290
> > + Timer m_passwordNotificationPostTimer;
>
> still curious why we need special timers for passwords</span >
To thwart analyzing the cadence of the notifications to narrow down possible password
<span class="quote">>
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:568
> > + if (AXObjectCache *cache = axObjectCache())
>
> * on wrong side
>
> > Source/WebCore/accessibility/AccessibilityNodeObject.h:91
> > + virtual bool isContainedByPasswordField() const override;
>
> i think we can move this implementation in AccessibilityObject and then we
> can remove the virtual</span >
Will do
<span class="quote">>
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:195
> > +static NSDictionary* textReplacementChangeDictionary(AccessibilityObject* obj, AXObjectCache::AXTextEditType type, const String& string, const VisiblePosition& position)
>
> ObjcC stars should be on the other side</span >
Will do
<span class="quote">>
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:197
> > + NSString* text = (NSString *)string;
>
> ditto about this star.</span >
Will do
<span class="quote">>
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:246
> > + if (!obj)
>
> is it possible to combine the implementation of this method with the more
> detailed one (or rather vice versa in this case)</span >
Do you mean combine postTextStateChangePlatformNotification and postTextReplacementPlatformNotification?
<span class="quote">>
> > Source/WebCore/editing/CutDeleteFromTextNodeCommand.h:44
> > + virtual AXObjectCache::AXTextEditType applyEditType() override;
>
> should this be final override</span >
Will do
<span class="quote">>
> > Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.cpp:53
> > + case AXObjectCache::AXTextEditTypePaste:
>
> can you just make the default: case be ASSERT_NOT_REACHED</span >
I could do that for non debug builds, but not having a default means I get warnings if I change the enum which is quite helpful</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>