Bug 28200 - ListMarker should be included as part of the text value to parse
Summary: ListMarker should be included as part of the text value to parse
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-11 16:25 PDT by chris fleizach
Modified: 2009-08-11 17:59 PDT (History)
0 users

See Also:


Attachments
patch (17.22 KB, patch)
2009-08-11 16:35 PDT, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2009-08-11 16:25:16 PDT
If a list includes list markers, the list marker values need to be included as part of the text string.

As an example, if bullet character is at the beginning of every list item, then each one of those bullet character needs to be part of the AXValue of the AXWebArea.  And asking any of the text parsing SPIs, such as AXStringForRange, should include the bullet character.
Comment 1 chris fleizach 2009-08-11 16:35:53 PDT
Created attachment 34611 [details]
patch
Comment 2 Darin Adler 2009-08-11 17:22:41 PDT
Comment on attachment 34611 [details]
patch

> +        Node* stringNode = node;
> +        while (stringNode) {

We'd normally write this as a for loop.

           for (Node* stringNode = node; stringNode; stringNode = stringNode->parent())

> +            RenderObject* renderObject = stringNode->renderer();
> +            if (renderObject->isListItem()) {

You need to check the renderObject for 0 here. There's not an ironclad guarantee it will be non-zero. If you were walking up the render tree then there would be, but a DOM parent of a renderer might not be rendered. I am not even sure this will do the right thing in unusual circumstances, like when a list is in a floating element. DOM parents might be far away from the element in question, so walking up the DOM tree like this is not necessarily a good idea.

I think the code would read better if you factored the code to find the RenderListItem* apart from the code to extract the marker text, maybe even in a separate function. It could help make the flow clearer to not have the marker text code inside a loop.

> +    String listMarkerTextForRange(const VisiblePosition& , Node*) const;

Extra space here before the comma. Strange that this takes both a VisiblePosition and a Node*. I'm not really sure I understand the nature and relationship of the two arguments.

> +    if ([string isKindOfClass:[NSString class]])
> +        return [string createJSStringRef];
> +    return 0;

We normally prefer to have the early exit be for the failure case.

> +JSStringRef AccessibilityUIElement::stringForRange(unsigned location, unsigned length)

Typically we omit argument names if they are not used in a function. I guess we don't have that warning turned on here, but it's worth following the style for new code.

r=me
Comment 3 chris fleizach 2009-08-11 17:59:22 PDT
http://trac.webkit.org/changeset/47076