[Webkit-unassigned] [Bug 153085] AX: Add a boundary value to AXTextStateChangeType

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #269085|review?                     |review-
              Flags|                            |

--- Comment #29 from Darin Adler <darin at apple.com> ---
Comment on attachment 269085
  --> https://bugs.webkit.org/attachment.cgi?id=269085
Patch

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

Generally looks good. I am concerned about this new VisiblePosition feature. Maybe this is the best way to do it, though. Did you consult with other WebKit editing code experts on the best way?

Also need some help from a Windows expert on figuring out why Windows bot failed to build.

review- because we at least want to make the "explicit" change and to get Windows building

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:385
> +            if (intent.selection.focusChange)
> +                [userInfo setObject:@(intent.selection.focusChange) forKey:NSAccessibilityTextSelectionChangedFocus];

Does moving this here fix a bug? If so, do is that fix covered by a test case?

> Source/WebCore/editing/FrameSelection.cpp:1110
> +        // FIXME: BIDI

Would be nice to write a more specific FIXME. I think I know what you are driving at, but explicitly stating it is nicer for other programmers who show up later.

> Source/WebCore/editing/FrameSelection.cpp:1117
> +        // FIXME: BIDI

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:1130
> +    case SentenceBoundary: // FIXME: Boundary should effect direction

Grammar error. We mean "should affect" not "should effect". Also should put a period; we use a sort of sentence style on these comments and call for a period.

> Source/WebCore/editing/FrameSelection.cpp:1137
> +    case ParagraphBoundary: // FIXME: Boundary should effect direction

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:1141
> +    case DocumentBoundary: // FIXME: Boundary should effect direction

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:1224
> +        notifyAccessibilityForSelectionChange((AXTextStateChangeIntent) { AXTextStateChangeTypeSelectionBoundary, textSelectionWithDirectionAndGranularity(direction, granularity) });

Shouldn’t need parentheses around (AXTextStateChangeIntent). Maybe you did that to quiet the mistaken complaint from the style checker?

> Source/WebCore/editing/VisiblePosition.h:53
> +    static VisiblePosition boundaryVisiblePosition();

I’m unclear on how this is intended to be used. We need a comment explaining this since it’s not an obvious concept.

> Source/WebCore/editing/VisiblePosition.h:116
> +    VisiblePosition(bool boundary) : m_affinity(VP_DEFAULT_AFFINITY), m_isBoundary(boundary) { };

Should qualify this with "explicit" so code inside VisiblePosition can’t make the mistake of returning a boolean and have it automatically turned into a VisiblePosition.

> Source/WebCore/editing/VisiblePosition.h:125
>      Position m_deepPosition;
>      EAffinity m_affinity;
> +    bool m_isBoundary = false;

I think it’s dangerous that now we have a type of VisiblePosition that won’t survive a round trop if broken into position and affinity and then made into a VIsiblePosition again. Do we have other options here rather than adding this state to VisiblePosition?

-- 
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/20160124/795a58ca/attachment.html>