[webkit-reviews] review denied: [Bug 121477] [ATK] Expose aria-valuetext as an object attribute. : [Attachment 211885] Patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Krzysztof Czech
<k.czech at samsung.com>'s request for review:
Bug 121477: [ATK] Expose aria-valuetext as an object attribute.
https://bugs.webkit.org/show_bug.cgi?id=121477

Attachment 211885: Patch
https://bugs.webkit.org/attachment.cgi?id=211885&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=211885&action=review


(In reply to comment #4)
> [...]
> > I have just commented on that bug. If we get the correct API in place, will
you please use that instead?
> Yes sure, as soon as we get the correct ATK API, I will use it instead.
> 
I've just reviewed the patch and, besides the things I pointed out below, I
think the implementation in the wrapper is ok if we compare it to what W3C says
now. However, I agree with joanie that we will need to re-do this anyway in
terms of the new API in AtkValue as soon as it gets there, but for the time
being I'd personally go with this.

Do you agree, Joanie?

Btw, giving r- because of the thing about sharing the tests and the missing
implementation for DRT

> LayoutTests/ChangeLog:17
> +	   * platform/efl/accessibility/aria-valuetext-expected.txt: Added.
> +	   *
platform/efl/accessibility/aria-valuetext-on-native-slider-expected.txt: Added.

> +	   * platform/efl/accessibility/aria-valuetext-on-native-slider.html:
Added.
> +	   * platform/efl/accessibility/aria-valuetext.html: Added.
> +	   * platform/gtk/accessibility/aria-valuetext-expected.txt: Added.
> +	   *
platform/gtk/accessibility/aria-valuetext-on-native-slider-expected.txt: Added.

> +	   * platform/gtk/accessibility/aria-valuetext-on-native-slider.html:
Added.
> +	   * platform/gtk/accessibility/aria-valuetext.html: Added.

It looks to me like these 2 tests could be shared as cross-platform ones. Why
not reusing the ones from the mac by moving them and adapting DRT & WKTR to
provide the same output?

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:808
> +    String attributeValue =
getAttributeSetValueForId(ATK_OBJECT(m_element.get()), "valuetext");

Here it might be a good place to prepend the "AXValueDescription: " string, so
we can share the tests.

Btw, I don't see the change for DRT in this patch