[webkit-reviews] review denied: [Bug 30883] [Gtk] Implement AtkText for HTML elements which contain text : [Attachment 45740] caret offset and event adjustments

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Joanmarie Diggs
<joanmarie.diggs at gmail.com>'s request for review:
Bug 30883: [Gtk] Implement AtkText for HTML elements which contain text
https://bugs.webkit.org/show_bug.cgi?id=30883

Attachment 45740: caret offset and event adjustments
https://bugs.webkit.org/attachment.cgi?id=45740&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
>From 7de96d0a93b5b44e0dc32bb141a4b52b890e15b1 Mon Sep 17 00:00:00 2001
>From: Joanmarie Diggs <jd at vblockhead.(none)>
>Date: Sat, 2 Jan 2010 11:14:37 -0500
>Subject: [PATCH 2/2] 2010-01-02  Joanmarie Diggs  <joanmarie.diggs at gmail.com>
>
>	 Reviewed by NOBODY (OOPS!).
>
>	 https://bugs.webkit.org/show_bug.cgi?id=30883
>	 [Gtk] Implement AtkText for HTML elements which contain text

We need some kind of explanation here of what the patch does...

>
>	 * accessibility/gtk/AccessibilityObjectWrapperAtk.h
>	 * accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:
>	 (objectAndOffsetUnignored):
>	 (webkit_accessible_text_get_caret_offset):
>	 * editing/gtk/SelectionControllerGtk.cpp:
>	 (SelectionController::notifyAccessibilityForSelectionChange)
>---


> static gint webkit_accessible_text_get_caret_offset(AtkText* text)
> {
>+    AccessibilityObject* coreObject = core(text);
>+    RenderObject* focusedNode =
coreObject->selection().end().node()->renderer();
>+    AccessibilityObject* focusedObject =
coreObject->document()->axObjectCache()->getOrCreate(focusedNode);
>+
>+    AccessibilityObject* object;
>+    int offset;
>+    objectAndOffsetUnignored(focusedObject, object, offset,
!coreObject->isLink());

Not sure I see a reason to not use the return value of the function for, say,
the 'realObject'?

Also, can you explain a bit the logic behind the last boolean parameter? In the
function below I see that it's used to decide whether to go one level up or not
when the object containing the text is a link, but I'm not sure I get why here
you pass !isLink(). If it's not a link you want to ignore them?


>+
>     // TODO: Verify this for RTL text.
>-    return core(text)->selection().end().offsetInContainerNode();
>+    return offset;
> }
> 
> static AtkAttributeSet* webkit_accessible_text_get_run_attributes(AtkText*
text, gint offset, gint* start_offset, gint* end_offset)
>@@ -1729,4 +1737,23 @@ AtkObject*
webkit_accessible_get_focused_element(WebKitAccessible* accessible)
>     return focusedObj->wrapper();
> }
> 
>+void objectAndOffsetUnignored(AccessibilityObject* coreObject,
AccessibilityObject*& realObject, int& offset, bool ignoreLinks)
>+{
>+
>+    Node* endNode =
static_cast<AccessibilityRenderObject*>(coreObject)->renderer()->node();
>+    int endOffset = coreObject->selection().end().offsetInContainerNode();
>+
>+    realObject = coreObject;
>+    if (realObject->accessibilityIsIgnored())
>+	  realObject = realObject->parentObjectUnignored();

This can only happen once? Any specific cases in mind?

>+
>+    if (ignoreLinks && realObject->isLink())
>+	  realObject = realObject->parentObjectUnignored();
>+
>+    RefPtr<Range> range =
rangeOfContents(static_cast<AccessibilityRenderObject*>(realObject)->renderer()
->node());
>+    ExceptionCode ec = 0;
>+    range->setEndBefore(endNode, ec);

We all know this will fail one day and we will say 'oh damn, should have
checked that value' :]

>+    offset = range->text().length() + endOffset;
>+}
>+
> #endif // HAVE(ACCESSIBILITY)

Marking r- to clarify my questions/doubts.