Bug 146549 – AX: <details> element should allow expand/close through AX API

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

VoiceOver needs a way to expand/collapse (either AXPress or make expanded writable) this group along with a notification when it is expaneded or collapsed (probably ValueChanged)

<rdar://problem/21636810>

[chris fleizach]

Created attachment 256081 [details]
patch

[WebKit Commit Bot]

Attachment 256081 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3099:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3099:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 256083 [details]
patch

[James Craig]

dupe of bug 108979?

[chris fleizach]

review?

[Mario Sanchez Prada]

Comment on attachment 256083 [details]
patch

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

Looks good to me, just a minor comment below for consideration before landing

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1617
> +        if ((expand && !downcast<HTMLDetailsElement>(node())->isOpen())
> +            || (!expand && downcast<HTMLDetailsElement>(node())->isOpen()))
> +            downcast<HTMLDetailsElement>(node())->toggleOpen();

Maybe you can put downcast<HTMLDetailsElement>(node()) in a variable so that you don't have to repeat that three times. Or just downcast<HTMLDetailsElement>(node())->isOpen(). Or both :)

[chris fleizach]

http://trac.webkit.org/changeset/186598

[Alex Christensen]

details-summary.html times out on all bots.

[Brent Fulgham]

This is timing out everywhere. Can you please fix it? Or should we deactivate the test?

[Mario Sanchez Prada]

(In reply to comment #8)
> details-summary.html times out on all bots.

Perhaps there's an issue with the notifications and finishJSTest() is never called? Maybe some missing bits in WKTR, Chris?

[chris fleizach]

(In reply to comment #10)
> (In reply to comment #8)
> > details-summary.html times out on all bots.
> 
> Perhaps there's an issue with the notifications and finishJSTest() is never
> called? Maybe some missing bits in WKTR, Chris?

Build bot ran it fine, so i'm looking into it now

[chris fleizach]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > details-summary.html times out on all bots.
> > 
> > Perhaps there's an issue with the notifications and finishJSTest() is never
> > called? Maybe some missing bits in WKTR, Chris?
> 
> Build bot ran it fine, so i'm looking into it now

Forgot a file

http://trac.webkit.org/changeset/186650

[Daniel Bates]

Comment on attachment 256083 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1617
>> +            downcast<HTMLDetailsElement>(node())->toggleOpen();
> 
> Maybe you can put downcast<HTMLDetailsElement>(node()) in a variable so that you don't have to repeat that three times. Or just downcast<HTMLDetailsElement>(node())->isOpen(). Or both :)

I would have written this as:

if (is<HTMLDetailsElement>(node())) {
    auto& details = downcast<HTMLDetailsElement>(*node());
    if (expand != details.isOpen())
        details.toggleOpen();
}

[Darin Adler]

Comment on attachment 256083 [details]
patch

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

>>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1617
>>> +            downcast<HTMLDetailsElement>(node())->toggleOpen();
>> 
>> Maybe you can put downcast<HTMLDetailsElement>(node()) in a variable so that you don't have to repeat that three times. Or just downcast<HTMLDetailsElement>(node())->isOpen(). Or both :)
> 
> I would have written this as:
> 
> if (is<HTMLDetailsElement>(node())) {
>     auto& details = downcast<HTMLDetailsElement>(*node());
>     if (expand != details.isOpen())
>         details.toggleOpen();
> }

Yes, we should update it to be that!

[chris fleizach]

(In reply to comment #14)
> Comment on attachment 256083 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256083&action=review
> 
> >>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1617
> >>> +            downcast<HTMLDetailsElement>(node())->toggleOpen();
> >> 
> >> Maybe you can put downcast<HTMLDetailsElement>(node()) in a variable so that you don't have to repeat that three times. Or just downcast<HTMLDetailsElement>(node())->isOpen(). Or both :)
> > 
> > I would have written this as:
> > 
> > if (is<HTMLDetailsElement>(node())) {
> >     auto& details = downcast<HTMLDetailsElement>(*node());
> >     if (expand != details.isOpen())
> >         details.toggleOpen();
> > }
> 
> Yes, we should update it to be that!

On top of it. Thanks for the comments

[chris fleizach]

(In reply to comment #15)
> (In reply to comment #14)
> > Comment on attachment 256083 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=256083&action=review
> > 
> > >>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1617
> > >>> +            downcast<HTMLDetailsElement>(node())->toggleOpen();
> > >> 
> > >> Maybe you can put downcast<HTMLDetailsElement>(node()) in a variable so that you don't have to repeat that three times. Or just downcast<HTMLDetailsElement>(node())->isOpen(). Or both :)
> > > 
> > > I would have written this as:
> > > 
> > > if (is<HTMLDetailsElement>(node())) {
> > >     auto& details = downcast<HTMLDetailsElement>(*node());
> > >     if (expand != details.isOpen())
> > >         details.toggleOpen();
> > > }
> > 
> > Yes, we should update it to be that!
> 
> On top of it. Thanks for the comments

http://trac.webkit.org/changeset/186679