Bug 25528 – [Gtk] Text attributes not exposed

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

Steps to reproduce:

1. Open the (to-be) attached test case.

2. In Accerciser, in the pane on the left, select any object from the page content for which the accessible text interface has been implemented. 

3. In Accerciser, in the pane on the right, choose the Interface Viewer from the notebook. Expand Text (Editable) to see the text, caret offset, the non-default text attributes associated at that offset, and the start and end offset for those attributes. 

4. Check the "Include defaults" checkbox to see the text attributes associated with the entire text object.

Expected results: The above attribute information and offsets would be exposed.

Actual results: None of the above attribute information or offsets are exposed.

Ideally, the attributes (and attribute names) exposed to assistive technologies would be taken from this list so as to be more consistent with existing Gtk+ apps:

http://library.gnome.org/devel/atk/unstable/AtkText.html#AtkTextAttribute

[Joanmarie Diggs]

Created attachment 29967 [details]
aforementioned test case

[José Millán Soto]

Created attachment 46683 [details]
Patch that implements webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes

This patch implements the following attributes:
  ATK_TEXT_ATTR_INVISIBLE,
  ATK_TEXT_ATTR_EDITABLE,
  ATK_TEXT_ATTR_RISE,
  ATK_TEXT_ATTR_UNDERLINE,
  ATK_TEXT_ATTR_STRIKETHROUGH,
  ATK_TEXT_ATTR_SIZE,
  ATK_TEXT_ATTR_WEIGHT,
  ATK_TEXT_ATTR_FAMILY_NAME,
  ATK_TEXT_ATTR_BG_COLOR,
  ATK_TEXT_ATTR_FG_COLOR,
  ATK_TEXT_ATTR_JUSTIFICATION and
  ATK_TEXT_ATTR_STYLE

[WebKit Review Bot]

Attachment 46683 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1271:  webkit_accessible_text_get_run_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1

[Joanmarie Diggs]

José, thanks for taking this on!

Having done some quick testing on it, it looks good. One minor thing I noticed with the following two lines from the test case:

* This sentence has the words sentence in superscript.
* This sentence has the word sentence in subscript.

The first instance of <sup></sup> and <sub></sub> each report a rise and size, but the second one does not.

[José Millán Soto]

> Having done some quick testing on it, it looks good. One minor thing I noticed
> with the following two lines from the test case:
> 
> * This sentence has the words sentence in superscript.
> * This sentence has the word sentence in subscript.
> 
> The first instance of <sup></sup> and <sub></sub> each report a rise and size,
> but the second one does not.

I tested and both instances of <sup> and <sub> work fine to me.

[Joanmarie Diggs]

