Summary: | ListMarker should be included as part of the text value to parse | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
chris fleizach
2009-08-11 16:25:16 PDT
Created attachment 34611 [details]
patch
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 |