Summary: | Expose the aria-label attribute for <video> elements. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andres Gonzalez <andresg_22> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cfleizach, commit-queue, darin, haleykyzer123, repstein | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Andres Gonzalez
2019-07-26 11:36:57 PDT
Created attachment 374979 [details]
Patch
Comment on attachment 374979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374979&action=review Not sure I’m a qualified reviewer for this. But I do have some comments. > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:689 > + // Convey the video object as interactive if it is not auto-playing. This comment says "if it is not auto-playing" but that’s not what the code below does. The code checks if autoplay is allowed, not if it’s actually actively happening. > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:691 > + auto* mediaObject = downcast<AccessibilityMediaObject>(m_object); > + return !mediaObject->isAutoplayEnabled(); Now that it gets this simple, we’d normally write it like this: return !downcast<AccessibilityMediaObject>(*m_object).isAutoplayEnabled(); I don’t think the two line version is better. Created attachment 374984 [details]
Patch
(In reply to Darin Adler from comment #3) > Comment on attachment 374979 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374979&action=review > > Not sure I’m a qualified reviewer for this. But I do have some comments. > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:689 > > + // Convey the video object as interactive if it is not auto-playing. > > This comment says "if it is not auto-playing" but that’s not what the code > below does. The code checks if autoplay is allowed, not if it’s actually > actively happening. > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:691 > > + auto* mediaObject = downcast<AccessibilityMediaObject>(m_object); > > + return !mediaObject->isAutoplayEnabled(); > > Now that it gets this simple, we’d normally write it like this: > > return > !downcast<AccessibilityMediaObject>(*m_object).isAutoplayEnabled(); > > I don’t think the two line version is better. Thanks Darin, both good points. fixed. Comment on attachment 374984 [details] Patch Clearing flags on attachment: 374984 Committed r247891: <https://trac.webkit.org/changeset/247891> All reviewed patches have been landed. Closing bug. (In reply to Andres Gonzalez from comment #5) > (In reply to Darin Adler from comment #3) > > Comment on attachment 374979 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=374979&action=review > > > > Not sure I’m a qualified reviewer for this. But I do have some comments. > > > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:689 > > > + // Convey the video object as interactive if it is not auto-playing. > > > > This comment says "if it is not auto-playing" but that’s not what the code > > below does. The code checks if autoplay is allowed, not if it’s actually > > actively happening. > > > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:691 > > > + auto* mediaObject = downcast<AccessibilityMediaObject>(m_object); > > > + return !mediaObject->isAutoplayEnabled(); > > > > Now that it gets this simple, we’d normally write it like this: > > > > return > > !downcast<AccessibilityMediaObject>(*m_object).isAutoplayEnabled(); > > > > I don’t think the two line version is better. > > Thanks Darin, both good points. fixed. It appears that this patch caused accessibility/ios-simulator/video-elements-ios.html to fail. This was not caught by EWS because the test has a test expectation, [ Pass Timeout ]. https://bugs.webkit.org/show_bug.cgi?id=200231 |