Bug 127447

Created attachment 221905 [details] first draft (works, but still needs substantial cleanup)

Note: the excess whitespace diffs are b/c my branch is a month or two out of date. I've been working on this in my spare time and was tired of resolving conflicts along the way. Will merge as part of the other cleanup.

Comment on attachment 221905 [details] first draft (works, but still needs substantial cleanup) View in context: https://bugs.webkit.org/attachment.cgi?id=221905&action=review Some drive by comments to help. > Source/WebCore/accessibility/AccessibilityObject.cpp:1508 > +typedef HashMap<int, String> ARIAReverseRoleMap; Nit: int => AccessibilityRole > Source/WebCore/accessibility/AccessibilityObject.cpp:1520 > -static ARIARoleMap* createARIARoleMap() > +static void initializeRoleMap() > { > + if (gAriaRoleMap) Does this need to be initialized in a thread safe manner? Or is it only ever accessed from the WebCore thread? > Source/WebCore/inspector/protocol/DOM.json:206 > + { "name": "objectGroup", "type": "string", "optional": true, "description": "todo" } I see "objectGroup" is listed as a Todo item. I don't think you need this, because you aren't creating any RemoteObjects or explicitly keeping any JavaScript objects alive on the inspected page. You can remove this. EventListeners uses the objectGroup to wrap the event listener handler function from the inspected page. You're just sending a bunch of strings over to the frontend, none of which are objects that could be garbage collected. > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1 > -þÿvar localizedStrings = new Object; > +ÿþvar localizedStrings = new Object; Yikes, the changes to this file look weird. > Source/WebInspectorUI/UserInterface/DOMNode.js:446 > + /** > + * @param {function(?Protocol.Error)=} callback > + */ No need to add these comments unless they help you. We no longer use them but we haven't gotten through and stripped them out. > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:257 > + if (label === "" && domNode.getAttribute('aria-label')) > + label = domNode.getAttribute('aria-label'); Style: Always use double quoted strings in the inspector. 'aria-label' => "aria-label" > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:260 > + WebInspector.UIString("%s (computed)").replace("%s", label); You should not use .replace() on a localized string. We provide String.prototype.format, which is like printf style filling in of %x format strings with .format()'s parameters. So: WebInspector.UIString("%s (computed)").format(label);

