Bug 268523 – AX: The value of date time fields is not read by VoiceOver when the VO cursor lands on them.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

.

[Radar WebKit Bug Importer]

<rdar://problem/122059024>

[Andres Gonzalez]

Created attachment 469642 [details]
Patch

[Andres Gonzalez]

Created attachment 469650 [details]
Patch

[Tyler Wilcock]

Comment on attachment 469650 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilitySpinButton.h:72
> +    bool m_isIncrementor : true;

I think the colon syntax is for bitfields. Do we want to keep it, or use the more standard:

bool m_isIncrementor { true }

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1004
> -        
> -    if (self.axBackingObject->isNonNativeTextControl())
> -        return true;
>  
> -    // Links can sometimes be elements (when they only contain static text or don't contain anything).
> -    // They should not be elements when containing text and other types.
> +        if (backingObject->isNonNativeTextControl())
> +            return true;
> +
> +        // Links can sometimes be elements (when they only contain static text or don't contain anything).
> +        // They should not be elements when containing text and other types.

I'm confused by this, both before and after your patch. Is this an if-statement wedged between switch case statements that doesn't belong to any case? If so, probably the indentation should at least match the cases (this patch moves indents it forwards one level) because it's some kind of ad-hoc "case". But seems odd in any case, unless this is standard practice I'm unaware of.

[Andres Gonzalez]

Created attachment 469658 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 469650 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=469650&action=review
> 
> > Source/WebCore/accessibility/AccessibilitySpinButton.h:72
> > +    bool m_isIncrementor : true;
> 
> I think the colon syntax is for bitfields. Do we want to keep it, or use the
> more standard:
> 
> bool m_isIncrementor { true }

AG: Fixed.
> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1004
> > -        
> > -    if (self.axBackingObject->isNonNativeTextControl())
> > -        return true;
> >  
> > -    // Links can sometimes be elements (when they only contain static text or don't contain anything).
> > -    // They should not be elements when containing text and other types.
> > +        if (backingObject->isNonNativeTextControl())
> > +            return true;
> > +
> > +        // Links can sometimes be elements (when they only contain static text or don't contain anything).
> > +        // They should not be elements when containing text and other types.
> 
> I'm confused by this, both before and after your patch. Is this an
> if-statement wedged between switch case statements that doesn't belong to
> any case? If so, probably the indentation should at least match the cases
> (this patch moves indents it forwards one level) because it's some kind of
> ad-hoc "case". But seems odd in any case, unless this is standard practice
> I'm unaware of.

AG: thanks for catching that. Removed the dead code and put the comment in the right place.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #6)
> (In reply to Tyler Wilcock from comment #4)
> > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1004
> > > -        
> > > -    if (self.axBackingObject->isNonNativeTextControl())
> > > -        return true;
> > >  
> > > -    // Links can sometimes be elements (when they only contain static text or don't contain anything).
> > > -    // They should not be elements when containing text and other types.
> > > +        if (backingObject->isNonNativeTextControl())
> > > +            return true;
> > > +
> > > +        // Links can sometimes be elements (when they only contain static text or don't contain anything).
> > > +        // They should not be elements when containing text and other types.
> > 
> > I'm confused by this, both before and after your patch. Is this an
> > if-statement wedged between switch case statements that doesn't belong to
> > any case? If so, probably the indentation should at least match the cases
> > (this patch moves indents it forwards one level) because it's some kind of
> > ad-hoc "case". But seems odd in any case, unless this is standard practice
> > I'm unaware of.
> 
> AG: thanks for catching that. Removed the dead code and put the comment in
> the right place.
TW: Did the if-statement have no effect? Or is removing it a behavior change? I'm not sure how this code would behave, never seen something like it before.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #7)
> (In reply to Andres Gonzalez from comment #6)
> > (In reply to Tyler Wilcock from comment #4)
> > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1004
> > > > -        
> > > > -    if (self.axBackingObject->isNonNativeTextControl())
> > > > -        return true;
> > > >  
> > > > -    // Links can sometimes be elements (when they only contain static text or don't contain anything).
> > > > -    // They should not be elements when containing text and other types.
> > > > +        if (backingObject->isNonNativeTextControl())
> > > > +            return true;
> > > > +
> > > > +        // Links can sometimes be elements (when they only contain static text or don't contain anything).
> > > > +        // They should not be elements when containing text and other types.
> > > 
> > > I'm confused by this, both before and after your patch. Is this an
> > > if-statement wedged between switch case statements that doesn't belong to
> > > any case? If so, probably the indentation should at least match the cases
> > > (this patch moves indents it forwards one level) because it's some kind of
> > > ad-hoc "case". But seems odd in any case, unless this is standard practice
> > > I'm unaware of.
> > 
> > AG: thanks for catching that. Removed the dead code and put the comment in
> > the right place.
> TW: Did the if-statement have no effect? Or is removing it a behavior
> change? I'm not sure how this code would behave, never seen something like
> it before.

That if statement wasn't ever executed because the previous line is a return. Doesn't matter that is inside a case block.

[Andres Gonzalez]

Created attachment 469666 [details]
Patch

[Andres Gonzalez]

Created attachment 469674 [details]
Patch

[Andres Gonzalez]

Created attachment 469692 [details]
Patch

[EWS]

Committed 274057@main (7c62441425c1): <https://commits.webkit.org/274057@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 469692 [details].