938197 - [ATK Accessibility] AtkTextAttributes derived names should be used on AtkText methods get_run/default_attributs

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Alejandro Piñeiro]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130917102914

Steps to reproduce:

atk_text_get_run/default_attributes [1][2] returns an AtkAttributeSet with text attributes. Although AtkAttributeSet allows to define custom name/value pairs, the documentation of those methods explains that the default names are defined on the enum AtkTextAttribute. But right now Firefox expose those settings using only custom firefox-defined names. Due this, ATs like Orca need to do some mapping with the common names [4], mapping that imho, should be done by the implementor.

Note that in order to get the name of the attribute from the enum, ATK provides the utility method atk_text_attribute_get_name [5].

You can see an example of usage of that enum and methods on WebKitGTK implementation here [6]

[1] https://developer.gnome.org/atk/stable/AtkText.html#atk-text-get-run-attributes
[2] https://developer.gnome.org/atk/stable/AtkText.html#atk-text-get-default-attributes
[3] https://developer.gnome.org/atk/stable/AtkText.html#AtkTextAttribute
[4] https://git.gnome.org/browse/orca/tree/src/orca/scripts/toolkits/Gecko/script.py#n209
[5] https://developer.gnome.org/atk/stable/AtkText.html#atk-text-attribute-get-name
[6] http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp#L81

Comment 1

[Trevor Saunders (:tbsaunde)]