(In reply to comment #3) > (From update of attachment 221905 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221905&action=review > > Source/WebCore/accessibility/AccessibilityObject.cpp:1508 > > +typedef HashMap<int, String> ARIAReverseRoleMap; > > Nit: int => AccessibilityRole The compiler actually chokes on this change, believe it or not. Could not get it to build until changing AccessibilityRole to int. I was planning to at that in a comment. Asumption is that it doesn't want an enum as the hashmap key. Would love a better solution if you know the fix.

Comment on attachment 221905 [details] first draft (works, but still needs substantial cleanup) View in context: https://bugs.webkit.org/attachment.cgi?id=221905&action=review >>> Source/WebCore/accessibility/AccessibilityObject.cpp:1508 >>> +typedef HashMap<int, String> ARIAReverseRoleMap; >> >> Nit: int => AccessibilityRole > > The compiler actually chokes on this change, believe it or not. Could not get it to build until changing AccessibilityRole to int. I was planning to at that in a comment. Asumption is that it doesn't want an enum as the hashmap key. Would love a better solution if you know the fix. Ahh, it is an enum. I see. Is the error you get something like DefaultHash: /usr/local/include/wtf/HashMap.h:41:79: error: implicit instantiation of undefined template 'WTF::DefaultHash<Foo>' I'm sure there is a way to fix this but for now jus leave it int. I'll ask the C++ gurus over here if they have a solution.

Created attachment 223290 [details] Patch fixes last crash and reconciles TOT merge; still needs some cleanup: comment/todo removals.

Comment on attachment 223290 [details] Patch fixes last crash and reconciles TOT merge; still needs some cleanup: comment/todo removals. View in context: https://bugs.webkit.org/attachment.cgi?id=223290&action=review > Source/WebCore/accessibility/AccessibilityObject.h:786 > + virtual String computedRoleString() const; this doesn't need to be virtual since no one will override > Source/WebCore/accessibility/AccessibilityObject.h:788 > + // MSAA no need to add this comment in this patch > Source/WebCore/inspector/InspectorDOMAgent.cpp:1427 > + if (node){ space after ) > Source/WebCore/inspector/InspectorDOMAgent.cpp:1429 > + if (axObjectCache) { this can be written as if (AXObjectCache* cache = node->document()...) > Source/WebCore/inspector/InspectorDOMAgent.cpp:1431 > + if (axObject) This can be written as if (AXObject* axObject = axObjectCache->getO...) > Source/WebCore/inspector/InspectorDOMAgent.h:134 > + virtual void getAccessibilityPropertiesForNode(ErrorString*, int in_nodeId, RefPtr<Inspector::TypeBuilder::DOM::AccessibilityProperties>& out_properties) override; these probably don't need to be virtual

Created attachment 223315 [details] progress Found one more crash (on page reload when displaying the Accessibility Node Inspector) and I still need to get the hashmap template working.

Not sure what's going on with all the diffs in the loc strings file (my local diff looks fine). I guess I'll have to revert that one and add back in those new strings manually.

Created attachment 223316 [details] patch seeing if this one fixes the unicode oddities in the loc strings file

localizedStrings.js is a binary file. You can use "svn-create-patch" or "webkit-patch upload".

Created attachment 223506 [details] patch with test coverage

Comment on attachment 223506 [details] patch with test coverage View in context: https://bugs.webkit.org/attachment.cgi?id=223506&action=review > LayoutTests/accessibility/roles-computedRoleString-expected.txt:2 > +This tests that native elements and ARIA overrides result in the same ARIA computed role, regardless of platform. You should add an entry in TestExpectation in GTK marking this as failing until computedRoleString is implemented > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2067 > +- (NSString*)computedRoleString need a space before the * > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2113 > + if ([attributeName isEqualToString: @"AXAriaRole"]) This should be AXARIARole and no space after the ":" even though other lines are doing that. > Source/WebCore/inspector/InspectorDOMAgent.cpp:879 > + Element* element = assertElement(errorString, nodeId); Is there a reason you need to get the element here? you don't see to use it. It seems just getting the node, and checking if that's nil should be good enough > Source/WebCore/inspector/InspectorDOMAgent.cpp:1412 > +PassRefPtr<TypeBuilder::DOM::AccessibilityProperties> InspectorDOMAgent::buildObjectForAccessibilityProperties(Node* node) you should probably ASSERT(node) in here, and if (!node) return nullptr > Source/WebCore/inspector/InspectorDOMAgent.cpp:1424 > + String label = ""; i don't think you need to initialize these strings to anything > Source/WebCore/inspector/InspectorDOMAgent.cpp:1429 > + isAccessibilityObject = true; this should probably check accessibilityIsIgnored() to determine true/false > Source/WebCore/inspector/protocol/DOM.json:55 > + "description": "A structure holding EventListener properties." this seems unrelated to this patch > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:598 > + NSString* computedRoleString = descriptionOfValue([m_element accessibilityAttributeValue:@"AXAriaRole"], m_element); * on the wrong side > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:642 > + NSString* computedRoleString = descriptionOfValue([m_element accessibilityAttributeValue:@"AXAriaRole"], m_element); * on wrong side

(In reply to comment #13) > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2067 > > +- (NSString*)computedRoleString > > need a space before the * Just following the file conventions. Could you raise a separate bug to fix all of those? - (NSString*)role - (NSString*)subrole - (NSString*)roleDescription > > Source/WebCore/inspector/InspectorDOMAgent.cpp:879 > > + Element* element = assertElement(errorString, nodeId); > > Is there a reason you need to get the element here? you don't see to use it. > It seems just getting the node, and checking if that's nil should be good enough Without it, there is a condition where I'll crash on page reload in between the time when node becomes valid, before element becomes valid. A few of the other InspectorDOMAgent methods use the same fix. I work on the rest of your feedback.

(In reply to comment #14) > (In reply to comment #13) > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2067 > > > +- (NSString*)computedRoleString > > > > need a space before the * > > Just following the file conventions. Could you raise a separate bug to fix all of those? > > - (NSString*)role > - (NSString*)subrole > - (NSString*)roleDescription > Yep those are wrong and need to be fixed at some point, but we shouldn't keep making the same mistake > > > Source/WebCore/inspector/InspectorDOMAgent.cpp:879 > > > + Element* element = assertElement(errorString, nodeId); > > > > Is there a reason you need to get the element here? you don't see to use it. > > It seems just getting the node, and checking if that's nil should be good enough > > Without it, there is a condition where I'll crash on page reload in between the time when node becomes valid, before element becomes valid. A few of the other InspectorDOMAgent methods use the same fix. So it seem you could just use Element then, since it's a subclass of Node > > I work on the rest of your feedback.

(In reply to comment #15) > So it seem you could just use Element then, since it's a subclass of Node Actually, your ASSERT(node) suggestion seems to be a better solution to the problem.

(In reply to comment #16) > (In reply to comment #15) > > > So it seem you could just use Element then, since it's a subclass of Node > > Actually, your ASSERT(node) suggestion seems to be a better solution to the problem. Assert only covers debug cases, so you'll still need to plan if this happens in production code

(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > > > So it seem you could just use Element then, since it's a subclass of Node > > > > Actually, your ASSERT(node) suggestion seems to be a better solution to the problem. > > Assert only covers debug cases, so you'll still need to plan if this happens in production code Included this too. if (!node) return nullptr; I just noticed element won't cover the cases of accessible text nodes, which have a "text" role and computed label value, so it'd be nice to keep those visible in the inspector.

Created attachment 223524 [details] patch with test coverage and first round review feedback Given that this feature patch is pushing 60k, I'd also like final review from a member of the Web Inspector team once Chris gives it the thumbs up.

Comment on attachment 223524 [details] patch with test coverage and first round review feedback View in context: https://bugs.webkit.org/attachment.cgi?id=223524&action=review Included some comments. Looks good overall. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2112 > + // this is used only by DumpRenderTree for testing (so far) Style: WebKit Style is for comments to be full sentences. Start with a capital, end with a period. > Source/WebCore/inspector/protocol/DOM.json:210 > + "name": "getAccessibilityPropertiesForNode", > + "parameters": [ > + { "name": "nodeId", "$ref": "NodeId", "description": "Id of the node for which to get accessibility properties." } > + ], > + "returns": [ > + { "name": "properties", "$ref": "AccessibilityProperties", "description": "Dictionary of relevant accessibility properties." } > + ], > + "description": "Returns a dictionary of accessibility properties for the node." > + }, Nit: I prefer the ordering name, description, parameters, returns. Easier on the eyes. Also, it would be nice to have a small protocol test for this: LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForDebugger.html We prefer any new protocol methods to include a test. But since there are probably no good tests for the DOM domain right now I think we can avoid it since you have such a nice LayoutTest for the AX portion of this. > Source/WebInspectorUI/UserInterface/DOMNode.js:446 > /** > + * @param {function(?Protocol.Error)=} callback > + */ Might as well remove this comment. The existing ones are legacy. And this comment is only partially accurate, the callback returns more then just an error. > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:252 > + role = WebInspector.UIString("No exact recognized role match."); Hmm, as a user I wouldn't quite understand what this string means. > Source/WebInspectorUI/UserInterface/InspectorWebBackendCommands.js:105 > +InspectorBackend.registerCommand("DOM.getAccessibilityPropertiesForNode", [{"name": "nodeId", "type": "number", "optional": false}, {"name": "objectGroup", "type": "string", "optional": true}], ["properties"]); This is out of date. This includes "objectGroup" which doesn't exist in the DOM.json protocol definition. Regenerate this file: shell> ./Source/WebInspectorUI/Scripts/update-InspectorBackendCommands.rb

(In reply to comment #20) > > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:252 > > + role = WebInspector.UIString("No exact recognized role match."); > > Hmm, as a user I wouldn't quite understand what this string means. I think I originally called it, "No exact ARIA role match." Do you think that would be more understandable?

(In reply to comment #20) > (From update of attachment 223524 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223524&action=review > > Included some comments. Looks good overall. Thanks. I'll put up a new patch with all the suggested changes. > Nit: I prefer the ordering name, description, parameters, returns. Easier on the eyes. Done, but if this bugs you, you might want to raise another bug to rearrange the rest of that file, as almost none of the rest order it that. > Also, it would be nice to have a small protocol test for this: > LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForDebugger.html > > We prefer any new protocol methods to include a test. But since there are probably no good tests for the DOM domain right now I think we can avoid it since you have such a nice LayoutTest for the AX portion of this. Okay. I went ahead and raised another bug <http://webkit.org/b/128493> to track the additional protocol test. I have some more features in mind for the Accessibility Node Inspector that should be tested in the protocol, even though the 1.0 version has good coverage in the accessibility layout test.

Created attachment 223619 [details] patch with review feedback

Comment on attachment 223619 [details] patch with review feedback View in context: https://bugs.webkit.org/attachment.cgi?id=223619&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:1582 > +static ARIARoleMap* ariaRoleMap() This should be: static ARIARoleMap& ariaRoleMap(). > Source/WebCore/accessibility/AccessibilityObject.cpp:1588 > +static ARIAReverseRoleMap* reverseAriaRoleMap() This should be: static ARIAReverseRoleMap& reverseAriaRoleMap() > Source/WebCore/accessibility/AccessibilityObject.cpp:1598 > + ARIARoleMap* roleMap = ariaRoleMap(); Would use a reference here then. We are trying to remove pointers where possible. > Source/WebCore/accessibility/AccessibilityObject.cpp:1618 > + ARIAReverseRoleMap* roleMap = reverseAriaRoleMap(); > + return roleMap->get(roleValue()); No need for the local, put on one line. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2114 > + // AXARIARole is only used by DumpRenderTree (so far). > + if ([attributeName isEqualToString:@"AXARIARole"]) > + return [self computedRoleString]; Is this exposed to the web? > Source/WebCore/inspector/protocol/DOM.json:61 > + { "name": "accessibilityIsIgnored", "type": "boolean", "description": "Returns whether the DOM node is exposed to the accessibility API." }, accessibilityIsIgnored is kind of wordy given that it is in AccessibilityProperties object. This could just be "ignored" or "hidden"? > Source/WebInspectorUI/UserInterface/DOMNode.js:447 > + accessibilityProperties: function(callback) > + { > + DOMAgent.getAccessibilityPropertiesForNode(this.id, callback); > + }, This leaks the protocol callback and result object into the Inspector. We try to insulate the other classes by keeping all the protocol objects and callbacks in the Manager classes. Some of the older DOM code still does not do this, but new code should. This should have a local callback that gets passed to DOMAgent.getAccessibilityPropertiesForNode and that local callback checks for errors, and makes a new object for the properties before calling the client callback. This allows us to insulate the calls from future compatibility changes. > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:255 > + role = WebInspector.UIString("%s (default)").replace("%s", role); Use: WebInspector.UIString("%s (default)").format(role); > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:257 > + role = WebInspector.UIString("%s (computed)").replace("%s", role); Ditto. > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:262 > + if (label && label != domNode.getAttribute("aria-label")) !== > Source/WebInspectorUI/UserInterface/DetailsSection.css:126 > - table-layout: fixed; > + /* Removed 'table-layout: fixed;' because of WebKit CSS bug: http://webkit.org/b/128294 */ Just remove the whole line. We don't commit commented out code. We might not even need to add it back, layout seems fine without it.

(In reply to comment #24) > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2114 > > + // AXARIARole is only used by DumpRenderTree (so far). > > + if ([attributeName isEqualToString:@"AXARIARole"]) > > + return [self computedRoleString]; > > Is this exposed to the web? Do you mean through a public DOM interface on Element? It's possible this will be added in ARIA 1.1, but nothing public yet. > > Source/WebInspectorUI/UserInterface/DetailsSection.css:126 > > - table-layout: fixed; > > + /* Removed 'table-layout: fixed;' because of WebKit CSS bug: http://webkit.org/b/128294 */ > > Just remove the whole line. We don't commit commented out code. We might not even need to add it back, layout seems fine without it. I'm worried that, without the comment, someone will add it back and regress this. I don't really consider this commented code. It's just a comment that references code. What about this? /* Removed fixed table layout because of WebKit CSS bug: http://webkit.org/b/128294 */

(In reply to comment #25) > (In reply to comment #24) > > > Source/WebInspectorUI/UserInterface/DetailsSection.css:126 > > > - table-layout: fixed; > > > + /* Removed 'table-layout: fixed;' because of WebKit CSS bug: http://webkit.org/b/128294 */ > > > > Just remove the whole line. We don't commit commented out code. We might not even need to add it back, layout seems fine without it. > > I'm worried that, without the comment, someone will add it back and regress this. I don't really consider this commented code. It's just a comment that references code. What about this? > > /* Removed fixed table layout because of WebKit CSS bug: http://webkit.org/b/128294 */ I would just remove it altogether. I don't think we need table-layout: fixed even if that bug is fixed.

(In reply to comment #24) > > Source/WebCore/inspector/protocol/DOM.json:61 > > + { "name": "accessibilityIsIgnored", "type": "boolean", "description": "Returns whether the DOM node is exposed to the accessibility API." }, > > accessibilityIsIgnored is kind of wordy given that it is in AccessibilityProperties object. This could just be "ignored" or "hidden"? I want to leave this one as-is for now. I don't think "presentational", "hidden", or "ignored" are particularly accurate given all the overlapping platform APIs that expose these nodes in different ways. I don't even think accessibilityIsIgnored is particularly accurate, but I kept that name b/c that's what WebCore calls it. A more accurate term might be heuristicallyDeterminedAsIrrelevantForAccessibility, but that's even more verbose, and "irrelevant" may be a little too vague. I'm sure a better term will present itself at some point, but I'd like to err on the side of the common terminology used in the accessibility part of WebCore.

Created attachment 223738 [details] patch with review feedback addresses remaining review feedback

(In reply to comment #27) > (In reply to comment #24) > > > > Source/WebCore/inspector/protocol/DOM.json:61 > > > + { "name": "accessibilityIsIgnored", "type": "boolean", "description": "Returns whether the DOM node is exposed to the accessibility API." }, > > > > accessibilityIsIgnored is kind of wordy given that it is in AccessibilityProperties object. This could just be "ignored" or "hidden"? > > I want to leave this one as-is for now. I don't think "presentational", "hidden", or "ignored" are particularly accurate given all the overlapping platform APIs that expose these nodes in different ways. I don't even think accessibilityIsIgnored is particularly accurate, but I kept that name b/c that's what WebCore calls it. A more accurate term might be heuristicallyDeterminedAsIrrelevantForAccessibility, but that's even more verbose, and "irrelevant" may be a little too vague. > > I'm sure a better term will present itself at some point, but I'd like to err on the side of the common terminology used in the accessibility part of WebCore. We try to lee the wording terse, yet accurate. Repeating the word "accessibility" in an object named AccessibilityProperties is redundant. Think of the protocol as API too, we try not to change it later.

(In reply to comment #27) > (In reply to comment #24) > > > > Source/WebCore/inspector/protocol/DOM.json:61 > > > + { "name": "accessibilityIsIgnored", "type": "boolean", "description": "Returns whether the DOM node is exposed to the accessibility API." }, > > > > accessibilityIsIgnored is kind of wordy given that it is in AccessibilityProperties object. This could just be "ignored" or "hidden"? > > I want to leave this one as-is for now. I don't think "presentational", "hidden", or "ignored" are particularly accurate given all the overlapping platform APIs that expose these nodes in different ways. I don't even think accessibilityIsIgnored is particularly accurate, but I kept that name b/c that's what WebCore calls it. A more accurate term might be heuristicallyDeterminedAsIrrelevantForAccessibility, but that's even more verbose, and "irrelevant" may be a little too vague. > > I'm sure a better term will present itself at some point, but I'd like to err on the side of the common terminology used in the accessibility part of WebCore. Not to pile on, but I agree with removing the "accessibility" prefix from "accessibilityIsIgnored". The context is enough. While many WebCore AccessibilityObject variables and methods do use the "accessibility" prefix as you mention, I think most are a mistake and could (should) be removed as we continue to cleanup.

(In reply to comment #30) > Not to pile on, but I agree with removing the "accessibility" prefix from "accessibilityIsIgnored". The context is enough. Okay. Final patch on its way soon.

Created attachment 223782 [details] patch with review feedback Differences in this patch: - Found one more stubbed method needed for AccessibilityUIElement::computedRoleString() in Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementAtk.mm - Review: property name changed from "accessibilityIsIgnored" to "ignored" - Review: modified the following callback mod in Source/WebInspectorUI/UserInterface/DOMNode.js accessibilityProperties: function(callback) { function accessibilityPropertiesCallback(error, accessibilityProperties) { if (!error && callback) { callback({ ignored: accessibilityProperties.ignored, role: accessibilityProperties.role, label: accessibilityProperties.label }); } } DOMAgent.getAccessibilityPropertiesForNode(this.id, accessibilityPropertiesCallback.bind(this)); },

Comment on attachment 223782 [details] patch with review feedback Clearing flags on attachment: 223782 Committed r163891: <http://trac.webkit.org/changeset/163891>

All reviewed patches have been landed. Closing bug.

Created attachment 223897 [details] build fix

I don't think the Commit Queue won't land patches on an already closed bug. I'll land this build fix for you.

Comment on attachment 223897 [details] build fix iOS Build Fix landed in: <http://trac.webkit.org/changeset/163913>

Comment on attachment 223897 [details] build fix View in context: https://bugs.webkit.org/attachment.cgi?id=223897&action=review > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:259 > -JSStringRef AccessibilityUIElement::computedRoleString() > +JSRetainPtr<JSStringRef> AccessibilityUIElement::computedRoleString() > { > // FIXME: implement > return JSStringCreateWithCharacters(0, 0); Writing it like this will result in a leak of every single one of these strings; needs an adopt since JSStringCreateWithCharacters needs to be balanced by a release.

Comment on attachment 223897 [details] build fix View in context: https://bugs.webkit.org/attachment.cgi?id=223897&action=review >> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:259 >> return JSStringCreateWithCharacters(0, 0); > > Writing it like this will result in a leak of every single one of these strings; needs an adopt since JSStringCreateWithCharacters needs to be balanced by a release. Needs to be: return adopt(JSStringCreateWithCharacters(nullptr, 0));

(In reply to comment #39) > (From update of attachment 223897 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223897&action=review > > >> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:259 > >> return JSStringCreateWithCharacters(0, 0); > > > > Writing it like this will result in a leak of every single one of these strings; needs an adopt since JSStringCreateWithCharacters needs to be balanced by a release. > > Needs to be: > > return adopt(JSStringCreateWithCharacters(nullptr, 0)); Since the entire file (and others) are done that way, I'll file another bug for that fix.

Bug 128630 - AX: AccessibilityUIElement leaks in DumpRenderTree and WebKitTestRunner