(In reply to comment #5)
> > Having done some quick testing on it, it looks good. One minor thing I noticed
> > with the following two lines from the test case:
> > 
> > * This sentence has the words sentence in superscript.
> > * This sentence has the word sentence in subscript.
> > 
> > The first instance of <sup></sup> and <sub></sub> each report a rise and size,
> > but the second one does not.
> 
> I tested and both instances of <sup> and <sub> work fine to me.

Hmmm. Are you testing this with Accerciser? 

Anyhoo, I'll try again....

[Xan Lopez]

Comment on attachment 46683 [details]
Patch that implements webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes

>-static AtkAttributeSet* webkit_accessible_text_get_run_attributes(AtkText* text, gint offset, gint* start_offset, gint* end_offset)
>+static void addAttribute(AtkAttributeSet* &attributeSet, AtkTextAttribute textAttribute, gchar* value, bool copyValue = true)

So are pointers to references more idiomatic than pointers to pointers? I don't think I've seen them used a lot (well, at all) in WebKit.

> {
>-    notImplemented();
>-    return 0;
>+    AtkAttribute* newAttribute = (AtkAttribute *)g_malloc(sizeof(AtkAttribute));

In theory the preferred method to allocate structures in glib is with g_slice_new. Also, C-style casts are against the style guidelines.

>+    newAttribute->name = g_strdup(atk_text_attribute_get_name(textAttribute));
>+    if (copyValue) value = g_strdup(value);

The assignment has to go in its own line.

>+    newAttribute->value = value;

Or you could just do... = copyValue? g_strdup(value) : value;

>+    attributeSet = g_slist_prepend(attributeSet, newAttribute);
>+}
>+
>+/*
>+    Returns an AtkAttributeSet with the AtkAttributes which define style.
>+    If parentObject is given, only the ones which are different from style and
>+    parentStyle would be in the result.
>+*/

I think C++-style comments are preferred. Also, the comment seems to refer to a parameter that no longer exists?

>+static AtkAttributeSet* getAttributeSetForAccessibilityObject(const AccessibilityObject* object, bool onlyDifferentFromParent = false)
>+{
>+    RenderObject* renderer;
>+    RenderObject* parentRenderer = 0;
>+    AtkAttributeSet* result = 0;
>+    RenderStyle* style;
>+    RenderStyle* parentStyle = 0;
>+    int intProp;
>+    bool styleProp;
>+
>+    if (object->isAccessibilityRenderObject()) {
>+        AccessibilityObject* parent;
>+        renderer = ((AccessibilityRenderObject*)object)->renderer();

Use C++ casts.

>+        style = renderer->style();
>+        if (onlyDifferentFromParent) {
>+            parent = object->parentObject();
>+            while (parent && !parent->isAccessibilityRenderObject())
>+                parent = parent->parentObject();

I believe you should check here if the parent objects you iterate through are not ignored (see parentObjectUnignored).

>+            if (parent && parent->isAccessibilityRenderObject())
>+                parentRenderer = ((AccessibilityRenderObject*)parent)->renderer();

Casting again.

>+        }
>+    } else
>+        return 0;

Just return early at the beginning of the function.

>+
>+    if (parentRenderer)
>+        parentStyle = parentRenderer->style();
>+
>+    intProp = style->fontSize();
>+    if (!parentStyle || (intProp != parentStyle->fontSize()))
>+        addAttribute(result, ATK_TEXT_ATTR_SIZE, g_strdup_printf("%i", intProp), false);
>+
>+    if (style->hasBackground()
>+        && !(parentStyle
>+             && parentStyle->hasBackground()
>+             && (style->backgroundColor() == parentStyle->backgroundColor()))
>+        && style->color().isValid())
>+        addAttribute(result, ATK_TEXT_ATTR_BG_COLOR, g_strdup_printf("%i,%i,%i", style->backgroundColor().red(), style->backgroundColor().green(), style->backgroundColor().blue()), false);
>+    if (style->color().isValid()
>+        && !(parentStyle && (style->color() == parentStyle->color())))
>+        addAttribute(result, ATK_TEXT_ATTR_FG_COLOR, g_strdup_printf("%i,%i,%i", style->color().red(), style->color().green(), style->color().blue()), false);
>+
>+    intProp = renderer->baselinePosition(true);
>+    if (!parentStyle
>+        || (parentStyle->verticalAlign() != style->verticalAlign())
>+        || (style->verticalAlign() == BASELINE ? false : (intProp != parentRenderer->baselinePosition(true)))) {
>+        bool includeRise;
>+        switch (style->verticalAlign()) {
>+        case SUB:
>+            intProp = -intProp;
>+        case SUPER:
>+            includeRise = true;
>+            break;
>+        case BASELINE:
>+            includeRise = true;
>+            intProp = 0;
>+            break;
>+        default:
>+            includeRise = false;
>+        }
>+        if (includeRise)
>+            addAttribute(result, ATK_TEXT_ATTR_RISE, g_strdup_printf("%i", intProp), false);
>+    }
>+
>+    if (!parentStyle
>+        || (parentStyle->textIndent() != style->textIndent())) {
>+        gint value = style->textIndent().calcValue(object->size().width());
>+        if (value != undefinedLength)
>+            addAttribute(result, ATK_TEXT_ATTR_INDENT, g_strdup_printf("%i", value), false);
>+    }
>+
>+    if (!parentStyle || (parentStyle->font().family().family() != style->font().family().family()))
>+        addAttribute(result, ATK_TEXT_ATTR_FAMILY_NAME, g_strdup(style->font().family().family().string().utf8().data()), false);
>+
>+    intProp = style->font().weight();
>+    if (!parentStyle || (intProp != parentStyle->font().weight())) {
>+        int fontWeight = -1;
>+        switch (intProp) {
>+        case FontWeight100:
>+            fontWeight = 100;
>+            break;
>+        case FontWeight200:
>+            fontWeight = 200;
>+            break;
>+        case FontWeight300:
>+            fontWeight = 300;
>+            break;
>+        case FontWeight400:
>+            fontWeight = 400;
>+            break;
>+        case FontWeight500:
>+            fontWeight = 500;
>+            break;
>+        case FontWeight600:
>+            fontWeight = 600;
>+            break;
>+        case FontWeight700:
>+            fontWeight = 700;
>+            break;
>+        case FontWeight800:
>+            fontWeight = 800;
>+            break;
>+        case FontWeight900:
>+            fontWeight = 900;
>+        }
>+        if (fontWeight > 0)
>+            addAttribute(result, ATK_TEXT_ATTR_WEIGHT, g_strdup_printf("%i", fontWeight), false);
>+    }
>+
>+    intProp = style->textAlign();
>+    if (!parentStyle || (intProp != parentStyle->textAlign())) {
>+        switch (intProp) {
>+        case TAAUTO:
>+            break;
>+        case LEFT:
>+        case WEBKIT_LEFT:
>+            addAttribute(result, ATK_TEXT_ATTR_JUSTIFICATION, (gchar *)"left");
>+            break;
>+        case RIGHT:
>+        case WEBKIT_RIGHT:
>+            addAttribute(result, ATK_TEXT_ATTR_JUSTIFICATION, (gchar *)"right");
>+            break;
>+        case CENTER:
>+        case WEBKIT_CENTER:
>+            addAttribute(result, ATK_TEXT_ATTR_JUSTIFICATION, (gchar *)"center");
>+            break;
>+        case JUSTIFY:
>+            addAttribute(result, ATK_TEXT_ATTR_JUSTIFICATION, (gchar *)"fill");
>+        }
>+    }
>+
>+    styleProp = style->textDecoration() & UNDERLINE;
>+    if (!parentStyle || (styleProp != (parentStyle->textDecoration() & UNDERLINE)))
>+        addAttribute(result, ATK_TEXT_ATTR_UNDERLINE, (styleProp ? (gchar*)"single" : (gchar*)"none"));
>+    styleProp = style->font().italic();
>+    if (!parentStyle || (styleProp != (parentStyle->font().italic())))
>+        addAttribute(result, ATK_TEXT_ATTR_STYLE, (styleProp ? (gchar*)"italic" : (gchar*)"normal"));
>+    styleProp = style->textDecoration() & LINE_THROUGH;
>+    if (!parentStyle || (styleProp != (parentStyle->textDecoration() & LINE_THROUGH)))
>+        addAttribute(result, ATK_TEXT_ATTR_STRIKETHROUGH, (styleProp ? (gchar*)"true" : (gchar*)"false"));
>+    styleProp = style->visibility() == HIDDEN;
>+    if (!parentStyle || (styleProp != (parentStyle->visibility() == HIDDEN)))
>+        addAttribute(result, ATK_TEXT_ATTR_INVISIBLE, (styleProp ? (gchar*)"true" : (gchar*)"false"));
>+
>+    return result;
>+}

I honestly feel that having the logic of getting the difference from the parent object in the same function makes the code less clear than it could be. I think it would work better if you had a (simple) function to get the attributes of one object, another to get the difference between two attribute sets, and possibly a third helper function to get the difference between one object's and its parent set. It will probably be more code, but I think in the end things will be more understandable.

>+
>+static gint compareAttributeName(const AtkAttribute* a, const AtkAttribute* b)
>+{
>+    return g_strcmp0(a->name, b->name);
>+}
>+
>+/*
>+    Returns the union of two AtkAttributeSet, having the second one priority.
>+    Releases a1, and the AtkAttributes no longer used

The memory management of the function seems a bit weird and tailored for you one use case. Making a new set with the union seems more reasonable if you think this could be used elsewhere in the future.

Another thing, possibly it would be more efficient to sort the attributes by name and then just iterate both lists at the same time.

>+*/
>+static AtkAttributeSet* attributeSetUnion(AtkAttributeSet* a1, AtkAttributeSet* a2)
>+{
>+    if (!a1)
>+        return a2;
>+    if (!a2)
>+        return a1;
>+
>+    AtkAttributeSet* i = a1;
>+    AtkAttributeSet* found;
>+
>+    do {
>+        found = g_slist_find_custom(a2, i->data, (GCompareFunc)compareAttributeName);
>+        if (!found) {
>+            AtkAttributeSet* t = i->next;
>+            a2 = g_slist_prepend(a2, i->data);
>+            a1 = g_slist_delete_link(a1, i);
>+            i = t;
>+        } else
>+            i = i->next;
>+    } while (i);
>+
>+    atk_attribute_set_free(a1);
>+    return a2;
>+}
>+
>+static guint accessibilityObjectLength(const AccessibilityObject* object)
>+{
>+    int value = object->stringValue().length();
>+    if (value)
>+        return value;
>+    if (object->isAccessibilityRenderObject()) {
>+        const RenderObject* renderer = ((AccessibilityRenderObject*)object)->renderer();
>+        if (renderer->isBR())
>+            return 1;
>+    }
>+    return object->textUnderElement().length();
>+}

use webkit_accessible_get_text and then calculate the length?

>+
>+/*
>+    Returns a list of AccessibilityObjects, so that each one is the
>+    child of the preceding one which is located in the desired offset.
>+    *startOffset and *endOffset would be the beggining and next offset of the
>+    most lower AccessibilityObject; if offset exceeds the length of the object
>+    *startOffset and *endOffset will be set to -1.
>+*/
>+static GSList *getAccessibilityObjectsForOffset(const AccessibilityObject *object, guint offset, gint *start_offset, gint *end_offset)
>+{
>+    GSList* result = 0;
>+    guint length = accessibilityObjectLength(object);
>+    if (length > offset) {
>+        *start_offset = 0;
>+        *end_offset = length;
>+        result = g_slist_append(result, (gpointer)object);
>+    } else {
>+        *start_offset = -1;
>+        *end_offset = -1;
>+    }
>+
>+    if (object->hasChildren()) {
>+        AccessibilityObject* child = object->firstChild();
>+        guint childPosition = 0;
>+        guint childLength;
>+        while (child) {
>+            childLength = accessibilityObjectLength(child);
>+            if ((childLength + childPosition) > offset) {
>+                gint childStartOffset;
>+                gint childEndOffset;
>+                result = g_slist_concat(getAccessibilityObjectsForOffset(child, offset-childPosition, &childStartOffset, &childEndOffset), result);
>+                if (childStartOffset >= 0) {
>+                    *start_offset = childStartOffset + childPosition;
>+                    *end_offset = childEndOffset + childPosition;
>+                }
>+                break;
>+            }
>+            childPosition += childLength;
>+            child = child->nextSibling();
>+        }
>+    }
>+    return result;
>+}
>+
>+static AtkAttributeSet* getRunAttributesFromAccesibilityObject(const AccessibilityObject* element, gint offset, gint* startOffset, gint* endOffset, RenderObject* defaultRenderer)
>+{
>+    AtkAttributeSet* result = 0;
>+
>+    GSList* childs = getAccessibilityObjectsForOffset(element, offset, startOffset, endOffset);

children.

>+    GSList* child = childs;
>+    bool previousReadOnly = element->isReadOnly();
>+
>+    while (child) {
>+        const AccessibilityObject* childObject = (const AccessibilityObject *)child->data;
>+        bool readOnly = childObject->isReadOnly();
>+        AtkAttributeSet* differentAttributes = getAttributeSetForAccessibilityObject(childObject, true);
>+        if (readOnly != previousReadOnly) {
>+            previousReadOnly = readOnly;
>+            addAttribute(differentAttributes, ATK_TEXT_ATTR_EDITABLE, readOnly ? (gchar *)"false" : (gchar *)"true");
>+        }
>+        result = attributeSetUnion(result, differentAttributes);
>+        child = child->next;
>+    }
>+    g_slist_free(childs);
>+
>+    return result;
>+}
>+
>+static AtkAttributeSet* webkit_accessible_text_get_run_attributes(AtkText* text, gint offset, gint* startOffset, gint* endOffset)
>+{
>+    AccessibilityObject* coreObject = core(text);
>+    AtkAttributeSet* result;
>+    int rStartOffset, rEndOffset;
>+
>+    if ((!coreObject) || (!coreObject->hasChildren()))
>+        return 0;
>+
>+    RenderObject* baseRenderer = 0;
>+    if (coreObject->isAccessibilityRenderObject())
>+        baseRenderer = ((AccessibilityRenderObject*)coreObject)->renderer();

This seems to be completely unused in getRunAttributesFromAccessibilityObject.

>+
>+    if (offset == -1)
>+        offset = atk_text_get_caret_offset(text);
>+
>+    result = getRunAttributesFromAccesibilityObject(coreObject, offset, &rStartOffset, &rEndOffset, baseRenderer);
>+
>+    if (rStartOffset >= 0) {
>+        *startOffset = rStartOffset;
>+        *endOffset = rEndOffset;
>+    } else {
>+        *startOffset = offset;
>+        *endOffset = offset;
>+    }
>+
>+    return result;
> }
> 
> static AtkAttributeSet* webkit_accessible_text_get_default_attributes(AtkText* text)
> {
>-    notImplemented();
>-    return 0;
>+    AccessibilityObject* coreObject = core(text);
>+    if ((!coreObject) || (!coreObject->isAccessibilityRenderObject()))
>+        return 0;

Extra parens not needed.

>+
>+    AtkAttributeSet* result = 0;
>+
>+    result = getAttributeSetForAccessibilityObject(coreObject);
>+    addAttribute(result, ATK_TEXT_ATTR_EDITABLE, coreObject->isReadOnly() ? (gchar*)"false" : (gchar*)"true");

Why is that not handled by the function? Are the casts to (gchar*) really needed? Couldn't the function take 'const char*'?

>+    return result;
> }
> 
> static void webkit_accessible_text_get_character_extents(AtkText* text, gint offset, gint* x, gint* y, gint* width, gint* height, AtkCoordType coords)


As a last comment, can we do first a patch just implementing webkit_accessible_text_get_default_attributes, seems it would get rid of the most complex functions needed for get_run_attributes, which we can add in a later patch.

Also, as always, it would be good to have some test for this.

[José Millán Soto]

Created attachment 49407 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

New version of the patch, which solves most of the issues appointed in comment 7.
It includes a new test in testatk.c also.

[WebKit Review Bot]

Attachment 49407 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1238:  webkit_accessible_text_get_run_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Ignoring "WebKit/gtk/tests/testatk.c": this file is exempt from the style guide.
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Comment on attachment 49407 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

> Index: WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp
> ===================================================================
> --- WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp	(revision 55194)
> +++ WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp	(working copy)
> @@ -1012,16 +1012,263 @@ static gint webkit_accessible_text_get_c
>      return offset;
>  }
>  
> -static AtkAttributeSet* webkit_accessible_text_get_run_attributes(AtkText* text, gint offset, gint* start_offset, gint* end_offset)
> +static AtkAttributeSet* getAttributeSetForAccessibilityObject(const AccessibilityObject* object)
>  {
> -    notImplemented();
> -    return 0;
> +    RenderObject* renderer;
> +    AtkAttributeSet* result = 0;
> +    RenderStyle* style;
> +    const int bufferSize = 16;
> +    gchar buffer[bufferSize];
> +
> +    if (!object->isAccessibilityRenderObject())
> +        return 0;
> +
> +    renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();
> +    style = renderer->style();
> +
> +    g_snprintf(buffer, bufferSize, "%i", style->fontSize());
> +    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_SIZE), buffer);
> +
> +    if (style->hasBackground()
> +        && style->backgroundColor().isValid()) {
> +        g_snprintf(buffer, bufferSize, "%i,%i,%i", style->backgroundColor().red(), style->backgroundColor().green(), style->backgroundColor().blue());
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_BG_COLOR), buffer);
> +    }
> +    if (style->color().isValid()) {
> +        g_snprintf(buffer, bufferSize, "%i,%i,%i", style->color().red(), style->color().green(), style->color().blue());
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_FG_COLOR), buffer);
> +    }
> +
> +    int baselinePosition = renderer->baselinePosition(true);
> +    bool includeRise;
> +    switch (style->verticalAlign()) {
> +    case SUB:
> +        baselinePosition = -baselinePosition;
> +    case SUPER:
> +        includeRise = true;
> +        break;
> +    case BASELINE:
> +        includeRise = true;
> +        baselinePosition = 0;
> +        break;
> +    default:
> +        includeRise = false;
> +    }
> +    if (includeRise) {
> +        g_snprintf(buffer, bufferSize, "%i", baselinePosition);
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_RISE), buffer);
> +    }
> +
> +    int indentation = style->textIndent().calcValue(object->size().width());
> +    if (indentation != undefinedLength) {
> +        g_snprintf(buffer, bufferSize, "%i", indentation);
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_INDENT), buffer);
> +    }
> +
> +    String fontFamilyName = style->font().family().family().string();
> +    if (fontFamilyName.left(8) == "-webkit-")
> +        fontFamilyName = fontFamilyName.substring(8);
> +
> +    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_FAMILY_NAME), fontFamilyName.utf8().data());
> +
> +    int fontWeight = -1;
> +    switch (style->font().weight()) {
> +    case FontWeight100:
> +        fontWeight = 100;
> +        break;
> +    case FontWeight200:
> +        fontWeight = 200;
> +        break;
> +    case FontWeight300:
> +        fontWeight = 300;
> +        break;
> +    case FontWeight400:
> +        fontWeight = 400;
> +        break;
> +    case FontWeight500:
> +        fontWeight = 500;
> +        break;
> +    case FontWeight600:
> +        fontWeight = 600;
> +        break;
> +    case FontWeight700:
> +        fontWeight = 700;
> +        break;
> +    case FontWeight800:
> +        fontWeight = 800;
> +        break;
> +    case FontWeight900:
> +        fontWeight = 900;
> +    }
> +    if (fontWeight > 0) {
> +        g_snprintf(buffer, bufferSize, "%i", fontWeight);
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_WEIGHT), buffer);
> +    }
> +
> +    switch (style->textAlign()) {
> +    case TAAUTO:
> +        break;
> +    case LEFT:
> +    case WEBKIT_LEFT:
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_JUSTIFICATION), "left");
> +        break;
> +    case RIGHT:
> +    case WEBKIT_RIGHT:
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_JUSTIFICATION), "right");
> +        break;
> +    case CENTER:
> +    case WEBKIT_CENTER:
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_JUSTIFICATION), "center");
> +        break;
> +    case JUSTIFY:
> +        result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_JUSTIFICATION), "fill");
> +    }
> +
> +    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_UNDERLINE), ((style->textDecoration() & UNDERLINE) ? "single" : "none"));
> +
> +    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_STYLE), (style->font().italic() ? "italic" : "normal"));
> +
> +    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_STRIKETHROUGH), ((style->textDecoration() & LINE_THROUGH) ? "true" : "false"));
> +
> +    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_INVISIBLE), ((style->visibility() == HIDDEN) ? "true" : "false"));
> +
> +    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_EDITABLE), object->isReadOnly() ? "false" : "true");
> +
> +    return result;
> +}
> +
> +static gint compareAttribute(const AtkAttribute* a, const AtkAttribute* b)
> +{
> +    return (g_strcmp0(a->name, b->name) || g_strcmp0(a->value, b->value));
> +}
> +
> +//  Returns an AtkAttributeSet with the elements of a1 which are either not present or different in a2.
> +//  Neither a1 nor a2 should be used after calling this function.
> +static AtkAttributeSet* attributeSetDifference(AtkAttributeSet* a1, AtkAttributeSet* a2)
> +{
> +    if (!a2)
> +        return a1;
> +
> +    AtkAttributeSet* i = a1;
> +    AtkAttributeSet* found;
> +    AtkAttributeSet* toDelete = 0;
> +
> +    while (i) {
> +        found = g_slist_find_custom(a2, i->data, (GCompareFunc)compareAttribute);
> +        if (found) {
> +            AtkAttributeSet* t = i->next;
> +            toDelete = g_slist_prepend(toDelete, i->data);
> +            a1 = g_slist_delete_link(a1, i);
> +            i = t;
> +        } else
> +            i = i->next;
> +    }
> +
> +    atk_attribute_set_free(a2);
> +    atk_attribute_set_free(toDelete);
> +    return a1;
> +}
> +
> +static guint accessibilityObjectLength(const AccessibilityObject* object)
> +{
> +    int value = object->stringValue().length();
> +    if (value)
> +        return value;
> +    if (object->isAccessibilityRenderObject()) {
> +        const RenderObject* renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();
> +        if (renderer->isBR())
> +            return 1;
> +    }
> +    return object->textUnderElement().length();
> +}
> +
> +static const AccessibilityObject* getAccessibilityObjectForOffset(const AccessibilityObject* object, guint offset, gint* startOffset, gint* endOffset)
> +{
> +    const AccessibilityObject* result;
> +    guint length = accessibilityObjectLength(object);
> +    if (length > offset) {
> +        *startOffset = 0;
> +        *endOffset = length;
> +        result = object;
> +    } else {
> +        *startOffset = -1;
> +        *endOffset = -1;
> +        result = 0;
> +    }
> +
> +    if (object->firstChild()) {
> +        AccessibilityObject* child = object->firstChild();
> +        guint childPosition = 0;
> +        guint childLength;
> +        while (child) {
> +            childLength = accessibilityObjectLength(child);
> +            if ((childLength + childPosition) > offset) {
> +                gint childStartOffset;
> +                gint childEndOffset;
> +                const AccessibilityObject* grandChild = getAccessibilityObjectForOffset(child, offset-childPosition,  &childStartOffset, &childEndOffset);
> +                if (childStartOffset >= 0) {
> +                    *startOffset = childStartOffset + childPosition;
> +                    *endOffset = childEndOffset + childPosition;
> +                    result = grandChild;
> +                }
> +                break;
> +            }
> +            childPosition += childLength;
> +            child = child->nextSibling();
> +        }
> +    }
> +    return result;
> +}
> +
> +static AtkAttributeSet* getRunAttributesFromAccesibilityObject(const AccessibilityObject* element, gint offset, gint* startOffset, gint* endOffset)
> +{
> +    const AccessibilityObject *child = getAccessibilityObjectForOffset(element, offset, startOffset, endOffset);
> +    if (!child) {
> +        *startOffset = -1;
> +        *endOffset = -1;
> +        return 0;
> +    }
> +
> +    AtkAttributeSet* defaultAttributes = getAttributeSetForAccessibilityObject(element);
> +    AtkAttributeSet* childAttributes = getAttributeSetForAccessibilityObject(child);
> +
> +    return attributeSetDifference(childAttributes, defaultAttributes);
> +}
> +
> +static AtkAttributeSet* webkit_accessible_text_get_run_attributes(AtkText* text, gint offset, gint* startOffset, gint* endOffset)
> +{
> +    AccessibilityObject* coreObject = core(text);
> +    AtkAttributeSet* result;
> +
> +    if (!coreObject) {
> +        *startOffset = 0;
> +        *endOffset = atk_text_get_character_count(text);
> +        return 0;
> +    }
> +
> +    if (offset == -1)
> +        offset = atk_text_get_caret_offset(text);
> +
> +    result = getRunAttributesFromAccesibilityObject(coreObject, offset, startOffset, endOffset);
> +
> +    if (*startOffset < 0) {
> +        *startOffset = offset;
> +        *endOffset = offset;
> +    }
> +
> +    return result;
>  }
>  
>  static AtkAttributeSet* webkit_accessible_text_get_default_attributes(AtkText* text)
>  {
> -    notImplemented();
> -    return 0;
> +    AccessibilityObject* coreObject = core(text);
> +    if (!coreObject || !coreObject->isAccessibilityRenderObject())
> +        return 0;
> +
> +    AtkAttributeSet* result = 0;
> +
> +    result = getAttributeSetForAccessibilityObject(coreObject);
> +    return result;
>  }
>  
>  static void webkit_accessible_text_get_character_extents(AtkText* text, gint offset, gint* x, gint* y, gint* width, gint* height, AtkCoordType coords)
> Index: WebKit/gtk/ChangeLog
> ===================================================================
> --- WebKit/gtk/ChangeLog	(revision 55194)
> +++ WebKit/gtk/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> [phone]  José Millán Soto  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Gtk] Text attributes not exposed
> +        https://bugs.webkit.org/show_bug.cgi?id=25528
> +
> +        Added new tests for accessible text attributes
> +
> +        * tests/testatk.c:
> +        (testWebkitAtkGetTextInParagraphAndBodyModerate):
> +        (compAtkAttribute):
> +        (compAtkAttributeName):
> +        (atkAttributeSetAttributeHasValue):
> +        (atkAttributeSetAreEqual):
> +        (testWebkitAtkTextAttributes):
> +        (main):
> +
>  2010-02-23  Leandro Pereira  <[e-mail]>
>  
>          Reviewed by Gustavo Noronha Silva.
> Index: WebKit/gtk/tests/testatk.c
> ===================================================================
> --- WebKit/gtk/tests/testatk.c	(revision 55194)
> +++ WebKit/gtk/tests/testatk.c	(working copy)
> @@ -38,6 +38,8 @@ static const char* contentsInParagraphAn
>  
>  static const char* contentsInParagraphAndBodyModerate = "<html><body><p>This is a test.</p>Hello world.<br /><font color='#00cc00'>This sentence is green.</font><br />This one is not.</body></html>";
>  
> +static const char* textWithAttributes = "<html><head><style>.st1 {font-family: monospace; color:rgb(120,121,122);} .st2 {text-decoration:underline; background-color:rgb(80,81,82);}</style></head><body><p style=\"font-size:14; text-align:right;\">This is the <i>first</i><b> sentence of this text.</b></p><p class=\"st1\">This sentence should have an style applied <span class=\"st2\">and this part should have another one</span>.</p><p>x<sub>1</sub><sup>2</sup>=x<sub>2</sub><sup>3</sup></p><p style=\"text-align:center;\">This sentence is the <strike>last</strike> one.</p></body></html>";
> +
>  static gboolean bail_out(GMainLoop* loop)
>  {
>      if (g_main_loop_is_running(loop))
> @@ -409,6 +411,7 @@ static void testWebkitAtkGetTextInParagr
>  static void testWebkitAtkGetTextInParagraphAndBodyModerate(void)
>  {
>      WebKitWebView* webView;
> +    WebKitWebFrame* webFrame;
>      AtkObject* obj;
>      AtkObject* obj1;
>      AtkObject* obj2;
> @@ -418,6 +421,7 @@ static void testWebkitAtkGetTextInParagr
>  
>      webView = WEBKIT_WEB_VIEW(webkit_web_view_new());
>      g_object_ref_sink(webView);
> +    webFrame = webkit_web_view_get_main_frame(webView);
>      GtkAllocation alloc = { 0, 0, 800, 600 };
>      gtk_widget_size_allocate(GTK_WIDGET(webView), &alloc);
>      webkit_web_view_load_string(webView, contentsInParagraphAndBodyModerate, NULL, NULL, NULL);
> @@ -450,6 +454,159 @@ static void testWebkitAtkGetTextInParagr
>      g_object_unref(webView);
>  }
>  
> +static gint compAtkAttribute(AtkAttribute *a1, AtkAttribute *a2)
> +{
> +    gint strcmpVal;
> +    strcmpVal = g_strcmp0(a1->name, a2->name);
> +    if (strcmpVal)
> +        return strcmpVal;
> +
> +    return g_strcmp0(a1->value, a2->value);
> +}
> +
> +static gint compAtkAttributeName(AtkAttribute *a1, AtkAttribute *a2)
> +{
> +    return g_strcmp0(a1->name, a2->name);
> +}
> +
> +static gboolean atkAttributeSetAttributeHasValue(AtkAttributeSet *set, AtkTextAttribute attribute, const gchar *value)
> +{
> +    GSList *element;
> +    AtkAttribute at;
> +    gboolean result;
> +    at.name = (gchar *)atk_text_attribute_get_name(attribute);
> +    element = g_slist_find_custom(set, &at, (GCompareFunc)compAtkAttributeName);
> +    result = (element && !g_strcmp0(((AtkAttribute*)(element->data))->value, value));
> +    if (!result && element)
> +        printf("Expected %s got %s\n", value, ((AtkAttribute*)(element->data))->value);
> +    return result;
> +}
> +
> +static gboolean atkAttributeSetAreEqual(AtkAttributeSet *set1, AtkAttributeSet *set2)
> +{
> +    if (!set1)
> +        return !set2;
> +
> +    set1 = g_slist_sort(set1, (GCompareFunc)compAtkAttribute);
> +    set2 = g_slist_sort(set2, (GCompareFunc)compAtkAttribute);
> +
> +    while (set1) {
> +        if (!set2 || compAtkAttribute(set1->data, set2->data))
> +            return FALSE;
> +
> +        set1 = set1->next;
> +        set2 = set2->next;
> +    }
> +
> +    return (!set2);
> +}
> +
> +static void testWebkitAtkTextAttributes(void)
> +{
> +    WebKitWebView* webView;
> +    AtkObject* obj;
> +    AtkObject* child;
> +    GMainLoop* loop;
> +    AtkText* childText;
> +    AtkAttributeSet *set1, *set2, *set3, *set4;
> +    gint startOffset, endOffset, length;
> +
> +    webView = WEBKIT_WEB_VIEW(webkit_web_view_new());
> +    g_object_ref_sink(webView);
> +    GtkAllocation alloc = { 0, 0, 800, 600 };
> +    gtk_widget_size_allocate(GTK_WIDGET(webView), &alloc);
> +
> +    webkit_web_view_load_string(webView, textWithAttributes, NULL, NULL, NULL);
> +    loop = g_main_loop_new(NULL, TRUE);
> +
> +    g_timeout_add(100, (GSourceFunc)bail_out, loop);
> +    g_main_loop_run(loop);
> +
> +    obj = gtk_widget_get_accessible(GTK_WIDGET(webView));
> +    g_assert(obj);
> +
> +    child = atk_object_ref_accessible_child(obj, 0);
> +    g_assert(child && ATK_IS_TEXT(child));
> +    childText = ATK_TEXT(child);
> +    set1 = atk_text_get_run_attributes(childText, 0, &startOffset, &endOffset);
> +    g_assert_cmpint(startOffset, ==, 0);
> +    g_assert_cmpint(endOffset, ==, 12);
> +    g_assert(atkAttributeSetAreEqual(set1, NULL));
> +    set1 = atk_text_get_run_attributes(childText, 15, &startOffset, &endOffset);
> +    g_assert_cmpint(startOffset, ==, 12);
> +    g_assert_cmpint(endOffset, ==, 17);
> +    g_assert(atkAttributeSetAttributeHasValue(set1, ATK_TEXT_ATTR_STYLE, "italic"));
> +    set2 = atk_text_get_run_attributes(childText, 17, &startOffset, &endOffset);
> +    g_assert_cmpint(startOffset, ==, 17);
> +    g_assert_cmpint(endOffset, ==, 40);
> +    g_assert(atkAttributeSetAttributeHasValue(set2, ATK_TEXT_ATTR_WEIGHT, "700"));
> +    set3 = atk_text_get_default_attributes(childText);
> +    g_assert(atkAttributeSetAttributeHasValue(set3, ATK_TEXT_ATTR_STYLE, "normal"));
> +    g_assert(atkAttributeSetAttributeHasValue(set3, ATK_TEXT_ATTR_JUSTIFICATION, "right"));
> +    g_assert(atkAttributeSetAttributeHasValue(set3, ATK_TEXT_ATTR_SIZE, "14"));
> +    atk_attribute_set_free(set1);
> +    atk_attribute_set_free(set2);
> +    atk_attribute_set_free(set3);
> +
> +    child = atk_object_ref_accessible_child(obj, 1);
> +    g_assert(child && ATK_IS_TEXT(child));
> +    childText = ATK_TEXT(child);
> +    set1 = atk_text_get_default_attributes(childText);
> +    g_assert(atkAttributeSetAttributeHasValue(set1, ATK_TEXT_ATTR_FAMILY_NAME, "monospace"));
> +    g_assert(atkAttributeSetAttributeHasValue(set1, ATK_TEXT_ATTR_STYLE, "normal"));
> +    g_assert(atkAttributeSetAttributeHasValue(set1, ATK_TEXT_ATTR_STRIKETHROUGH, "false"));
> +    g_assert(atkAttributeSetAttributeHasValue(set1, ATK_TEXT_ATTR_WEIGHT, "400"));
> +    g_assert(atkAttributeSetAttributeHasValue(set1, ATK_TEXT_ATTR_FG_COLOR, "120,121,122"));
> +    set2 = atk_text_get_run_attributes(childText, 43, &startOffset, &endOffset);
> +    g_assert_cmpint(startOffset, ==, 43);
> +    g_assert_cmpint(endOffset, ==, 80);
> +    // Checks that default attributes of text are not returned when called to atk_text_get_run_attributes
> +    g_assert(!atkAttributeSetAttributeHasValue(set2, ATK_TEXT_ATTR_FG_COLOR, "120,121,122"));
> +    g_assert(atkAttributeSetAttributeHasValue(set2, ATK_TEXT_ATTR_UNDERLINE, "single"));
> +    g_assert(atkAttributeSetAttributeHasValue(set2, ATK_TEXT_ATTR_BG_COLOR, "80,81,82"));
> +    atk_attribute_set_free(set1);
> +    atk_attribute_set_free(set2);
> +
> +    child = atk_object_ref_accessible_child(obj, 2);
> +    g_assert(child && ATK_IS_TEXT(child));
> +    childText = ATK_TEXT(child);
> +    set1 = atk_text_get_run_attributes(childText, 0, &startOffset, &endOffset);
> +    set2 = atk_text_get_run_attributes(childText, 3, &startOffset, &endOffset);
> +    g_assert(atkAttributeSetAreEqual(set1, set2));
> +    atk_attribute_set_free(set2);
> +    set2 = atk_text_get_run_attributes(childText, 1, &startOffset, &endOffset);
> +    set3 = atk_text_get_run_attributes(childText, 5, &startOffset, &endOffset);
> +    g_assert(atkAttributeSetAreEqual(set2, set3));
> +    g_assert(!atkAttributeSetAreEqual(set1, set2));
> +    atk_attribute_set_free(set3);
> +    set3 = atk_text_get_run_attributes(childText, 2, &startOffset, &endOffset);
> +    set4 = atk_text_get_run_attributes(childText, 6, &startOffset, &endOffset);
> +    g_assert(atkAttributeSetAreEqual(set3, set4));
> +    g_assert(!atkAttributeSetAreEqual(set1, set3));
> +    g_assert(!atkAttributeSetAreEqual(set2, set3));
> +    atk_attribute_set_free(set1);
> +    atk_attribute_set_free(set2);
> +    atk_attribute_set_free(set3);
> +    atk_attribute_set_free(set4);
> +
> +    child = atk_object_ref_accessible_child(obj, 3);
> +    g_assert(child && ATK_IS_TEXT(child));
> +    childText = ATK_TEXT(child);
> +    set1 = atk_text_get_run_attributes(childText, 24, &startOffset, &endOffset);
> +    g_assert_cmpint(startOffset, ==, 21);
> +    g_assert_cmpint(endOffset, ==, 25);
> +    g_assert(atkAttributeSetAttributeHasValue(set1, ATK_TEXT_ATTR_STRIKETHROUGH, "true"));
> +    set2 = atk_text_get_run_attributes(childText, 25, &startOffset, &endOffset);
> +    g_assert_cmpint(startOffset, ==, 25);
> +    g_assert_cmpint(endOffset, ==, 30);
> +    g_assert(atkAttributeSetAreEqual(set2, NULL));
> +    set3 = atk_text_get_default_attributes(childText);
> +    g_assert(atkAttributeSetAttributeHasValue(set3, ATK_TEXT_ATTR_JUSTIFICATION, "center"));
> +    atk_attribute_set_free(set1);
> +    atk_attribute_set_free(set2);
> +    atk_attribute_set_free(set3);
> +}
> +
>  int main(int argc, char** argv)
>  {
>      g_thread_init(NULL);
> @@ -463,6 +620,7 @@ int main(int argc, char** argv)
>      g_test_add_func("/webkit/atk/get_text_at_offset_text_input", test_webkit_atk_get_text_at_offset_text_input);
>      g_test_add_func("/webkit/atk/getTextInParagraphAndBodySimple", testWebkitAtkGetTextInParagraphAndBodySimple);
>      g_test_add_func("/webkit/atk/getTextInParagraphAndBodyModerate", testWebkitAtkGetTextInParagraphAndBodyModerate);
> +    g_test_add_func("/webkit/atk/testWebkitAtkTextAttributes", testWebkitAtkTextAttributes);
>      return g_test_run ();
>  }
>  

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1014
 +  
The ChangeLog for Webcore is missing.

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1022
 +  
A nit, but the style guide says to declare variables when needed, not at the beginning of the block.

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1036
 +      }
