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

[bugzilla-daemon at webkit.org]

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

--- Comment #102 from Darin Adler <darin at apple.com> ---
Comment on attachment 251507
  --> https://bugs.webkit.org/attachment.cgi?id=251507
Update from review

View in context: https://bugs.webkit.org/attachment.cgi?id=251507&action=review

> Source/WebCore/ChangeLog:2238
> +2015-04-14  Doug Russell  <d_russell at apple.com>
> +
> +        AX: richer text change notifications
> +        https://bugs.webkit.org/show_bug.cgi?id=142719
> +
> +        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  <d_russell at apple.com>
> +
> +        AX: richer text change notifications
> +        https://bugs.webkit.org/show_bug.cgi?id=142719
> +
> +        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)
> +

Extra change log entry here due to merging problems I assume.

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

Use AXPostNotificationWithUserInfo.

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

Use AXPostNotificationWithUserInfo.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:386
> +#if PLATFORM(COCOA) && !PLATFORM(IOS)

#if PLATFORM(MAC)

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:400
> +    NSMutableArray *mutableArray = [array mutableCopy];

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.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:419
> +    NSMutableDictionary *mutableDictionary = [dictionary mutableCopy];

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.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.h:36
> +- (id)textMarkerRangeFromVisiblePositions:(WebCore::VisiblePosition)startPosition endPosition:(WebCore::VisiblePosition)endPosition;

Shouldn’t the arguments be const VisiblePosition& like in the method just below?

> Source/WebCore/editing/CompositeEditCommand.cpp:179
> +    default:
> +        break;

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.

> Source/WebCore/editing/CompositeEditCommand.h:36
> +class DeleteFromTextNodeCommand;

Why is this needed? I don’t see it used below.

> Source/WebCore/editing/CompositeEditCommand.h:63
> +    AXTextEditType unapplyEditType();

Should be a const member function.

> Source/WebCore/editing/DeleteFromTextNodeCommand.cpp:53
> +const String& DeleteFromTextNodeCommand::deletedText()
> +{
> +    return m_text;
> +}

Should probably be inlined in the header. Just add "inline" and put it at the bottom of the header.

> Source/WebCore/editing/EditCommand.cpp:133
> +    default:
> +        break;

Same comment about default as above.

> Source/WebCore/editing/FrameSelection.cpp:1035
> +AXTextStateChangeIntent inline FrameSelection::TextSelectionIntent(EAlteration alter, SelectionDirection direction, TextGranularity granularity)

Why inline? That doesn’t seem like a good idea. Also, we normally put inline to the left of the type.

> Source/WebCore/editing/InsertIntoTextNodeCommand.cpp:56
> +const String& InsertIntoTextNodeCommand::insertedText()
> +{
> +    return m_text;
> +}

Inline in header?

> Source/WebCore/editing/InsertIntoTextNodeCommand.h:44
> +    const String& insertedText();
> +protected:
> +    InsertIntoTextNodeCommand(PassRefPtr<Text> node, unsigned offset, const String& text, EditAction editingAction);

Missing blank line before protected. Also, should just make this private again. I believe the reason to make it protected is obsolete.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:33
> +class Text;

Not needed.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:35
> +class ReplaceDeleteFromTextNodeCommand : public DeleteFromTextNodeCommand {

Should mark this class final.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:37
> +    static Ref<ReplaceDeleteFromTextNodeCommand> create(PassRefPtr<Text> node, unsigned offset, unsigned count)

New code should not use PassRefPtr. See <https://www.webkit.org/coding/RefPtr.html>.

> Source/WebCore/editing/ReplaceDeleteFromTextNodeCommand.h:43
> +    ReplaceDeleteFromTextNodeCommand(PassRefPtr<Text>, unsigned, unsigned);

New code should not use PassRefPtr. See <https://www.webkit.org/coding/RefPtr.html>.

This should be private, not protected.

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:33
> +class Text;

Not needed.

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:35
> +class ReplaceInsertIntoTextNodeCommand : public InsertIntoTextNodeCommand {

Should mark this class final.

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:37
> +    static Ref<ReplaceInsertIntoTextNodeCommand> create(PassRefPtr<Text> node, unsigned offset, const String& text, const String& deletedText, EditAction editingAction)

New code should not use PassRefPtr. See <https://www.webkit.org/coding/RefPtr.html>.

> Source/WebCore/editing/ReplaceInsertIntoTextNodeCommand.h:43
> +    ReplaceInsertIntoTextNodeCommand(PassRefPtr<Text>, unsigned, const String&, const String&, EditAction);

New code should not use PassRefPtr. See <https://www.webkit.org/coding/RefPtr.html>.

This should be private, not protected.

> Source/WebCore/editing/ReplaceSelectionCommand.h:124
> +    AXTextEditType m_applyEditType;

This looks uninitialized and unused. Please omit it.

-- 
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/20150424/2c7c7f9d/attachment-0001.html>