<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #269085 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<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#c29">Comment # 29</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:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>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 class="quote">> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:385
> + if (intent.selection.focusChange)
> + [userInfo setObject:@(intent.selection.focusChange) forKey:NSAccessibilityTextSelectionChangedFocus];</span >
Does moving this here fix a bug? If so, do is that fix covered by a test case?
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:1110
> + // FIXME: BIDI</span >
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 class="quote">> Source/WebCore/editing/FrameSelection.cpp:1117
> + // FIXME: BIDI</span >
Ditto.
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:1130
> + case SentenceBoundary: // FIXME: Boundary should effect direction</span >
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 class="quote">> Source/WebCore/editing/FrameSelection.cpp:1137
> + case ParagraphBoundary: // FIXME: Boundary should effect direction</span >
Ditto.
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:1141
> + case DocumentBoundary: // FIXME: Boundary should effect direction</span >
Ditto.
<span class="quote">> Source/WebCore/editing/FrameSelection.cpp:1224
> + notifyAccessibilityForSelectionChange((AXTextStateChangeIntent) { AXTextStateChangeTypeSelectionBoundary, textSelectionWithDirectionAndGranularity(direction, granularity) });</span >
Shouldn’t need parentheses around (AXTextStateChangeIntent). Maybe you did that to quiet the mistaken complaint from the style checker?
<span class="quote">> Source/WebCore/editing/VisiblePosition.h:53
> + static VisiblePosition boundaryVisiblePosition();</span >
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 class="quote">> Source/WebCore/editing/VisiblePosition.h:116
> + VisiblePosition(bool boundary) : m_affinity(VP_DEFAULT_AFFINITY), m_isBoundary(boundary) { };</span >
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 class="quote">> Source/WebCore/editing/VisiblePosition.h:125
> Position m_deepPosition;
> EAffinity m_affinity;
> + bool m_isBoundary = false;</span >
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?</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>