Bug 117730 - [ATK] Added support for aria-required attribute
Summary: [ATK] Added support for aria-required attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-18 02:52 PDT by Krzysztof Czech
Modified: 2013-06-21 03:34 PDT (History)
10 users (show)

See Also:


Attachments
Patch (11.93 KB, patch)
2013-06-18 06:48 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
Patch (12.59 KB, patch)
2013-06-18 07:29 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
Patch (12.51 KB, patch)
2013-06-19 04:38 PDT, Krzysztof Czech
cfleizach: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Rebased patch (18.63 KB, patch)
2013-06-21 02:00 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Czech 2013-06-18 02:52:03 PDT
The aria-required attribute is used to indicate that user input is required on an element before a form can be submitted. This attribute can be used with any typical HTML form element.
Comment 1 Krzysztof Czech 2013-06-18 06:48:38 PDT
Created attachment 204909 [details]
Patch
Comment 2 Mario Sanchez Prada 2013-06-18 07:07:31 PDT
The patch looks good to me, although I think it might be a good idea to extend it slightly so we also have the isRequired() method implemented in DumpRenderTree (AccessibityUIElementAtk.cpp)
Comment 3 Krzysztof Czech 2013-06-18 07:29:43 PDT
Created attachment 204912 [details]
Patch
Comment 4 Krzysztof Czech 2013-06-18 07:31:16 PDT
(In reply to comment #2)
> The patch looks good to me, although I think it might be a good idea to extend it slightly so we also have the isRequired() method implemented in DumpRenderTree (AccessibityUIElementAtk.cpp)

Sounds good, done.
Comment 5 chris fleizach 2013-06-18 09:04:53 PDT
Comment on attachment 204912 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204912&action=review

Why does these tests need to be efl/gtk only? they look like they could pass on the Mac without issue as well.
There are also a few accessibility "required" tests that already exist. Do those provide the same coverage? Can they be unskilled by GTK?

> LayoutTests/platform/efl/accessibility/aria-required.html:25
> +          var succeeded = obj.isRequired;

Before outputting these results it would be nice to explain what is being tested like
debug("Verify that isRequired is true when aria-required=true");
So that a failing case is easy to diagnose

> LayoutTests/platform/efl/accessibility/aria-required.html:26
> +          shouldBe("succeeded", "true");

for these, it would be preferable to write
shouldBeTrue("obj.isRequired")

which will give more context if they fail
Comment 6 Krzysztof Czech 2013-06-19 04:37:29 PDT
(In reply to comment #5)
> (From update of attachment 204912 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204912&action=review
> 
> Why does these tests need to be efl/gtk only? they look like they could pass on the Mac without issue as well.
Yes you are right, they're all passing on mac as well. Basically, those two tests are already in mac. I guess moving them to LayoutTests/accessibility it's a good idea. I wasn't really sure about moving them. Thanks.

> There are also a few accessibility "required" tests that already exist. Do those provide the same coverage? Can they be unskilled by GTK?
Right, I think there are 3 accessibility "required" tests. Those two I'm moving are passing on GTK/EFL port, other one it's not.
> 
> > LayoutTests/platform/efl/accessibility/aria-required.html:25
> > +          var succeeded = obj.isRequired;
> 
> Before outputting these results it would be nice to explain what is being tested like
> debug("Verify that isRequired is true when aria-required=true");
> So that a failing case is easy to diagnose
> 
> > LayoutTests/platform/efl/accessibility/aria-required.html:26
> > +          shouldBe("succeeded", "true");
> 
> for these, it would be preferable to write
> shouldBeTrue("obj.isRequired")
> 
> which will give more context if they fail

+ var succeeded = !obj.isRequired;
+ shouldBe("succeeded", "true");
I think, this might be change to shouldBeFalse("obj.isRequired") since we are
expecting to be false.

Thanks for your suggestions.
Comment 7 Krzysztof Czech 2013-06-19 04:38:03 PDT
Created attachment 204987 [details]
Patch
Comment 8 WebKit Commit Bot 2013-06-20 09:09:49 PDT
Comment on attachment 204987 [details]
Patch

Rejecting attachment 204987 [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-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 204987, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/git.webkit.org/WebKit
   7598594..0c9e165  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 151775 = 759859404bea2807d382deb49e3811336870ab76
r151776 = 0c9e16534f2239a101584831d3f9d49671deff40
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.appspot.com/results/922393
Comment 9 Krzysztof Czech 2013-06-21 02:00:41 PDT
Created attachment 205159 [details]
Rebased patch
Comment 10 WebKit Commit Bot 2013-06-21 03:33:57 PDT
Comment on attachment 205159 [details]
Rebased patch

Clearing flags on attachment: 205159

Committed r151831: <http://trac.webkit.org/changeset/151831>
Comment 11 WebKit Commit Bot 2013-06-21 03:34:00 PDT
All reviewed patches have been landed.  Closing bug.