<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: [Mac] Implement next/previous text marker functions using TextIterator"
href="https://bugs.webkit.org/show_bug.cgi?id=152728#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: [Mac] Implement next/previous text marker functions using TextIterator"
href="https://bugs.webkit.org/show_bug.cgi?id=152728">bug 152728</a>
from <span class="vcard"><a class="email" href="mailto:cfleizach@apple.com" title="chris fleizach <cfleizach@apple.com>"> <span class="fn">chris fleizach</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=268269&action=diff" name="attach_268269" title="patch">attachment 268269</a> <a href="attachment.cgi?id=268269&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=268269&action=review">https://bugs.webkit.org/attachment.cgi?id=268269&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:8
> + Implemented a way to navigate to previous/next text marker using Range and TextIterator.</span >
can you add a few sentences as to why you're doing this
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1431
> + Node* currNode = nullptr;</span >
currNode -> currentNode
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1448
> + if (preNode)</span >
if (Node* preNode = previousNode(currNode))
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1457
> + int currLen = iterator.text().length();</span >
currLen = currentLength
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1469
> + </span >
extra newline can be removed
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1509
> + RefPtr<Range> range = Range::create(*document);</span >
too many spaces
"= Range"
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1512
> + if (ec)</span >
return ec ? nullptr : range;
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1626
> + if (node->parentNode() && node->parentNode()->renderer() && node->parentNode()->renderer()->isBody() && !node->previousSibling())</span >
too many spaces between the &&
"node->parentNode()->renderer() && node->parentNode()->renderer()->isBody()"
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.h:65
> + CharacterOffset(Node* n, int offset, int remaining)</span >
you might be able to put default values for these parameters and then remove the default constructor
CharacterOffset() { }
-->
CharacterOffset(Node* n = nullptr, int offset = 0, int remaining = 0)
<span class="quote">> Source/WebCore/accessibility/AccessibilityObject.cpp:724
> + return AXObjectCache::rangeForNodeContents(node);</span >
seems like you could probably just do
return AXObjectCache::rangeForNodeContents(node());
and then handle the nil case inside rangeForNodeContents
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:790
> +static AccessibilityObject* accessibilityObjectForTextMarker(AXObjectCache* cache, CFTypeRef textMarker)</span >
these methods might be better off inside WebAXObjectWrapperBase so that they can be used by iOS and OSX
<span class="quote">> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:830
> +- (id)textMarkerForNode:(Node&)node Offset:(int)offset</span >
Offset -> offset
<span class="quote">> LayoutTests/ChangeLog:7
> +</span >
we should add a test for navigation within a user-select:none area</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>