One space here?

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1046
 +          baselinePosition = -baselinePosition;
My guess is that you are falling through deliberately here to get a negative rise, with includeRise assigned to true in the next case. Please don't do this, makes the code much harder to guess just to save one variable assignment.

Init includeRise to true outside the switch, and put a break here.

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1110
 +          break;
Put this at the end together with default.

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1147
 +  static AtkAttributeSet* attributeSetDifference(AtkAttributeSet* a1, AtkAttributeSet* a2)
Extra space at the begining of the comment?

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1184
 +  
Still believe it makes sense to reuse webkit_accessible_text_get_text here.

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1271
 +      return result;
return getAttributeSetForAccessibilityObject(coreObject);

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1215
 +              }
Let's keep a variable with the current offset and use that as the while condition instead of using a break ...

WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1200
 +          AccessibilityObject* child = object->firstChild();
Just exit early if there's no firstChild, gets rid of one level of indentation.

[Mario Sanchez Prada]

Created attachment 58253 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

Trying to help unblock this issue, here is a new version of Jose's patch address all the issues in comment #10, almost one month ago.

I've kept the authorship as it was since I did not introduce any major change on them, just those needed to address the mentioned issues and some other minor fixes and tweaks I found while testing this stuff both through unit tests and accerciser.

[WebKit Review Bot]

