[Webkit-unassigned] [Bug 142719] AX: richer text change notifications
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 21 09:39:27 PDT 2015
https://bugs.webkit.org/show_bug.cgi?id=142719
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #251203|review? |
Flags| |
--- Comment #93 from Darin Adler <darin at apple.com> ---
Comment on attachment 251203
--> https://bugs.webkit.org/attachment.cgi?id=251203
Update from review
View in context: https://bugs.webkit.org/attachment.cgi?id=251203&action=review
I started reviewing but I didn’t get to the end yet.
Generally this patch is messing up the editing machinery with way too much explicit accessibility-specific code and should be done more elegantly if possible.
> Source/WebCore/ChangeLog:210
> +2015-04-17 Doug Russell <d_russell at apple.com>
> +
> + AX: richer text change notifications
> + https://bugs.webkit.org/show_bug.cgi?id=142719
> +
> + Richer accessibility selection change notifications. Introduce AXTextStateChangeIntent, and an overload of postTextReplacementNotification to give assistive tech apps more reliable context for responding to changes in web content selection. Also block posting selection changes on password fields.
> +
> +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.
> +
Need to remove these extra change log entries.
> Source/WebCore/accessibility/AXObjectCache.cpp:1319
> + if (!rootObject)
> + return nullptr;
> + if (rootObject->isAccessibilityScrollView())
> + return downcast<AccessibilityScrollView>(*rootObject).webAreaObject();
> + return nullptr;
Slightly nicer to stick 100% with the early return style so the real code is on the left, not nested. I’d write it like this:
if (!rootObject || !rootObject->isAccessibiltyScrollView())
return nullptr;
return downcast<AccessibilityScrollView>(*rootObject).webAreaObject();
> Source/WebCore/accessibility/AXObjectCache.h:199
> + // Simple text value change
I don’t think this comment adds much.
> Source/WebCore/accessibility/AXObjectCache.h:201
> + // Compound text value change
I don’t think this comment adds much.
> Source/WebCore/accessibility/AXObjectCache.h:203
> + // Selection change
I don’t think this comment adds much.
> Source/WebCore/accessibility/AXObjectCache.h:224
> +#if PLATFORM(COCOA) && !PLATFORM(IOS)
Should instead be:
#if PLATFORM(MAC)
> Source/WebCore/accessibility/AXObjectCache.h:257
> + AXTextChange textChangeForEditType(AXTextEditType type)
> + {
> + switch (type) {
> + case AXTextEditTypeCut:
> + case AXTextEditTypeDelete:
> + return AXTextDeleted;
> + case AXTextEditTypeInsert:
> + case AXTextEditTypeDictation:
> + case AXTextEditTypeTyping:
> + case AXTextEditTypePaste:
> + return AXTextInserted;
> + default:
> + break;
> + }
> + ASSERT_NOT_REACHED();
> + return AXTextInserted;
> + }
Why does this function have to be here in the header? I suggest declaring the function here, but defining it in the cpp file. Or we could define it later in this file. Putting the entire definition in the class makes the class unnecessarily hard to read. Also, this should be a static member function, not a member function. The "default: break;†code should be omitted. Including a default case causes the compiler to turn off its checking that the switch statement covers all enum values.
> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:563
> + return const_cast<AccessibilityNodeObject*>(this);
No need for this const_cast.
> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:567
> + if (!is<HTMLInputElement>(*element))
Need to remove the * here, in case node->shadowHost() is null. The is function will handle null, but only if passed a pointer rather than a reference.
> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:571
> + if (AXObjectCache* cache = axObjectCache())
> + return cache->getOrCreate(element);
Missing a null check for element here, I think.
> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:92
> +static NSUInteger const NSAccessibilityValueChangeTruncationLength = 1000;
We normally put the "const" before the type in declarations like this one.
It’s not appropriate to define something here with an NS prefix; that’s reserved for AppKit, and WebKit shouldn’t use that prefix for its own internal constants.
> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:111
> +void AXObjectCache::setShouldRepostNotificationsForTests(bool value)
Missing blank lines between functions here.
> Source/WebCore/editing/CompositeEditCommand.cpp:332
> +static void setCommandApplyEditTypeForEditingAction(SimpleEditCommand *command, EditAction editingAction)
Incorrect formatting of SimpleEditCommand* here.
> Source/WebCore/editing/CutDeleteSelectionCommand.h:43
> + virtual EditAction editingAction() const override;
Do we really need an entire new DeleteSelectionCommand subclass just to customize the editing action? Instead we should just have a new create function in DeleteSelectionCommand and have it store a flag to record the fact that it’s for a cut.
> Source/WebCore/editing/DeleteFromTextNodeCommand.h:29
> +#include "AXObjectCache.h"
Doesn’t make sense to add this include. Why is it needed?
> Source/WebCore/editing/DeleteFromTextNodeCommand.h:43
> + String& deletedText();
This seems wrong. We don’t want to expose the deleted text as a non-const reference. We don’t want to allow someone to change this underneath us.
> Source/WebCore/editing/DeleteFromTextNodeCommand.h:49
> +
Please don’t add this blank line here.
> Source/WebCore/editing/DictationInsertTextCommand.h:43
> + virtual EditAction editingAction() const override;
Same comment. Don’t create an entire new derived class just to customize the editing action. Just add a new create function and a data member instead.
> Source/WebCore/editing/EditCommand.h:29
> +#include "AXObjectCache.h"
Can we instead include only the header that defines the types, not the entire AXObjectCache.h header?
> Source/WebCore/editing/Editor.cpp:386
> + if (editingAction == EditActionCut)
> + applyCommand(CutDeleteSelectionCommand::create(document(), smartDelete));
> + else
> + applyCommand(DeleteSelectionCommand::create(document(), smartDelete));
Clearly we should just pass in the editingAction here instead of having two completely separate classes!
> Source/WebCore/editing/Editor.cpp:1008
> + changeSelectionAfterCommand(newSelection, options, AXTextStateChangeIntent(cmd->applyEditType()));
Should just be able to pass cmd->applyEditType() here. Should not need an explicit conversion to AXTextStateChangeIntent, because the constructor is not marked explicit.
> Source/WebCore/editing/Editor.cpp:1039
> + changeSelectionAfterCommand(newSelection, FrameSelection::defaultSetSelectionOptions(), AXTextStateChangeIntent(cmd->unapplyEditType()));
Should just be able to pass cmd->applyEditType() here. Should not need an explicit conversion to AXTextStateChangeIntent, because the constructor is not marked explicit.
> Source/WebCore/editing/Editor.h:177
> + WEBCORE_EXPORT void deleteSelectionWithSmartDelete(bool smartDelete, EditAction editingAction = EditActionDelete);
Should omit the argument name editingAction here.
> Source/WebCore/editing/EditorCommand.cpp:196
> + command->setApplyEditType(AXTextEditTypeInsert);
Should add this as a constructor argument, not require a separate set function after the fact for this.
> Source/WebCore/editing/FrameSelection.cpp:261
> + if (AXObjectCache::accessibilityEnabled()) {
> + if (newSelection.isCaret())
Should just use && here.
> Source/WebCore/editing/FrameSelection.cpp:266
> + else
> + intent = AXTextStateChangeIntent();
> + } else
> + intent = AXTextStateChangeIntent();
This code isn’t needed. The variable intent has already been initialized and there’s no need to set it again.
> Source/WebCore/editing/FrameSelection.cpp:1046
> + if (isRange()) {
> + if (directionOfSelection() == RTL)
> + flip = true;
> + }
Reads better as:
flip = isRange() && directionOfSelection() == RTL;
> Source/WebCore/editing/FrameSelection.h:136
> + static inline AXTextStateChangeIntent defaultSetSelectionIntent()
The inline keyword here is not needed and not helpful. Also seems overkill to write a function for this since the default is the default value for the class.
> Source/WebCore/editing/InsertIntoTextNodeCommand.h:29
> +#include "AXObjectCache.h"
Should not add this include.
> Source/WebCore/editing/InsertIntoTextNodeCommand.h:43
> + String& insertedText();
Same problem as above. No reason to expose a non-const reference to internal state.
--
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/20150421/be891e72/attachment-0001.html>
More information about the webkit-unassigned
mailing list