(In reply to Alejandro Piñeiro from comment #0)
> User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101
> Firefox/24.0 (Beta/Release)
> Build ID: 20130917102914
> 
> Steps to reproduce:
> 
> atk_text_get_run/default_attributes [1][2] returns an AtkAttributeSet with
> text attributes. Although AtkAttributeSet allows to define custom name/value
> pairs, the documentation of those methods explains that the default names
> are defined on the enum AtkTextAttribute. But right now Firefox expose those
> settings using only custom firefox-defined names. Due this, ATs like Orca
> need to do some mapping with the common names [4], mapping that imho, should
> be done by the implementor.

It shouldn't be too hard to expose the attributes using the common names as well, I guess it isn't an issue if we keep the old names as well for backward compat?

> Note that in order to get the name of the attribute from the enum, ATK
> provides the utility method atk_text_attribute_get_name [5].

that seems rather silly, isn't there just a list of what those enum values map to as strings someplace?  Why is it an enum anyway?

Comment 2

[Alejandro Piñeiro]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> (In reply to Alejandro Piñeiro from comment #0)
> > User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101
> > Firefox/24.0 (Beta/Release)
> > Build ID: 20130917102914
> > 
> > Steps to reproduce:
> > 
> > atk_text_get_run/default_attributes [1][2] returns an AtkAttributeSet with
> > text attributes. Although AtkAttributeSet allows to define custom name/value
> > pairs, the documentation of those methods explains that the default names
> > are defined on the enum AtkTextAttribute. But right now Firefox expose those
> > settings using only custom firefox-defined names. Due this, ATs like Orca
> > need to do some mapping with the common names [4], mapping that imho, should
> > be done by the implementor.
> 
> It shouldn't be too hard to expose the attributes using the common names as
> well, I guess it isn't an issue if we keep the old names as well for
> backward compat?
> 
> > Note that in order to get the name of the attribute from the enum, ATK
> > provides the utility method atk_text_attribute_get_name [5].
> 
> that seems rather silly, isn't there just a list of what those enum values
> map to as strings someplace?  Why is it an enum anyway?

The enum is convenient and easy to use while writing code.

About the mapping, right now is basically a sttriped form of the enum name. Eventually we would go to use CSS/XSLT name convention:
https://bugzilla.gnome.org/show_bug.cgi?id=363439#c5

Comment 3

[Trevor Saunders (:tbsaunde)]

> > > Note that in order to get the name of the attribute from the enum, ATK
> > > provides the utility method atk_text_attribute_get_name [5].
> > 
> > that seems rather silly, isn't there just a list of what those enum values
> > map to as strings someplace?  Why is it an enum anyway?
> 
> The enum is convenient and easy to use while writing code.

sure, but you could get  basically the same benefits with a bunch of const char *atk_text_attribute_foo = "foobar"; but I guess that's a design decision from a while ago we can't fix now :(

> About the mapping, right now is basically a sttriped form of the enum name.
> Eventually we would go to use CSS/XSLT name convention:
> https://bugzilla.gnome.org/show_bug.cgi?id=363439#c5

that sounds like it could have backwards compat fun, but anyway can I get a list of the strings some place? or do I have to write code to cache them at run time?

Comment 4

[Alejandro Piñeiro]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > > Note that in order to get the name of the attribute from the enum, ATK
> > > > provides the utility method atk_text_attribute_get_name [5].
> > > 
> > > that seems rather silly, isn't there just a list of what those enum values
> > > map to as strings someplace?  Why is it an enum anyway?
> > 
> > The enum is convenient and easy to use while writing code.
> 
> sure, but you could get  basically the same benefits with a bunch of const
> char *atk_text_attribute_foo = "foobar"; but I guess that's a design
> decision from a while ago we can't fix now :(

Yep.

> > About the mapping, right now is basically a sttriped form of the enum name.
> > Eventually we would go to use CSS/XSLT name convention:
> > https://bugzilla.gnome.org/show_bug.cgi?id=363439#c5
> 
> that sounds like it could have backwards compat fun, but anyway can I get a
> list of the strings some place? or do I have to write code to cache them at
> run time?

This is the reason I think that it is better to just use atk_text_attribute_get_name. In that way ATK implementors shouldn't worry about setting a name, and just use what ATK provides.

And in relation with backwards compatibility, it would be also help us if we change the name convention as mentioned on bgo#363439, as all the ATK implementors would change at the same time, and for the same names.

Comment 5

[Trevor Saunders (:tbsaunde)]

builds with a test patch should soon show up at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-68ede9eb55a8 can you please check all the attributes you think should use the atk name do?

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: bug 938197 - alias atk text attr names to gecko ones

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 8345464 [details] [diff] [review]
bug 938197 - alias atk text attr names to gecko ones

Review of attachment 8345464 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsMaiInterfaceText.cpp
@@ +20,3 @@
>  using namespace mozilla::a11y;
>  
> +static const char* atkTextAttrNames[ATK_TEXT_ATTR_LAST_DEFINED];

adding g or s prefix is usually what we do

@@ +24,5 @@
> +static AtkAttributeSet*
> +ConvertToAtkTextAttributeSet(nsIPersistentProperties* aAttributes)
> +{
> +    if (!aAttributes)
> +        return nullptr;

nit: two spaces offset please (in whole method)

@@ +26,5 @@
> +{
> +    if (!aAttributes)
> +        return nullptr;
> +
> +    AtkAttributeSet *objAttributeSet = nullptr;

nit: type* name (here and below)

@@ +31,5 @@
> +    nsCOMPtr<nsISimpleEnumerator> propEnum;
> +    nsresult rv = aAttributes->Enumerate(getter_AddRefs(propEnum));
> +    NS_ENSURE_SUCCESS(rv, nullptr);
> +
> +    bool hasMore;

= fasle; pls

@@ +51,5 @@
> +
> +        AtkAttribute *objAttr = (AtkAttribute *)g_malloc(sizeof(AtkAttribute));
> +        objAttr->name = g_strdup(name.get());
> +        objAttr->value = g_strdup(NS_ConvertUTF16toUTF8(value).get());
> +        objAttributeSet = g_slist_prepend(objAttributeSet, objAttr);

so keeping two version of same attribute, backward compat?

@@ +57,5 @@
> +        // Handle attributes where atk has its own name.
> +        const char* atkName = nullptr;
> +        nsAutoString atkValue;
> +        if (name.EqualsLiteral("color")) {
> +          atkValue = Substring(value, 5, value.Length() - 1);

cutting rgb() thing? it'd be nice to have a comment

@@ +81,5 @@
> +          objAttr = static_cast<AtkAttribute*>(g_malloc(sizeof(AtkAttribute)));
> +              objAttr->name = g_strdup(atkName);
> +              objAttr->value = g_strdup(NS_ConvertUTF16toUTF8(atkValue).get());
> +              objAttributeSet = g_slist_prepend(objAttributeSet, objAttr);
> +              }

nit: wrong indentation as well

@@ +485,5 @@
>    aIface->remove_selection = removeTextSelectionCB;
>    aIface->set_selection = setTextSelectionCB;
>    aIface->set_caret_offset = setCaretOffsetCB;
> +
> +  for (uint32_t i = 0; i < ArrayLength(atkTextAttrNames); i++)

nit: would be nice to have small comment

@@ +487,5 @@
>    aIface->set_caret_offset = setCaretOffsetCB;
> +
> +  for (uint32_t i = 0; i < ArrayLength(atkTextAttrNames); i++)
> +    atkTextAttrNames[i] =
> +      atk_text_attribute_get_name (static_cast<AtkTextAttribute>(i));

nit: no space after function name

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: bug 938197 - alias atk text attr names to gecko ones

yes, keeping the old attributes is about backward compat.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8345534 [details] [diff] [review]
bug 938197 - alias atk text attr names to gecko ones

Review of attachment 8345534 [details] [diff] [review]:
-----------------------------------------------------------------

few more nits

::: accessible/src/atk/nsMaiInterfaceText.cpp
@@ +48,5 @@
> +    nsAutoString value;
> +    rv = propElem->GetValue(value);
> +    NS_ENSURE_SUCCESS(rv, objAttributeSet);
> +
> +    AtkAttribute *objAttr = (AtkAttribute *)g_malloc(sizeof(AtkAttribute));

type* name

@@ +70,5 @@
> +    } else if (name.EqualsLiteral("font-family")) {
> +      atkValue = value;
> +      atkName = sAtkTextAttrNames[ATK_TEXT_ATTR_FAMILY_NAME];
> +    } else if (name.Equals("font-size")) {
> +      atkValue = StringHead(value, value.Length() - 2);

nit: a comment we cut measure unit

@@ +88,5 @@
> +      objAttributeSet = g_slist_prepend(objAttributeSet, objAttr);
> +    }
> +  }
> +
> +  //libspi will free it

nit: space after //

Comment 10

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb34959ba6f8

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cb34959ba6f8