[Webkit-unassigned] [Bug 142719] AX: richer text change notifications

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=142719

--- Comment #43 from Doug Russell <d_russell at apple.com> ---
(In reply to comment #42)
> Comment on attachment 250290 [details]
> value change notifications
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250290&action=review
> 
> > 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

Will do

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

Will do

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

It's using passwordFieldValue() if the element is a password field or contained by a password field

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:893
> > +        if (sanitizePasswordText(*obj, sanitizedText, position)) {
> 
> why do we have this special code for password fields?

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

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:906
> > +    AccessibilityObject* obj = getOrCreate(node);
> 
> this line is common to both platforms and can be moved above

Will do

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:924
> > +    if (obj) {
> 
> if there's no obj can we do an early return?

The Mac logic handles cases where obj == nullptr

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:938
> >      AccessibilityObject* obj = getOrCreate(node);
> 
> ditto about AXObject

Will do

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

Sounds reasonable, I'll check.

> 
> > Source/WebCore/accessibility/AXObjectCache.h:192
> > +    enum AXTextStateChangeType {
> 
> what other change types can you envision?

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.

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

A good example of insert not being typing is using javascript to set a form value

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

For the deletion and insert, I'll add variable names

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

Answered above

> 
> > Source/WebCore/accessibility/AXObjectCache.h:290
> > +    Timer m_passwordNotificationPostTimer;
> 
> still curious why we need special timers for passwords

To thwart analyzing the cadence of the notifications to narrow down possible password

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

Will do

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

Will do

> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:197
> > +    NSString* text = (NSString *)string;
> 
> ditto about this star.

Will do

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

Do you mean combine postTextStateChangePlatformNotification and postTextReplacementPlatformNotification?

> 
> > Source/WebCore/editing/CutDeleteFromTextNodeCommand.h:44
> > +    virtual AXObjectCache::AXTextEditType applyEditType() override;
> 
> should this be final override

Will do

> 
> > Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.cpp:53
> > +    case AXObjectCache::AXTextEditTypePaste:
> 
> can you just make the default: case be ASSERT_NOT_REACHED

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150408/3f9af3e8/attachment.html>