[webkit-reviews] review granted: [Bug 45116] AX: accessibility not returning strings when visibility is hidden : [Attachment 66390] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 45116: AX: accessibility not returning strings when visibility is hidden
https://bugs.webkit.org/show_bug.cgi?id=45116

Attachment 66390: Patch
https://bugs.webkit.org/attachment.cgi?id=66390&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66390&action=prettypatch

> WebCore/WebCore.xcodeproj/project.pbxproj:20494
> +			developmentRegion = English;
This change should not be committed.

> WebCore/editing/TextIterator.cpp:2231
> +    int behavior = (isDisplayString) ? TextIteratorDefaultBehavior:
TextIteratorEmitsTextsWithoutTranscoding;
The 'behavior' variable should be of type TextIteratorBehavior here.

No need for parenthesis around isDisplayString here.

> WebCore/editing/TextIterator.cpp:2234
> +    for (TextIterator it(r, static_cast<TextIteratorBehavior>(behavior));
!it.atEnd(); it.advance()) {
Then there is no need to cast 'behavior' to TextIteratorBehavior here.

> WebCore/editing/TextIterator.h:51
> +String plainText(const Range*, bool ignoreStyleVisibility = false);
It would be nice if this could be an enum (maybe use TextIteratorBehavior?)
here instead of just a boolean to make it easier to read the code at call sites
without referring to the argument list.  This change is not necessary to land
this patch, though.

> WebCore/editing/TextIterator.h:52
> +UChar* plainTextToMallocAllocatedBuffer(const Range*, unsigned&
bufferLength, bool isDisplayString, bool ignoreStyleVisibility = false);
Ditto.

> WebCore/editing/TextIterator.h:80
> +    TextIteratorIgnoreStyleVisibility = 1 << 4
This should be named "TextIteratorIgnoresStyleVisibility" to match the verb
tense of the other enums.

> WebCore/editing/TextIterator.h:178
> +    bool m_ignoreStyleVisibility;
This should be named "m_ignoresStyleVisibility" to match the verb tense of
other ivars.

> LayoutTests/platform/mac/accessibility/clipped-text-under-element.html:20
> +  <h1><a id="hidden-link" tabindex=0
href="http://domain.com/path/i-am-the-URL?id=1234">I am the link text.</a></h1>

Change the description to:  "I am the hidden link text."

> LayoutTests/platform/mac/accessibility/clipped-text-under-element.html:26
> +  <h1><a id="visible-link" tabindex=0
href="http://domain.com/path/i-am-the-URL?id=1234">I am the link text.</a></h1>

Change the description to:  "I am the visible link text."

> LayoutTests/platform/mac/accessibility/clipped-text-under-element.html:34
> +    description("This tests that even if an element is not visible,
textUnderElement() will still give the text");
Nit: Add period at end of the description sentence.

> LayoutTests/platform/mac/accessibility/clipped-text-under-element.html:38
> +	   var link1 = accessibilityController.focusedElement;
Use "hiddenLink" instead of "link1" here.

> LayoutTests/platform/mac/accessibility/clipped-text-under-element.html:42
> +	   var link2 = accessibilityController.focusedElement;
Use "visibleLink" instead of "link2" here.