[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

--- Comment #30 from Doug Russell <d_russell at apple.com> ---
(In reply to comment #29)
> Comment on attachment 269085 [details]
> 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

I sent the patch to Brent Fulgham hoping for some help on this, but haven't heard back yet.

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

There's not a known bug related to this, but I noticed that in it's previous location intent.selection.focusChange wasn't guaranteed to be initialized so I moved it up. I can submit it in it's own patch if you'd like me to.

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

Will do.

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

Will do.

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

I'll remove them.

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

I'll add one.

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

Will do.

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

We could thread error state through the modifyMovingX logic to return why the VisiblePosition it's returning is NULL. It would mean touching a lot more code though.

-- 
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/20160125/5524322a/attachment-0001.html>