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

[bugzilla-daemon at webkit.org]

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

--- Comment #42 from chris fleizach <cfleizach at apple.com> ---
Comment on attachment 250290
  --> https://bugs.webkit.org/attachment.cgi?id=250290
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

> 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

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

> Source/WebCore/accessibility/AXObjectCache.cpp:893
> +        if (sanitizePasswordText(*obj, sanitizedText, position)) {

why do we have this special code for password fields?

> Source/WebCore/accessibility/AXObjectCache.cpp:906
> +    AccessibilityObject* obj = getOrCreate(node);

this line is common to both platforms and can be moved above

> Source/WebCore/accessibility/AXObjectCache.cpp:924
> +    if (obj) {

if there's no obj can we do an early return?

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

ditto about AXObject

> 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

> Source/WebCore/accessibility/AXObjectCache.h:192
> +    enum AXTextStateChangeType {

what other change types can you envision?

> 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

> 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

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

> Source/WebCore/accessibility/AXObjectCache.h:290
> +    Timer m_passwordNotificationPostTimer;

still curious why we need special timers for passwords

> 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

> 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

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

ditto about this star.

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

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

should this be final override

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.cpp:53
> +    case AXObjectCache::AXTextEditTypePaste:

can you just make the default: case be ASSERT_NOT_REACHED

-- 
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/d2c34d59/attachment-0001.html>