Call log items should be listbox options

RESOLVED FIXED in 2.1 S2 (15aug)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: eeejay, Assigned: yzen)

Tracking

({access})

unspecified
2.1 S2 (15aug)
All
Gonk (Firefox OS)
access
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2ga11y p=1] )

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Also, the embedded aside should be descriptive (missed call vs. outgoing vs. incoming).
(Reporter)

Comment 1

4 years ago
Re-aligning priorities with 2.1 accessibility goals.
Whiteboard: [b2ga11y p=1]
(Assignee)

Updated

4 years ago
Depends on: 1050926
(Assignee)

Updated

4 years ago
Assignee: nobody → yzenevich
(Assignee)

Comment 2

4 years ago
Created attachment 8470260 [details] [review]
Github pull request.
Attachment #8470260 - Flags: review?(etienne)
Comment on attachment 8470260 [details] [review]
Github pull request.

redirecting...
Attachment #8470260 - Flags: review?(etienne) → review?(anthony)
Comment on attachment 8470260 [details] [review]
Github pull request.

Sorry for the multiple redirections. We have some 2.1 committed work in bug 1045499. We may delay landing this patch for after bug 1045499 does.

Doug, since you're working on the other bug, I'll let you review this and decide on the landing order.
Attachment #8470260 - Flags: review?(anthony) → review?(drs+bugzilla)
Status: NEW → ASSIGNED
Target Milestone: --- → 2.1 S2 (15aug)
Comment on attachment 8470260 [details] [review]
Github pull request.

I'd rather land this before bug 1045499 and I'll deal with the bitrotting.

I left some comments in the PR. I'm disappointed in the lack of test coverage here, but I don't think it's fair to ask for you to write an arsenal of new tests just for this. Though I would like to ask that you file a followup bug for this so we can get to it at some point.

You will also find that the call_log_test.js file has several failing tests, so you'll have to fix those. Once you've dealt with this, I may ask for one or two new tests, but we'll see when the time comes.
Attachment #8470260 - Flags: review?(drs+bugzilla) → review-
(Assignee)

Comment 6

4 years ago
Comment on attachment 8470260 [details] [review]
Github pull request.

I updated the PR with your comments. Regarding longpress/failing test, I missed that logItem var was used lower in the function. All works now.

(In reply to Doug Sherk (:drs) from comment #5)
> Comment on attachment 8470260 [details] [review]
> Github pull request.
> 
> I'd rather land this before bug 1045499 and I'll deal with the bitrotting.
> 
> I left some comments in the PR. I'm disappointed in the lack of test
> coverage here, but I don't think it's fair to ask for you to write an
> arsenal of new tests just for this. 

This is an incremental improvement to the original call log a11y (bug 923139). There are a number of gaia ui tests written for the original bugs and afaik there are no regression with this one.

> Though I would like to ask that you file
> a followup bug for this so we can get to it at some point.
> 
> You will also find that the call_log_test.js file has several failing tests,
> so you'll have to fix those. Once you've dealt with this, I may ask for one
> or two new tests, but we'll see when the time comes.

Fixed (see above).

FYI, testing for a11y attributes as part of unit tests is not an optimal. As an example, some component might be re-written in a way that the attributes would still be present however the functionality would change (different controls, different interactions etc). This is why pretty much all the a11y tests are written as gaia ui tests to test the actual interaction.
Attachment #8470260 - Flags: review- → review?(drs+bugzilla)
Comment on attachment 8470260 [details] [review]
Github pull request.

(In reply to Yura Zenevich [:yzen] from comment #6)
> FYI, testing for a11y attributes as part of unit tests is not an optimal. As
> an example, some component might be re-written in a way that the attributes
> would still be present however the functionality would change (different
> controls, different interactions etc). This is why pretty much all the a11y
> tests are written as gaia ui tests to test the actual interaction.

Thanks, that's good to know, and I didn't know about that. This looks good to me now.

I'm asking for feedback from Anthony as I'm relatively unfamiliar with a11y. I had to look up some stuff, especially about ARIA, to be able to review this. Please don't land it until he comments.
Attachment #8470260 - Flags: feedback?(anthony)
Attachment #8470260 - Flags: review?(drs+bugzilla) → review+
(Assignee)

Comment 8

4 years ago
(In reply to Doug Sherk (:drs) from comment #7)
> I'm asking for feedback from Anthony as I'm relatively unfamiliar with a11y.
> I had to look up some stuff, especially about ARIA, to be able to review
> this. Please don't land it until he comments.

Sounds good, thanks
Comment on attachment 8470260 [details] [review]
Github pull request.

I haven't tested on a device but I believe this could break one case.
Attachment #8470260 - Flags: feedback?(anthony) → feedback-
(Assignee)

Comment 10

4 years ago
Comment on attachment 8470260 [details] [review]
Github pull request.

Addressed comments in the PR. Anthony, I left another comment in the PR regarding the handling click. Thanks.
Attachment #8470260 - Flags: feedback- → feedback?(anthony)

Updated

4 years ago
Attachment #8470260 - Flags: feedback?(anthony) → feedback+
(Assignee)

Comment 11

4 years ago
https://github.com/mozilla-b2g/gaia/commit/a2e85e3947b1c99e5cead61a302bfc1eedfe211c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.