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>
Created attachment 256081 [details] patch
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.
Created attachment 256083 [details] patch
dupe of bug 108979?
review?
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 :)
http://trac.webkit.org/changeset/186598
details-summary.html times out on all bots.
This is timing out everywhere. Can you please fix it? Or should we deactivate the test?
(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?
(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
(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
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(); }
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!
(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
(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