Bug 25452 - AX: Don't create addition space AXStaticText element for every bold or link tag
Summary: AX: Don't create addition space AXStaticText element for every bold or link tag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-28 11:41 PDT by chris fleizach
Modified: 2009-04-28 12:14 PDT (History)
0 users

See Also:


Attachments
patch (5.36 KB, patch)
2009-04-28 11:45 PDT, chris fleizach
darin: review-
Details | Formatted Diff | Diff
patch (9.85 KB, patch)
2009-04-28 11:51 PDT, chris fleizach
darin: review-
Details | Formatted Diff | Diff
patch (9.75 KB, patch)
2009-04-28 12:03 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-04-28 11:41:53 PDT
In the following simplified html example with two bold tag and two links, I would expect four uiElements represented thru accessibility.  Instead, I see eight uiElements.  Somehow WebKit is creating another AXStaticText that contains a space character as the AXValue after every bold or link tag.  See the screenshot for an example of one of these spacer AXStaticText.  Note that the start marker is the same as the end marker.

<b>First</b>
<b>Second</b>
<a href="http://www.apple.com">Apple</a>
<a href="http://www.yahoo.com">Yahoo</a>
Comment 1 chris fleizach 2009-04-28 11:45:57 PDT
Created attachment 29855 [details]
patch
Comment 2 Darin Adler 2009-04-28 11:51:52 PDT
Comment on attachment 29855 [details]
patch 

> +        // text elements that are just empty whitespace should not be returned
> +        String text = renderText->text()->simplifyWhiteSpace();
> +        if (text.isNull() || text.isEmpty())
> +            return true;
> +        return false;

This should just be:

    return renderText->text()->simplifyWhiteSpace().isEmpty();

There's no need for a separate isNull check since null strings are also guaranteed to be empty. But really, to be efficient, it should be:

    return renderText->text()->containsOnlyWhitespace();

Unfortunately, that function is currently defined for StringImpl* but not for String. Since RenderText guarantees that text() will never return null (you can see assertions to that effect inside the class), you can do this:

    return renderText->text()->impl()->containsOnlyWhitespace();

Iċ€¤l say review- because it would be better to do the more efficient version.
Comment 3 chris fleizach 2009-04-28 11:51:54 PDT
Created attachment 29856 [details]
patch
Comment 4 Darin Adler 2009-04-28 11:54:29 PDT
Comment on attachment 29856 [details]
patch

Same comments on this patch, but this also seems to include other seemingly-unrelated changes. Maybe fixing two bugs at once?
Comment 5 chris fleizach 2009-04-28 11:55:28 PDT
you're too quick for me. first time uploaded i forgot the DumpRenderTree changes
i obsoleted the first patch, then tried to upload again, but didn't see the comments until i had uploaded
Comment 6 chris fleizach 2009-04-28 12:03:44 PDT
Created attachment 29857 [details]
patch

updated patch based on darin's review.
Comment 7 chris fleizach 2009-04-28 12:04:10 PDT
it seems that i can use

renderText->text()->containsOnlyWhitespace();
without problem
Comment 8 Darin Adler 2009-04-28 12:10:10 PDT
Comment on attachment 29857 [details]
patch

r=me
Comment 9 chris fleizach 2009-04-28 12:14:33 PDT
http://trac.webkit.org/changeset/42946