Closed Bug 457166 Opened 12 years ago Closed 12 years ago

Conversation links in GMail Inbox do not expose an action, cannot be activated by screen readers.

Categories

(Core :: Disability Access APIs, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: MarcoZ, Assigned: surkov)

References

()

Details

(Keywords: access, verified1.9.0.7, verified1.9.1)

Attachments

(2 files, 2 obsolete files)

This was reported by Freedom Scientific, but probably affects all screen readers across all platforms.

STR:
1. Login to your GMail account.
2. Go to Inbox.
3. Look at the links for any of the inbox conversations. Using a screen reader, try to activate any of the links.

Result: It does not work.

Observations:
1. The table cell containing the subject and possible number of messages in the conversation is given an ARIA role of "link".
2. The text is nested within 2 divs and a span within that table cell.
3. The link accessible created for this does not expose any actions, making it impossible for screen readers to activate it.

This appears to be new in Google's markup, I remember having seen this working previously.

This prevents *any* screen reader from working with this major web app, requesting that this block 1.9.1 and also one of the next 1.9.0.x releases.
Flags: blocking1.9.1?
Flags: blocking1.9.0.4?
Assignee: nobody → surkov.alexander
Attached patch patch (obsolete) — Splinter Review
Attachment #340545 - Flags: review?(aaronleventhal)
Attachment #340545 - Flags: review?(marco.zehe)
Attachment #340545 - Flags: review?(marco.zehe) → review+
Comment on attachment 340545 [details] [diff] [review]
patch

I'm getting one test failure when running the whole suite:
821 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_nsIAccessible_actions.html | Wrong action name of the accessible for combobox_expanded - got "open", expected "close"

Did you see this while developing the feature? r=me with that answered/if the tests run OK on tinderboxes.

Other than that, the patch fixes the problem at gmail. Great work!
(In reply to comment #2)
> (From update of attachment 340545 [details] [diff] [review])
> I'm getting one test failure when running the whole suite:
> 821 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochikit/content/a11y/accessible/test_nsIAccessible_actions.html |
> Wrong action name of the accessible for combobox_expanded - got "open",
> expected "close"
> 
> Did you see this while developing the feature? r=me with that answered/if the
> tests run OK on tinderboxes.

Yep. I didn't think you test the patches ;) Actually I think it's a bug that aria-expanded didn't drop collapsed state. I'll file the bug and fix it so this patch won't be modified. Of course, I'm ensure to land that bug before this one. Thank you you keeping your eyes open.

> Other than that, the patch fixes the problem at gmail. Great work!

Great. Well we ware right then!
Marco, I filed bug 457219 for the issue with collapsed/expanded states.
Why do we pass in the state as a parameter instead of just using State(this) inside the method?
Attachment #340545 - Flags: review?(aaronleventhal) → review+
Comment on attachment 340545 [details] [diff] [review]
patch

Other than that r+
(In reply to comment #5)
> Why do we pass in the state as a parameter instead of just using State(this)
> inside the method?

I just remember state calculating may be complex task (for example visibility states). So once I got state I try to reuse it.
Status: NEW → ASSIGNED
Thanks, that's what I thought. I didn't see the state reuse before but I see it now.
Attached patch patch2Splinter Review
up to dated to trunk
Attachment #340545 - Attachment is obsolete: true
Flags: in-testsuite+
checked in, http://hg.mozilla.org/mozilla-central/rev/32bfbcb0e5dd
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: blocking1.9.0.4? → blocking1.9.0.4-
Attached patch Gecko 1.9.0 port of patch (obsolete) — Splinter Review
Alex, can you take a look at this? It works in Gmail, but something is wrong with the tests. It doesn't run through the tests, doesn't output a log, and the test harness window is open with some content I don't have access to. I suppose something went wrong with porting the test or with what gets included from common.js.
Attachment #358357 - Flags: review?(surkov.alexander)
Attached patch 1.9.0 port2Splinter Review
I see one test failure in test_nsIAccessible_actions.html

not ok - Wrong action name of the accessible for combobox_expanded got "open", expected "close"

it's because bug 457219 wasn't fixed on 1.9.0. Until we don't need that bug there I would suggest to remove tests for "combobox_expanded".

test_nsIAccessible_actions.xul works fine

running all tests together (manually "run tests" button) works fine with fixed test_nsIAccessible_actions.html.

Does it work for you?
Attachment #358357 - Attachment is obsolete: true
Attachment #358357 - Flags: review?(surkov.alexander)
Marco, ping
Yes, this patch works in principle, although I find it strange that I can't seem to be able to get all tests running with our usual command line. It opens the test harness window instead of running the tests and putting stuff into the log file. But anyway...The tests pass for me with your patch applied.
Comment on attachment 358430 [details] [diff] [review]
1.9.0 port2

This patch addresses a problem screen readers have in GMail where they can't open a message anymore due to changes in Google's markup.
Attachment #358430 - Flags: approval1.9.0.7?
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
Priority: -- → P3
Comment on attachment 358430 [details] [diff] [review]
1.9.0 port2

Approved for 1.9.0.7, a=dveditz for release-drivers.
Attachment #358430 - Flags: approval1.9.0.7? → approval1.9.0.7+
Checking in accessible/src/base/nsARIAMap.cpp;
/cvsroot/mozilla/accessible/src/base/nsARIAMap.cpp,v  <--  nsARIAMap.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in accessible/src/base/nsARIAMap.h;
/cvsroot/mozilla/accessible/src/base/nsARIAMap.h,v  <--  nsARIAMap.h
new revision: 1.11; previous revision: 1.10
done
Checking in accessible/src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.380; previous revision: 1.379
done
Checking in accessible/src/base/nsAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsAccessible.h,v  <--  nsAccessible.h
new revision: 1.125; previous revision: 1.124
done
Checking in accessible/tests/mochitest/nsIAccessible_actions.js;
/cvsroot/mozilla/accessible/tests/mochitest/nsIAccessible_actions.js,v  <--  nsIAccessible_actions.js
new revision: 1.2; previous revision: 1.1
done
Checking in accessible/tests/mochitest/test_nsIAccessible_actions.html;
/cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessible_actions.html,v  <--  test_nsIAccessible_actions.html
new revision: 1.2; previous revision: 1.1
done
Checking in accessible/tests/mochitest/test_nsIAccessible_actions.xul;
/cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessible_actions.xul,v  <--  test_nsIAccessible_actions.xul
new revision: 1.3; previous revision: 1.2
done
Keywords: fixed1.9.0.7
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.7pre) Gecko/2009021006 GranParadiso/3.0.7pre

Also, this landed on m-c when m-c was still 1.9.1, setting verified accordingly.
You need to log in before you can comment on or make changes to this bug.