[Webkit-unassigned] [Bug 25528] [Gtk] Text attributes not exposed

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 29 04:05:57 PST 2010


https://bugs.webkit.org/show_bug.cgi?id=25528


Xan Lopez <xan.lopez at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #46683|review?                     |review-
               Flag|                            |




--- Comment #7 from Xan Lopez <xan.lopez at gmail.com>  2010-01-29 04:05:57 PST ---
(From update of attachment 46683)
>-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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list