Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184103036:
Follow ARIA spec guidelines for role=button with aria-pressed. (Closed)
|
Created:
6 years, 9 months ago by aboxhall Modified:
6 years, 9 months ago CC:
chromium-reviews, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, jam, yuzo+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFollow ARIA spec guidelines for role=button with aria-pressed.
BUG=349626
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255584
Patch Set 1 #Patch Set 2 : Remove unnecessary lines from value method #Patch Set 3 : Remove unused include of string_util.h in browser_accessibility_cocoa.mm #
Total comments: 4
Patch Set 4 : Camel case, trailing space, re-ordering check, checking for toggle button role in role() #Patch Set 5 : Trailing space for real #
Total comments: 6
Patch Set 6 : Fix togglebutton test and expectation on mac #
Messages
Total messages: 27 (0 generated)
Not sure who the best reviewer is out of Dominic and David.
+jamesr@ for webkit/glue strings
lgtm https://codereview.chromium.org/184103036/diff/40001/webkit/glue/webkit_strin... File webkit/glue/webkit_strings.grd (right): https://codereview.chromium.org/184103036/diff/40001/webkit/glue/webkit_strin... webkit/glue/webkit_strings.grd:33: * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. this seems unrelated
lgtm https://codereview.chromium.org/184103036/diff/40001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/184103036/diff/40001/content/browser/accessib... content/browser/accessibility/browser_accessibility_cocoa.mm:668: bool is_aria_pressed_defined; Nit: Obj-C files use camelCase for local variables.
For future reference: David's a better reviewer if you're doing anything nontrivial with Objective-C or Cocoa, though I'm getting a lot more comfortable with it. In this case it's all trivial so either of us is fine.
https://codereview.chromium.org/184103036/diff/40001/webkit/glue/webkit_strin... File webkit/glue/webkit_strings.grd (right): https://codereview.chromium.org/184103036/diff/40001/webkit/glue/webkit_strin... webkit/glue/webkit_strings.grd:33: * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. On 2014/03/06 05:53:58, jamesr wrote: > this seems unrelated Yeah, my editor is configured to strip trailing whitespace on save. Is it a problem if I leave this in, or should I go fix it?
https://codereview.chromium.org/184103036/diff/40001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/184103036/diff/40001/content/browser/accessib... content/browser/accessibility/browser_accessibility_cocoa.mm:668: bool is_aria_pressed_defined; On 2014/03/06 06:09:09, dmazzoni wrote: > Nit: Obj-C files use camelCase for local variables. Done.
https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... content/browser/accessibility/browser_accessibility_cocoa.mm:209: { ui::AX_ROLE_TOGGLE_BUTTON, @"AXToggleButton" }, I would match whatever WebKit uses here for a subrole. Cocoa defines NSAccessibilityToggleSubrole, but I'm not sure if that's the subrole expected here. Did you try reading such a control with VoiceOver? https://bugs.webkit.org/show_bug.cgi?id=115298 appears to be the WebKit version of this bug and doesn't seem to be fixed. https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... content/browser/accessibility/browser_accessibility_cocoa.mm:673: if (isAriaPressedDefined) || isMixed as well.
On Thu, Mar 6, 2014 at 10:15 AM, <dtseng@chromium.org> wrote: > ui::AX_ROLE_TOGGLE_BUTTON, @"AXToggleButton" }, > I would match whatever WebKit uses here for a subrole. Cocoa defines > NSAccessibilityToggleSubrole, but I'm not sure if that's the subrole > expected here. > I think this matches WebKit. I'm not sure what OS X API version NSAccessibilityToggleSubrole is defined in, it's relatively recent. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... content/browser/accessibility/browser_accessibility_cocoa.mm:209: { ui::AX_ROLE_TOGGLE_BUTTON, @"AXToggleButton" }, On 2014/03/06 18:15:17, David Tseng wrote: > I would match whatever WebKit uses here for a subrole. Cocoa defines > NSAccessibilityToggleSubrole, but I'm not sure if that's the subrole expected > here. > > Did you try reading such a control with VoiceOver? > > https://bugs.webkit.org/show_bug.cgi?id=115298 > appears to be the WebKit version of this bug and doesn't seem to be fixed. This is broken in Safari currently. Should I use the pre-defined subrole? I'm just going on what the spec says. https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... content/browser/accessibility/browser_accessibility_cocoa.mm:673: if (isAriaPressedDefined) On 2014/03/06 18:15:17, David Tseng wrote: > || isMixed as well. isAriaPressedDefined is set to true if the value of aria-pressed is of {true,false,mixed}. I just need to define isMixed here to avoid a NPE in GetAriaTristate.
https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... content/browser/accessibility/browser_accessibility_cocoa.mm:209: { ui::AX_ROLE_TOGGLE_BUTTON, @"AXToggleButton" }, On 2014/03/06 18:23:31, aboxhall wrote: > On 2014/03/06 18:15:17, David Tseng wrote: > > I would match whatever WebKit uses here for a subrole. Cocoa defines > > NSAccessibilityToggleSubrole, but I'm not sure if that's the subrole expected > > here. > > > > Did you try reading such a control with VoiceOver? > > > > https://bugs.webkit.org/show_bug.cgi?id=115298 > > appears to be the WebKit version of this bug and doesn't seem to be fixed. > > This is broken in Safari currently. Should I use the pre-defined subrole? I'm > just going on what the spec says. It depends; does VoiceOver read the literal subrole (i.e. "<button name> axtogglebutton"? Does it echo the new state(s) when pressed (with control-opt-space)? https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... content/browser/accessibility/browser_accessibility_cocoa.mm:673: if (isAriaPressedDefined) On 2014/03/06 18:23:31, aboxhall wrote: > On 2014/03/06 18:15:17, David Tseng wrote: > > || isMixed as well. > > isAriaPressedDefined is set to true if the value of aria-pressed is of > {true,false,mixed}. I just need to define isMixed here to avoid a NPE in > GetAriaTristate. Yup; that makes sense.
On 2014/03/06 18:36:30, David Tseng wrote: > https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... > File content/browser/accessibility/browser_accessibility_cocoa.mm (right): > > https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... > content/browser/accessibility/browser_accessibility_cocoa.mm:209: { > ui::AX_ROLE_TOGGLE_BUTTON, @"AXToggleButton" }, > On 2014/03/06 18:23:31, aboxhall wrote: > > On 2014/03/06 18:15:17, David Tseng wrote: > > > I would match whatever WebKit uses here for a subrole. Cocoa defines > > > NSAccessibilityToggleSubrole, but I'm not sure if that's the subrole > expected > > > here. > > > > > > Did you try reading such a control with VoiceOver? > > > > > > https://bugs.webkit.org/show_bug.cgi?id=115298 > > > appears to be the WebKit version of this bug and doesn't seem to be fixed. > > > > This is broken in Safari currently. Should I use the pre-defined subrole? I'm > > just going on what the spec says. > > It depends; does VoiceOver read the literal subrole (i.e. "<button name> > axtogglebutton"? Does it echo the new state(s) when pressed (with > control-opt-space)? "toggle button" "selected toggle button" "mixed toggle button" respectively, for the examples in the test in this CL (excluding the plain button and eliding the text on the button)
On 2014/03/06 19:25:10, aboxhall wrote: > On 2014/03/06 18:36:30, David Tseng wrote: > > > https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... > > File content/browser/accessibility/browser_accessibility_cocoa.mm (right): > > > > > https://codereview.chromium.org/184103036/diff/80001/content/browser/accessib... > > content/browser/accessibility/browser_accessibility_cocoa.mm:209: { > > ui::AX_ROLE_TOGGLE_BUTTON, @"AXToggleButton" }, > > On 2014/03/06 18:23:31, aboxhall wrote: > > > On 2014/03/06 18:15:17, David Tseng wrote: > > > > I would match whatever WebKit uses here for a subrole. Cocoa defines > > > > NSAccessibilityToggleSubrole, but I'm not sure if that's the subrole > > expected > > > > here. > > > > > > > > Did you try reading such a control with VoiceOver? > > > > > > > > https://bugs.webkit.org/show_bug.cgi?id=115298 > > > > appears to be the WebKit version of this bug and doesn't seem to be fixed. > > > > > > This is broken in Safari currently. Should I use the pre-defined subrole? > I'm > > > just going on what the spec says. > > > > It depends; does VoiceOver read the literal subrole (i.e. "<button name> > > axtogglebutton"? Does it echo the new state(s) when pressed (with > > control-opt-space)? > > "toggle button" > "selected toggle button" > "mixed toggle button" > > respectively, for the examples in the test in this CL (excluding the plain > button and eliding the text on the button) Echoes "select" and "deselect" (and then the rest of the original text) when pressing.
LGTM Not sure if that's the right thing to say, but it can make sense to a user. I was sort of expecting "pressed" or "not pressed"; don't think we can have any influence on this though (beyond filing a bug on open radar). On Thu, Mar 6, 2014 at 11:27 AM, <aboxhall@chromium.org> wrote: > On 2014/03/06 19:25:10, aboxhall wrote: > >> On 2014/03/06 18:36:30, David Tseng wrote: >> > >> > > https://codereview.chromium.org/184103036/diff/80001/ > content/browser/accessibility/browser_accessibility_cocoa.mm > >> > File content/browser/accessibility/browser_accessibility_cocoa.mm(right): >> > >> > >> > > https://codereview.chromium.org/184103036/diff/80001/ > content/browser/accessibility/browser_accessibility_cocoa.mm#newcode209 > >> > content/browser/accessibility/browser_accessibility_cocoa.mm:209: { >> > ui::AX_ROLE_TOGGLE_BUTTON, @"AXToggleButton" }, >> > On 2014/03/06 18:23:31, aboxhall wrote: >> > > On 2014/03/06 18:15:17, David Tseng wrote: >> > > > I would match whatever WebKit uses here for a subrole. Cocoa defines >> > > > NSAccessibilityToggleSubrole, but I'm not sure if that's the subrole >> > expected >> > > > here. >> > > > >> > > > Did you try reading such a control with VoiceOver? >> > > > >> > > > https://bugs.webkit.org/show_bug.cgi?id=115298 >> > > > appears to be the WebKit version of this bug and doesn't seem to be >> > fixed. > >> > > >> > > This is broken in Safari currently. Should I use the pre-defined >> subrole? >> I'm >> > > just going on what the spec says. >> > >> > It depends; does VoiceOver read the literal subrole (i.e. "<button name> >> > axtogglebutton"? Does it echo the new state(s) when pressed (with >> > control-opt-space)? >> > > "toggle button" >> "selected toggle button" >> "mixed toggle button" >> > > respectively, for the examples in the test in this CL (excluding the plain >> button and eliding the text on the button) >> > > Echoes "select" and "deselect" (and then the rest of the original text) > when > pressing. > > > https://codereview.chromium.org/184103036/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/06 23:02:42, David Tseng wrote: > LGTM > > > Not sure if that's the right thing to say, but it can make sense to a user. > I was sort of expecting "pressed" or "not pressed"; don't think we can have > any influence on this though (beyond filing a bug on open radar). I agree - I assume it's because it's using a main role of CheckBox since it's just based on the value being set to 0 or 1. Can we file a bug?
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/184103036/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/184103036/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg
The CQ bit was checked by aboxhall@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/184103036/100001
I filed radar 16257881. On Thu, Mar 6, 2014 at 8:24 PM, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/aboxhall@ > chromium.org/184103036/100001 > > > https://codereview.chromium.org/184103036/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Change committed as 255584 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
