LabelRole is currently exposed as a group, but would be more appropriate as static text if all the elements it contains are static text. This will save AT the complexity of evaluating the labels subtree and stitching the text manually.
<rdar://problem/26747020>
Created attachment 281045 [details] Patch
Comment on attachment 281045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281045&action=review > Source/WebCore/ChangeLog:8 > + Use AccessibilityLabel to represent HTMLLabelElement to AT. AT -> assistive technologies > Source/WebCore/accessibility/AccessibilityLabel.cpp:71 > + if (!child->accessibilityIsIgnored() && !child->isDetached()) { I think we're guaranteed to only have unignored children in m_children The same might be true of detached children > Source/WebCore/accessibility/AccessibilityLabel.cpp:76 > + staticText = true; Seems like you don't need to keep track of staticText. As soon as you get a failure can you can just return false Then you can return true at the end of it right Also, you could probably cache this value and then update when children are updated > Source/WebCore/accessibility/AccessibilityLabel.cpp:92 > + return WebCore::containsOnlyStaticText(m_children); This WebCore:: prefix is probably unnecessary > Source/WebCore/accessibility/AccessibilityLabel.h:41 > + AccessibilityRole roleValue() const override { return LabelRole; } Add a final to these roleValue and stringValue() can probably be private > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2237 > + if (role == LabelRole && is<AccessibilityLabel>(*m_object) && downcast<AccessibilityLabel>(*m_object).containsOnlyStaticText()) This check role == LabelRole Is probably not necessary since is<AccessibilityLabel> guarantees role = Label
Comment on attachment 281045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281045&action=review >> Source/WebCore/accessibility/AccessibilityLabel.cpp:76 >> + staticText = true; > > Seems like you don't need to keep track of staticText. As soon as you get a failure can you can just return false > > Then you can return true at the end of it right > > Also, you could probably cache this value and then update when children are updated Having bool staticText means I don't have to check if children is empty. I could switch to size == 0 at the top. Caching seemed tricky given there seem to be a lot of cases where the funnels aren't used and m_children.append() is used directly. And this method needs to be const to be called from stringValue() so calculating lazily and caching is tricky. >> Source/WebCore/accessibility/AccessibilityLabel.cpp:92 >> + return WebCore::containsOnlyStaticText(m_children); > > This WebCore:: prefix is probably unnecessary You need it to disambiguate from AccessibilityLabel::containsOnlyStaticText() (the compiler gets huffy without it) >> Source/WebCore/accessibility/AccessibilityLabel.h:41 >> + AccessibilityRole roleValue() const override { return LabelRole; } > > Add a final to these > > roleValue and stringValue() can probably be private Will do on final. I'll see about role and string. >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2237 >> + if (role == LabelRole && is<AccessibilityLabel>(*m_object) && downcast<AccessibilityLabel>(*m_object).containsOnlyStaticText()) > > This check > role == LabelRole > > Is probably not necessary since is<AccessibilityLabel> guarantees role = Label It's not strictly necessary, but it seemed likely that role == Label was cheaper than calling is<> on every m_object.
Comment on attachment 281045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281045&action=review >>> Source/WebCore/accessibility/AccessibilityLabel.cpp:76 >>> + staticText = true; >> >> Seems like you don't need to keep track of staticText. As soon as you get a failure can you can just return false >> >> Then you can return true at the end of it right >> >> Also, you could probably cache this value and then update when children are updated > > Having bool staticText means I don't have to check if children is empty. I could switch to size == 0 at the top. > > Caching seemed tricky given there seem to be a lot of cases where the funnels aren't used and m_children.append() is used directly. And this method needs to be const to be called from stringValue() so calculating lazily and caching is tricky. >> I could switch to size == 0 at the top. That seems better to me >>> Source/WebCore/accessibility/AccessibilityLabel.cpp:92 >>> + return WebCore::containsOnlyStaticText(m_children); >> >> This WebCore:: prefix is probably unnecessary > > You need it to disambiguate from AccessibilityLabel::containsOnlyStaticText() (the compiler gets huffy without it) Might be better named objectsContainOnlyStaticText or elementsContainOnlyStaticText Since children is no longer accurate, the method being static rattan than an instance
Comment on attachment 281045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281045&action=review >>>> Source/WebCore/accessibility/AccessibilityLabel.cpp:92 >>>> + return WebCore::containsOnlyStaticText(m_children); >>> >>> This WebCore:: prefix is probably unnecessary >> >> You need it to disambiguate from AccessibilityLabel::containsOnlyStaticText() (the compiler gets huffy without it) > > Might be better named > > objectsContainOnlyStaticText or elementsContainOnlyStaticText > > Since children is no longer accurate, the method being static rattan than an instance The argument type is const AccessibilityObject::AccessibilityChildrenVector& so I think children is fair to use. I'll rename is childrenContainOnlyStaticText.
Created attachment 281050 [details] Patch
Created attachment 281067 [details] Patch
Comment on attachment 281067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281067&action=review > Source/WebCore/CMakeLists.txt:1049 > + accessibility/AccessibilityLabel.cpp Do we need to all to AccessibilityAllInOne.cpp for windows builds? > Source/WebCore/accessibility/AccessibilityLabel.h:46 > + AccessibilityRole roleValue() const final { return LabelRole; } These should have the override keyword on them? Or does final take care of that
Comment on attachment 281067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281067&action=review >> Source/WebCore/CMakeLists.txt:1049 >> + accessibility/AccessibilityLabel.cpp > > Do we need to all to AccessibilityAllInOne.cpp for windows builds? I believe so. I could try submitting the patch without it. >> Source/WebCore/accessibility/AccessibilityLabel.h:46 >> + AccessibilityRole roleValue() const final { return LabelRole; } > > These should have the override keyword on them? Or does final take care of that My understanding is that final implies override.
Created attachment 281154 [details] Patch
Comment on attachment 281154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281154&action=review We should add to these tests so we check that if A) <label> element does not contain only text children we do not concat values B) when elements inside <label> change we are validating that we get the correct data > LayoutTests/accessibility/mac/label-element-with-link-string-value.html:21 > + description("This tests that if a label element contains text children it's AXValue will be their concatenated AXValues."); This description is the same as the previous test (LayoutTests/accessibility/mac/label-element-all-text-string-value.html) - seems like it should be different
Comment on attachment 281154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281154&action=review > Source/WebCore/accessibility/AccessibilityLabel.cpp:100 > + m_containsOnlyStaticTextDirty = false; Shouldn't this one be set to true (m_containsOnlyStaticTextDirty) because we've just cleared the children and we want to recalculate?
Comment on attachment 281154 [details] Patch Were any of my comments addressed?
(In reply to comment #14) > Comment on attachment 281154 [details] > Patch > > Were any of my comments addressed? Forgot to hit submit before sending it back. Fixing now.
Comment on attachment 281154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281154&action=review >> Source/WebCore/accessibility/AccessibilityLabel.cpp:100 >> + m_containsOnlyStaticTextDirty = false; > > Shouldn't this one be set to true (m_containsOnlyStaticTextDirty) because we've just cleared the children and we want to recalculate? No, it should be false because there are no children so m_containsOnlyStaticText is definitely false and we don't need to recalculate.
> We should add to these tests so we check that if > A) <label> element does not contain only text children we do not concat > values label-element-with-link-string-value.html addresses this > B) when elements inside <label> change we are validating that we get the > correct data I'll add a test for that > > > LayoutTests/accessibility/mac/label-element-with-link-string-value.html:21 > > + description("This tests that if a label element contains text children it's AXValue will be their concatenated AXValues."); > > This description is the same as the previous test > (LayoutTests/accessibility/mac/label-element-all-text-string-value.html) - > seems like it should be different Fixed in my next patch.
Created attachment 281279 [details] Patch
Comment on attachment 281279 [details] Patch Clearing flags on attachment: 281279 Committed r202063: <http://trac.webkit.org/changeset/202063>
All reviewed patches have been landed. Closing bug.