AX: aria-level does not override implicit level on h1, h2, etc

[James Craig]

Change will be in AccessibilityNodeObject::headingLevel.
Update commented lines of heading-level layout test at the same time.

[Radar WebKit Bug Importer]

<rdar://problem/13665073>

[James Craig]

Created attachment 198479 [details]
patch

[chris fleizach]

Comment on attachment 198479 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=198479&action=review

> Source/WebCore/ChangeLog:3
> +        aria-level does not override implicit level on h1, h2, etc

We should prefix accessibility bugs with AX: so it's easier to know what aspect of the code their referring to

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:719
> +    if (isHeading() && getAttribute(aria_levelAttr).toInt() > 0)

You should cache the value of getAttribute rather than call it two times

[James Craig]

(In reply to comment #3)
> We should prefix accessibility bugs with AX: so it's easier to know what aspect…

Is this concern only for the ChangeLog comments? I hadn't been prefixing the bug titles b/c they're all filed in the "Accessibility" component.

> You should cache the value of getAttribute rather than call it two times

Okay, thanks.

[chris fleizach]

(In reply to comment #4)
> (In reply to comment #3)
> > We should prefix accessibility bugs with AX: so it's easier to know what aspect…
> 
> Is this concern only for the ChangeLog comments? I hadn't been prefixing the bug titles b/c they're all filed in the "Accessibility" component.

My preference is that all Accessibility bugs have AX: in the title. The category of the bug isn't transferred to change logs, commit logs, etc, etc

> 
> > You should cache the value of getAttribute rather than call it two times
> 
> Okay, thanks.

[James Craig]

Created attachment 198581 [details]
patch

[chris fleizach]

Comment on attachment 198581 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=198581&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:720
> +    if (isHeading() && ariaLevel > 0)

we should probably do
if (isHeading()) {
    int ariaLevel = getAttribute(aria_levelAttr).toInt();
    if (ariaLevel > 0)
        return ariaLevel;
}

so that every object doesn't have to make that getAttribute call

[James Craig]

Created attachment 198589 [details]
patch with review feedback

[chris fleizach]

Comment on attachment 198589 [details]
patch with review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=198589&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:721
> +        if (ariaLevel > 0) {

brackets not needed for a one line if (this will fail style-bot)

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:743
> +    

this extra insertion should probably be removed to reduce churn

[James Craig]

Created attachment 198606 [details]
patch with review feedback

[WebKit Commit Bot]

Comment on attachment 198606 [details]
patch with review feedback

Clearing flags on attachment: 198606

Committed r148654: <http://trac.webkit.org/changeset/148654>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.