Bug 111665

Summary: AXRoleDescription for DL should be "description list" instead of "list"
Product: WebKit Reporter: James Craig <jcraig>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aboxhall, apinheiro, buildbot, cfleizach, dglazkov, dmazzoni, fishd, gyuyoung.kim, jamesr, jcraig, jdiggs, jochen, mifenton, rakuco, rniwa, rwlbuis, tkent+wkapi, tonikitoo, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
patch. none

Description James Craig 2013-03-06 19:10:01 PST
AXRoleDescripton for DL is “list”; should be “description list"

Probably fallout from: http://webkit.org/b/107650
Comment 1 James Craig 2013-04-01 18:30:04 PDT
<rdar://problem/13537579>
Comment 2 James Craig 2013-04-01 18:31:09 PDT
Created attachment 196051 [details]
patch
Comment 3 WebKit Review Bot 2013-04-01 22:54:11 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 chris fleizach 2013-04-01 22:55:03 PDT
Comment on attachment 196051 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1884
> +        if (listObject->isDescriptionList())

is it necessary to cast to a list? 
I don't know if isList() corresponds to AccessibilityList. 
If it is necessary we should use toAccessibilityList() instead of a static cast
Comment 5 Build Bot 2013-04-02 03:44:31 PDT
Comment on attachment 196051 [details]
patch

Attachment 196051 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17354951

New failing tests:
media/video-controls-visible-audio-only.html
Comment 6 Build Bot 2013-04-02 03:44:35 PDT
Created attachment 196116 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 7 James Craig 2013-04-02 11:52:25 PDT
Not sure why that test is failing.

$ run-webkit-tests -2 media/video-controls-visible-audio-only.html
Using port 'mac-mountainlion-wk2'
Test configuration: <mountainlion, x86_64, release>
Placing test results in ./WebKitBuild/Release/layout-test-results
Baseline search path: mac-wk2 -> wk2 -> mac -> generic
Using Release build
Pixel tests disabled
Regular timeout: 80000, slow test timeout: 400000
Command line: ./WebKitBuild/Release/WebKitTestRunner -

Found 1 test; running 1, skipping 0.
Running 1 WebKitTestRunner.     

The test ran as expected.
Comment 8 James Craig 2013-04-02 12:03:02 PDT
(In reply to comment #4)
> (From update of attachment 196051 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196051&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1884
> > +        if (listObject->isDescriptionList())
> 
> is it necessary to cast to a list? 
> I don't know if isList() corresponds to AccessibilityList. 
> If it is necessary we should use toAccessibilityList() instead of a static cast

static_cast is used in identically in the subrole() method just before this use in roleDescription().
Comment 9 chris fleizach 2013-04-02 12:06:02 PDT
(In reply to comment #5)
> (From update of attachment 196051 [details])
> Attachment 196051 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-commit-queue.appspot.com/results/17354951
> 
> New failing tests:
> media/video-controls-visible-audio-only.html

seems unlikely to be related

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff949c9212 __pthread_kill + 10
1   libsystem_c.dylib             	0x00007fff8e406af4 pthread_kill + 90
2   libsystem_c.dylib             	0x00007fff8e44adce abort + 143
3   libc++abi.dylib               	0x00007fff96f15a17 abort_message + 257
4   libc++abi.dylib               	0x00007fff96f13490 __cxa_pure_virtual + 18
5   com.apple.WebCore             	0x0000000105053059 WebCore::MediaControlTextTrackContainerElement::updateDisplay() + 105 (MediaControlElements.cpp:1249)
6   com.apple.WebCore             	0x0000000104b6c483 WebCore::HTMLMediaElement::~HTMLMediaElement() + 355 (OwnPtr.h:63)
7   com.apple.WebCore             	0x0000000104bccda2 WebCore::HTMLVideoElement::~HTMLVideoElement() + 162 (FastMalloc.h:269)
8   com.apple.WebCore             	0x00000001047eeed8 WebCore::ContainerNode::removeDetachedChildren() + 200 (ContainerNodeAlgorithms.h:91)
9   com.apple.WebCore             	0x00000001047ef590 WebCore::ContainerNode::~ContainerNode() + 48 (ContainerNode.cpp:144)
10  com.apple.WebCore             	0x0000000104a2a16f WebCore::Element::~Element() + 351 (Element.cpp:217)
11  com.apple.WebCore             	0x0000000104b5ebee WebCore::HTMLHtmlElement::~HTMLHtmlElement() + 14 (FastMalloc.h:269)
12  com.apple.WebCore             	0x0000000104e9082f WebCore::JSNodeOwner::finalize(JSC::Handle<JSC::Unknown>, void*) + 63 (JSNode.h:69)
1
Comment 10 chris fleizach 2013-04-02 12:06:28 PDT
Comment on attachment 196051 [details]
patch

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

>>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1884
>>> +        if (listObject->isDescriptionList())
>> 
>> is it necessary to cast to a list? 
>> I don't know if isList() corresponds to AccessibilityList. 
>> If it is necessary we should use toAccessibilityList() instead of a static cast
> 
> static_cast is used in identically in the subrole() method just before this use in roleDescription().

looked at the code. looks like this is the state of the art. 
we should add the toAccessibilityList() method in another patch
Comment 11 chris fleizach 2013-04-02 12:07:34 PDT
Comment on attachment 196051 [details]
patch

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

> Source/WebCore/ChangeLog:3
> +        AXRoleDescripton for DL is âlistâ; should be âdescription list"

are there some funky unicode characters in this title?

> Tools/ChangeLog:3
> +        AXRoleDescripton for DL is âlistâ; should be âdescription list"

ditto for the funky characters
Comment 12 James Craig 2013-04-02 14:40:55 PDT
Created attachment 196229 [details]
patch.

removed unicode char errs and accidental whitespace diffs from previous patch.
Comment 13 James Craig 2013-04-02 14:45:15 PDT
(In reply to comment #10)
> we should add the toAccessibilityList() method in another patch

https://bugs.webkit.org/show_bug.cgi?id=113828
Comment 14 WebKit Review Bot 2013-04-02 23:15:46 PDT
Comment on attachment 196229 [details]
patch.

Clearing flags on attachment: 196229

Committed r147520: <http://trac.webkit.org/changeset/147520>
Comment 15 WebKit Review Bot 2013-04-02 23:15:52 PDT
All reviewed patches have been landed.  Closing bug.