Attachment 58253 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1255:  webkit_accessible_text_get_run_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testatk.c"
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

(In reply to comment #12)
> Attachment 58253 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1255:  webkit_accessible_text_get_run_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testatk.c"
> Total errors found: 1 in 4 files

Yes, I already saw that but I don't have a clue of what to do here since it's a common practice in that file to name all the functions for the ATK roles that way.

Any suggestion?

[Xan Lopez]

Comment on attachment 58253 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

>+    RenderObject* renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();

You don't need those parenthesis.

>+    RenderStyle* style = renderer->style();
>+
>+    const int bufferSize = 16;
>+    gchar buffer[bufferSize];
>+    g_snprintf(buffer, bufferSize, "%i", style->fontSize());

Can we just use g_strdup_printf with GOwnPtr here and in the other places instead of buffers with hardcoded lenght?

>+    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_UNDERLINE), ((style->textDecoration() & UNDERLINE) ? "single" : "none"));

Don't really need that parenthesis either AFAIK.

>+static gint compareAttribute(const AtkAttribute* a, const AtkAttribute* b)
>+{
>+    return (g_strcmp0(a->name, b->name) || g_strcmp0(a->value, b->value));

Neither this.

>+}

>+static guint accessibilityObjectLength(const AccessibilityObject* object)
>+{
>+    int value = object->stringValue().length();
>+    if (value)
>+        return value;
>+    if (object->isAccessibilityRenderObject()) {
>+        const RenderObject* renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();
>+        if (renderer->isBR())
>+            return 1;
>+    }
>+    // return object->textUnderElement().length();
>+    GOwnPtr<gchar> text(webkit_accessible_text_get_text(ATK_TEXT(object->wrapper()), 0, -1));
>+    return g_utf8_strlen (text.get(), -1);

You are kind of duplicating, I think, everything webkit_accessible_get_text does (at least it has code for stringValue().lenght and some special casing for BR); do you positively know you can't just return the lenght of what is returned by that method? If not, why?. Also, there's an extra space in the strlen call.

> 
>+static gint compAtkAttribute(AtkAttribute *a1, AtkAttribute *a2)
>+{
>+    gint strcmpVal;
>+    strcmpVal = g_strcmp0(a1->name, a2->name);
>+    if (strcmpVal)
>+        return strcmpVal;
>+    return g_strcmp0(a1->value, a2->value);
>+}
>+
>+static gint compAtkAttributeName(AtkAttribute *a1, AtkAttribute *a2)
>+{
>+    return g_strcmp0(a1->name, a2->name);
>+}
>+

The '*' are all wrong; I think the style script should just ignore this by now, but if it still gives false warnings just ignore it.

>+static gboolean atkAttributeSetAttributeHasValue(AtkAttributeSet *set, AtkTextAttribute attribute, const gchar *value)
>+{
>+    AtkAttribute at;
>+    at.name = g_strdup(atk_text_attribute_get_name(attribute));
>+
>+    GSList *element = g_slist_find_custom(set, &at, (GCompareFunc)compAtkAttributeName);
>+    g_free(at.name);

Do you really need to make that copy?

>+
>+    gboolean result = (element && !g_strcmp0(((AtkAttribute*)(element->data))->value, value));

No parenthesis.

>+    if (!result && element)
>+        printf("Expected %s got %s\n", value, ((AtkAttribute*)(element->data))->value);

What is this printf for? Shouldn't we assert or something, being this a test...

I think the patch is almost there, let's just fix those things.

[Mario Sanchez Prada]

Created attachment 58670 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

(In reply to comment #14)
> (From update of attachment 58253 [details])
> >+    RenderObject* renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();
> 
> You don't need those parenthesis.

Fixed

> >+    RenderStyle* style = renderer->style();
> >+
> >+    const int bufferSize = 16;
> >+    gchar buffer[bufferSize];
> >+    g_snprintf(buffer, bufferSize, "%i", style->fontSize());
> 
> Can we just use g_strdup_printf with GOwnPtr here and in the other places instead of buffers with hardcoded lenght?

Sure! :-)
Done.

> >+    result = addAttributeToSet(result, atk_text_attribute_get_name(ATK_TEXT_ATTR_UNDERLINE), ((style->textDecoration() & UNDERLINE) ? "single" : "none"));
> 
> Don't really need that parenthesis either AFAIK.

Fixed

> >+static gint compareAttribute(const AtkAttribute* a, const AtkAttribute* b)
> >+{
> >+    return (g_strcmp0(a->name, b->name) || g_strcmp0(a->value, b->value));
> 
> Neither this.

Fixed

> >+static guint accessibilityObjectLength(const AccessibilityObject* object)
> >+{
> >+    int value = object->stringValue().length();
> >+    if (value)
> >+        return value;
> >+    if (object->isAccessibilityRenderObject()) {
> >+        const RenderObject* renderer = (static_cast<const AccessibilityRenderObject*>(object))->renderer();
> >+        if (renderer->isBR())
> >+            return 1;
> >+    }
> >+    // return object->textUnderElement().length();
> >+    GOwnPtr<gchar> text(webkit_accessible_text_get_text(ATK_TEXT(object->wrapper()), 0, -1));
> >+    return g_utf8_strlen (text.get(), -1);
> 
> You are kind of duplicating, I think, everything webkit_accessible_get_text does (at least it has code for stringValue().lenght and some special casing for BR); do you positively know you can't just return the lenght of what is returned by that method? If not, why?. Also, there's an extra space in the strlen call.

Fixed
 
> > 
> >+static gint compAtkAttribute(AtkAttribute *a1, AtkAttribute *a2)
> >+{
> >+    gint strcmpVal;
> >+    strcmpVal = g_strcmp0(a1->name, a2->name);
> >+    if (strcmpVal)
> >+        return strcmpVal;
> >+    return g_strcmp0(a1->value, a2->value);
> >+}
> >+
> >+static gint compAtkAttributeName(AtkAttribute *a1, AtkAttribute *a2)
> >+{
> >+    return g_strcmp0(a1->name, a2->name);
> >+}
> >+
> 
> The '*' are all wrong; I think the style script should just ignore this by now, but if it still gives false warnings just ignore it.

Fixed

> >+static gboolean atkAttributeSetAttributeHasValue(AtkAttributeSet *set, AtkTextAttribute attribute, const gchar *value)
> >+{
> >+    AtkAttribute at;
> >+    at.name = g_strdup(atk_text_attribute_get_name(attribute));
> >+
> >+    GSList *element = g_slist_find_custom(set, &at, (GCompareFunc)compAtkAttributeName);
> >+    g_free(at.name);
> 
> Do you really need to make that copy?

I guess not. Reverted to the original code from Millan, which didn't duplicate anything.

> >+
> >+    gboolean result = (element && !g_strcmp0(((AtkAttribute*)(element->data))->value, value));
> 
> No parenthesis.

Fixed

> >+    if (!result && element)
> >+        printf("Expected %s got %s\n", value, ((AtkAttribute*)(element->data))->value);
> 
> What is this printf for? Shouldn't we assert or something, being this a test...

Reading the whole patch I think this is just some legacy code for debugging purposes, as the return value for this function is immediately used in an assertion so it wouldn't make sense either to assert the same thing here also. Hence, code removed.

> I think the patch is almost there, let's just fix those things.

Attached a new patch addressing all these issues, and tested it both in Accerciser and with the unittests/testatk binary.

[WebKit Review Bot]

Attachment 58670 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1243:  webkit_accessible_text_get_run_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testatk.c"
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

(In reply to comment #16)
> Attachment 58670 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1243:  webkit_accessible_text_get_run_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testatk.c"
> Total errors found: 1 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Can't do anything about this, since that's the common codestyle used in this file and I guess it's better not to change it.

Any idea on how to workaround this? (I'm usually working on this file and this kind of WK Bot failures are a bit annoying).

[Xan Lopez]

Comment on attachment 58670 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

Looks good, but this still uses g_timeout_add in the test. Can you change it to an idle and we get this in? Sorry for the trouble.

[Mario Sanchez Prada]

Created attachment 60022 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

(In reply to comment #18)
> (From update of attachment 58670 [details])
> Looks good, but this still uses g_timeout_add in the test. Can you change it to an idle and we get this in? Sorry for the trouble.

Thanks for noticing. After all it's not just a matter of replacing the timeout for and idle_add but also about having a patch that compiles against current status of trunk :-)

Created a new patch against trunk then and tested, so here you have it.

[WebKit Review Bot]

Attachment 60022 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1229:  webkit_accessible_text_get_run_attributes is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/gtk/tests/testatk.c"
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Comment on attachment 60022 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

r=me

[WebKit Commit Bot]

Comment on attachment 60022 [details]
webkit_accessible_text_get_default_attributes and webkit_accessible_text_get_run_attributes implementation

Clearing flags on attachment: 60022

Committed r62164: <http://trac.webkit.org/changeset/62164>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.