<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: Add a boundary value to AXTextStateChangeType"
href="https://bugs.webkit.org/show_bug.cgi?id=153085#c30">Comment # 30</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: Add a boundary value to AXTextStateChangeType"
href="https://bugs.webkit.org/show_bug.cgi?id=153085">bug 153085</a>
from <span class="vcard"><a class="email" href="mailto:d_russell@apple.com" title="Doug Russell <d_russell@apple.com>"> <span class="fn">Doug Russell</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=153085#c29">comment #29</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=269085&action=diff" name="attach_269085" title="Patch">attachment 269085</a> <a href="attachment.cgi?id=269085&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=269085&action=review">https://bugs.webkit.org/attachment.cgi?id=269085&action=review</a>
>
> 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</span >
I sent the patch to Brent Fulgham hoping for some help on this, but haven't heard back yet.
<span class="quote">>
> > 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?</span >
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.
<span class="quote">>
> > 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.</span >
Will do.
<span class="quote">>
> > 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.</span >
Will do.
<span class="quote">>
> > 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?</span >
I'll remove them.
<span class="quote">>
> > 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.</span >
I'll add one.
<span class="quote">>
> > 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.</span >
Will do.
<span class="quote">>
> > 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?</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>