Bug 114354

Summary: [ATK] Adds an accessibility support to access a value of the color control element
Product: WebKit Reporter: Krzysztof Czech <k.czech>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, gyuyoung.kim, jdiggs, lucas.de.marchi, mario, rakuco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
mario: review-, commit-queue: commit-queue-
Patch
none
Patch none

Description Krzysztof Czech 2013-04-10 05:56:45 PDT 
Making color control element to be accessible by adding AtkText interface.
Supporting new role "AXColorWell" in WTR and DRT.
Providing new expected results of color-well.html into EFL port.
Comment 1 Krzysztof Czech 2013-04-12 02:02:55 PDT 
Created attachment 197732 [details]
Patch
Comment 2 Mario Sanchez Prada 2013-04-15 03:21:18 PDT 
I think it would be more accurate to implement AtkValue interface [1] for this kind of input elements, instead of partly implementing AtkText, which seems a bit "forced" to me in this case.

API, joanie... what do you think?

[1] https://developer.gnome.org/atk/unstable/AtkValue.html
Comment 3 Joanmarie Diggs 2013-04-15 06:53:31 PDT 
(In reply to comment #2)
> I think it would be more accurate to implement AtkValue interface [1] for this kind of input elements, instead of partly implementing AtkText, which seems a bit "forced" to me in this case.
> 
> API, joanie... what do you think?
> 
> [1] https://developer.gnome.org/atk/unstable/AtkValue.html

While AtkValue's docs suggest the Value can be whatever, AT-SPI2 seems to expect [2] -- and provide [3] -- a double. So I would either go with AtkText or I would change the bridge and the AT-SPI2 API to accommodate string values first.

As for whether or not it is "forced" to go with AtkText, with AtkValue [1] you also need to provide a minimum, a maximum, and increments. What will those be here? (For me, AtkValue is the more "forced" of the two.)

[2] https://git.gnome.org/browse/at-spi2-atk/tree/atk-adaptor/adaptors/value-adaptor.c
[3] https://git.gnome.org/browse/at-spi2-core/tree/atspi/atspi-value.c
Comment 4 Mario Sanchez Prada 2013-04-15 08:21:34 PDT 
(In reply to comment #3)
> (In reply to comment #2)
> > I think it would be more accurate to implement AtkValue interface [1] for this kind of input elements, instead of partly implementing AtkText, which seems a bit "forced" to me in this case.
> > 
> > API, joanie... what do you think?
> > 
> > [1] https://developer.gnome.org/atk/unstable/AtkValue.html
> 
> While AtkValue's docs suggest the Value can be whatever, AT-SPI2 seems to 
> expect [2] -- and provide [3] -- a double. So I would either go with AtkText
> or I would change the bridge and the AT-SPI2 API to accommodate string values
> first.

Ah, I see the problem now. However I'm still not convinced whether to expose it through AtkText will be the right thing to do because it will create more questions in my mind related to the implementation of other functions in AtkText (e.g. those related to the position of the caret).

I'm not saying I'm against this change, after all I will basically trust you Joanie way more than myself when it comes to define requirements for ATs, just trying to see the pros and the cons of each solution.
 
> As for whether or not it is "forced" to go with AtkText, with AtkValue [1]
> you also need to provide a minimum, a maximum, and increments. What will
> those be here? (For me, AtkValue is the more "forced" of the two.)

Yes, I already thought about that and there was no easy answer for those cases either, other than the obvious "let's use 0 as minimum, 2^64 as maximum and 1.0 as increment" :-)
Comment 5 Krzysztof Czech 2013-09-04 03:45:32 PDT 
Created attachment 210443 [details]
Patch
Comment 6 Krzysztof Czech 2013-09-04 04:06:41 PDT 
(In reply to comment #2)
> I think it would be more accurate to implement AtkValue interface [1] for this kind of input elements, instead of partly implementing AtkText, which seems a bit "forced" to me in this case.
> 
> API, joanie... what do you think?
> 

I do not have any other arguments than those what joanie said. I agree with her.
Comment 7 Krzysztof Czech 2013-09-04 05:40:48 PDT 
Created attachment 210447 [details]
Patch
Comment 8 Mario Sanchez Prada 2013-09-04 05:45:48 PDT 
Comment on attachment 210447 [details]
Patch

(In reply to comment #6)
> (In reply to comment #2)
> > I think it would be more accurate to implement AtkValue interface [1] for this kind of input elements, instead of partly implementing AtkText, which seems a bit "forced" to me in this case.
> > 
> > API, joanie... what do you think?
> > 
> 
> I do not have any other arguments than those what joanie said. I agree with her.

It seems you both agree on that and, on top of that, I also can't see a better solution so I'm fine with that.

We'll probably need to rebaseline the results for GTK too, but that's something we can do once it's landed.
Comment 9 WebKit Commit Bot 2013-09-04 05:47:11 PDT 
Comment on attachment 210447 [details]
Patch

Rejecting attachment 210447 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 210447, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/1693587
Comment 10 Mario Sanchez Prada 2013-09-04 05:51:03 PDT 
Comment on attachment 210447 [details]
Patch

(In reply to comment #9)
> (From update of attachment 210447 [details])
> Rejecting attachment 210447 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 210447, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
> 
> ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
> 
> Full output: http://webkit-queues.appspot.com/results/1693587

Oops! It seems the ChangeLog was kind of duplicated (and I missed that one for some reason, too).

Please fix it and provide a new patch
Comment 11 Krzysztof Czech 2013-09-04 05:58:23 PDT 
Created attachment 210450 [details]
Patch
Comment 12 Krzysztof Czech 2013-09-04 06:00:13 PDT 
> Oops! It seems the ChangeLog was kind of duplicated (and I missed that one for some reason, too).
> 
> Please fix it and provide a new patch
Done.
Comment 13 Krzysztof Czech 2013-09-04 06:30:13 PDT 
Created attachment 210453 [details]
Patch
Comment 14 Mario Sanchez Prada 2013-09-04 06:54:50 PDT 
Comment on attachment 210453 [details]
Patch

Thanks. Let's give it another shot then...
Comment 15 WebKit Commit Bot 2013-09-04 07:18:02 PDT 
Comment on attachment 210453 [details]
Patch

Clearing flags on attachment: 210453

Committed r155041: <http://trac.webkit.org/changeset/155041>
Comment 16 WebKit Commit Bot 2013-09-04 07:18:06 PDT 
All reviewed patches have been landed.  Closing bug.