718627 - [Mac] Navigating by character, or interacting with, the text in the awesome bar does not speak the character.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Run the try- build from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b2c705a32cf8/
2. Press Cmd+L to go to the Awesome bar.
3. Type in something.
4. Press LeftArrow to try and review it.

Result: VoiceOver will remain silent.
Expected: VoiceOver would speak the characters in backwards order.

Comment 1

[Marco Zehe (:MarcoZ)]

Without this, users are not able to enter anything into text fields in a controlled fashion. They can only do blind flying.

Comment 2

[Hubert Figuiere [:hub]]

Apparently we don't return the the selected text range (which is just the insertion point when there is a selection length of 0).

I need to investigate this further.

Comment 3

[Hubert Figuiere [:hub]]

Attachment: Implement text edit accessibility. r=

Comment 4

[Hubert Figuiere [:hub]]

This patch isn't ready for review. It needs fixing and cleanup.

Comment 5

[Hubert Figuiere [:hub]]

Attachment: Implement text edit accessibility and fix the URL location bar. r=

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #5)
> Created attachment 604585 [details] [diff] [review]
> Implement text edit accessibility and fix the URL location bar. r=

ready for review?

Comment 7

[Hubert Figuiere [:hub]]

not yet. Still some tricky issues.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Let me know if you need me to reach out to Apple/VO.

Comment 9

[Hubert Figuiere [:hub]]

Attachment: Part 1: Implement text edit accessibility and fix the URL location bar. r=

Comment 10

[Hubert Figuiere [:hub]]

Comment on attachment 607183 [details] [diff] [review]
Part 1: Implement text edit accessibility and fix the URL location bar. r=

This is part 1. Implement most of the protocol needed to support text navigation. Still doesn't notify when moving the caret with the arrow keys, but things are better than last time.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 607183 [details] [diff] [review]
Part 1: Implement text edit accessibility and fix the URL location bar. r=

Review of attachment 607183 [details] [diff] [review]:
-----------------------------------------------------------------

honestly I don't have a clever idea what you implement here, it looks like a working draft of big novel. I would appreciate if you can split this work into feature complete pieces with short description what is that for. In either case you just implement an API so it should be easy to do and it'll be much easier to review especially for those who doesn't have a good knowledge of NSAccessibility protocol.

r- since it sounds too messy

::: accessible/src/html/nsHyperTextAccessible.h
@@ +294,5 @@
> +                          nsAString *aText = nsnull,
> +                          nsIFrame **aEndFrame = nsnull,
> +                          nsIntRect *aBoundsRect = nsnull,
> +                          nsAccessible **aStartAcc = nsnull,
> +                          nsAccessible **aEndAcc = nsnull);

nit: type* name

@@ +301,5 @@
> +   * Provide the line number for the caret, relative to the
> +   * current DOM node.
> +   * @return 1-based index for the line number with the caret
> +   */
> +  PRInt32 GetCaretLineNumber();

-> CaretLineCount() I think since we use count over number and no Get

::: accessible/src/mac/mozAccessible.h
@@ +44,5 @@
>  
>  @class mozRootAccessible;
>  
> +/**
> + * all mozAccessibles are either abstract objects (that correspond to XUL

all -> All

@@ +47,5 @@
> +/**
> + * all mozAccessibles are either abstract objects (that correspond to XUL
> + * widgets, HTML frames, etc) or are attached to a certain view; for example
> + * a document view. when we hand an object off to an AT, we always want
> + * to give it the represented view, in the latter case.

would be nice to reword it, otherwise what the method is supposed for gets clear when you read till the end

@@ +50,5 @@
> + * a document view. when we hand an object off to an AT, we always want
> + * to give it the represented view, in the latter case.
> + */
> +id <mozAccessible>
> +GetObjectOrRepresentedView(id <mozAccessible> anObject);

shouldn't you move its body to make it truly inline?

::: accessible/src/mac/mozAccessible.mm
@@ +427,5 @@
>    for (; index < totalCount; index++) {
>      nsAccessibleWrap *curAccessible = childrenArray.ElementAt(index);
>      if (curAccessible) {
>        mozAccessible *curNative = GetNativeFromGeckoAccessible(curAccessible);
> +      if (curNative && ![curNative accessibilityIsIgnored])

childrenArray shouldn't contain ignored accessibles since GetUnignoredChildren takes care of this

::: accessible/src/mac/mozTextAccessible.h
@@ -17,5 @@
> -// equivalent to pressing return key in this textfield.
> -- (void)confirm;
> -
> -// shows the menu for this combobox.
> -- (void)showMenu;

why do you remove these?

::: accessible/src/mac/mozTextAccessible.mm
@@ +92,5 @@
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
>  
> +  if ([attribute isEqualToString:NSAccessibilityVisitedAttribute])
> +    return [NSNumber numberWithBool:NO];

this is a link implementation aka traversed state. Why do you need it for autocompletes?

@@ +126,5 @@
> +  if (!supportedParametrizedAttributes) {
> +    // standard attributes that are shared and supported by all generic elements.
> +    supportedParametrizedAttributes = [[NSArray alloc] 
> +					initWithObjects:NSAccessibilityStringForRangeParameterizedAttribute,
> +					kAXLineForIndexParameterizedAttribute,

sounds like wrong indentation

@@ +127,5 @@
> +    // standard attributes that are shared and supported by all generic elements.
> +    supportedParametrizedAttributes = [[NSArray alloc] 
> +					initWithObjects:NSAccessibilityStringForRangeParameterizedAttribute,
> +					kAXLineForIndexParameterizedAttribute,
> +					kAXRangeForLineParameterizedAttribute,

where these kAX goes from, why don't you use constants from http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Protocols/NSAccessibility_Protocol/Reference/Reference.html#//apple_ref/doc/uid/20000945-11523

@@ +175,5 @@
> +      return @"";
> +    }
> +
> +    return [self stringFromRange:&range];
> +  } else if ([attribute isEqualToString:(NSString*)kAXRangeForLineParameterizedAttribute])

else if after return doesn't make sense and hard to read

@@ +204,5 @@
> +    
> +    nsIntRect bounds;
> +    PRInt32 start = range.location;
> +    PRInt32 end = start + range.length;
> +    mGeckoTextAccessible->GetPosAndText(start, end, nsnull, nsnull, &bounds);

you need provide a shortcut for GetPosAndText to get boundaries instead of making GetPosAndText public

@@ +221,5 @@
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
>  
>    if ([attribute isEqualToString:NSAccessibilityValueAttribute])
>      return [self isReadOnly];
> +  if ([attribute isEqualToString: NSAccessibilitySelectedTextAttribute] ||

you don't implement it but claims you do

@@ +256,5 @@
> +    if (!rangeSet || !mGeckoTextAccessible)
> +      return;
> +
> +    nsresult rv = mGeckoTextAccessible->SetSelectionBounds(0, range.location, 
> +							     range.location + range.length);

wrong indentation

@@ +264,5 @@
> +    if (!rangeSet || !mGeckoTextAccessible)
> +      return;
> +
> +    mGeckoTextAccessible->ScrollSubstringTo(range.location, range.location + range.length,
> +					    nsIAccessibleScrollType::SCROLL_TYPE_TOP_EDGE);

sort of funny that NSAccessibilityVisibleCharacterRangeAttribute setter and getter are different things. VisibleCharacterRangeAttribute setter makes the given range visible. VisibleCharacterRangeAttribute getter just removes the characters number.

@@ +292,5 @@
>  #pragma mark -
>  
> +- (BOOL)canBeFocused
> +{
> +  return YES;

if I don't miss anything then it's generic mozTextAccessible which is not focusable in general

@@ +386,5 @@
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
>  
>    if (mGeckoTextAccessible) {
> +    PRInt32 start, end, count;
> +    start = end = count = 0;

makes sense to initialize on construction time

@@ +393,5 @@
> +    NS_ENSURE_SUCCESS(rv, nil);
> +
> +    if (count) {
> +      rv = mGeckoTextAccessible->GetSelectionBounds(0, &start, &end);
> +      NS_ENSURE_SUCCESS(rv, nil);

it appears you can try GetSelectionBounds(0) without selection count

@@ +412,5 @@
> +- (NSValue*)visibleCharacterRange
> +{
> +  return [NSValue valueWithRange:
> +		    NSMakeRange(0, mGeckoTextAccessible ? 
> +				mGeckoTextAccessible->CharacterCount() : 0)];

NSAccessibilityVisibleCharacterRangeAttribute doesn't look like it expects to include lines out of visible area so this code is likely incorrect for textareas

btw, something weird with indentation

@@ +435,5 @@
> +				  NSAccessibilitySelectedTextChangedNotification);
> +  NSAccessibilityPostNotification(obj,
> +				  NSAccessibilitySelectedRowsChangedNotification);
> +  NSAccessibilityPostNotification(obj,
> +				  NSAccessibilitySelectedColumnsChangedNotification);

two last things are for tables/trees (at least per names and chromimum code)

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +202,5 @@
>        [nativeAcc valueDidChange];
>        break;
> +    case nsIAccessibleEvent::EVENT_TEXT_CARET_MOVED:
> +    case nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED:
> +      [nativeAcc selectedTextDidChange];

don't mac a11y object supports downcasting, 'cause I'd prefer to define methods on the object when they make sense for it

Comment 12

[Hubert Figuiere [:hub]]

Comment on attachment 607183 [details] [diff] [review]
Part 1: Implement text edit accessibility and fix the URL location bar. r=

Review of attachment 607183 [details] [diff] [review]:
-----------------------------------------------------------------

I'll take these comments into account and try to split the patch in two.

::: accessible/src/html/nsHyperTextAccessible.h
@@ +294,5 @@
> +                          nsAString *aText = nsnull,
> +                          nsIFrame **aEndFrame = nsnull,
> +                          nsIntRect *aBoundsRect = nsnull,
> +                          nsAccessible **aStartAcc = nsnull,
> +                          nsAccessible **aEndAcc = nsnull);

argh. forgot that when I moved it.

@@ +301,5 @@
> +   * Provide the line number for the caret, relative to the
> +   * current DOM node.
> +   * @return 1-based index for the line number with the caret
> +   */
> +  PRInt32 GetCaretLineNumber();

->CaretLineNumber() as it is the line number we are on. Not count.

::: accessible/src/mac/mozAccessible.h
@@ +50,5 @@
> + * a document view. when we hand an object off to an AT, we always want
> + * to give it the represented view, in the latter case.
> + */
> +id <mozAccessible>
> +GetObjectOrRepresentedView(id <mozAccessible> anObject);

I don't think it is warranted.

::: accessible/src/mac/mozAccessible.mm
@@ +427,5 @@
>    for (; index < totalCount; index++) {
>      nsAccessibleWrap *curAccessible = childrenArray.ElementAt(index);
>      if (curAccessible) {
>        mozAccessible *curNative = GetNativeFromGeckoAccessible(curAccessible);
> +      if (curNative && ![curNative accessibilityIsIgnored])

oops. I was meaning to remove this change when I cleaned up.

::: accessible/src/mac/mozTextAccessible.h
@@ -17,5 @@
> -// equivalent to pressing return key in this textfield.
> -- (void)confirm;
> -
> -// shows the menu for this combobox.
> -- (void)showMenu;

I removed that type of accessible. See the change in roles.

::: accessible/src/mac/mozTextAccessible.mm
@@ +92,5 @@
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
>  
> +  if ([attribute isEqualToString:NSAccessibilityVisitedAttribute])
> +    return [NSNumber numberWithBool:NO];

they are more than just autocompletes. the Mac accessibility stuff expects it too.

@@ +126,5 @@
> +  if (!supportedParametrizedAttributes) {
> +    // standard attributes that are shared and supported by all generic elements.
> +    supportedParametrizedAttributes = [[NSArray alloc] 
> +					initWithObjects:NSAccessibilityStringForRangeParameterizedAttribute,
> +					kAXLineForIndexParameterizedAttribute,

The lack of modelines... Adding modelines for Emacs and the indentation will be fixed.

@@ +127,5 @@
> +    // standard attributes that are shared and supported by all generic elements.
> +    supportedParametrizedAttributes = [[NSArray alloc] 
> +					initWithObjects:NSAccessibilityStringForRangeParameterizedAttribute,
> +					kAXLineForIndexParameterizedAttribute,
> +					kAXRangeForLineParameterizedAttribute,

because they are in the CoreFoundation side of the API. Welcome the mess.

@@ +175,5 @@
> +      return @"";
> +    }
> +
> +    return [self stringFromRange:&range];
> +  } else if ([attribute isEqualToString:(NSString*)kAXRangeForLineParameterizedAttribute])

k

@@ +221,5 @@
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
>  
>    if ([attribute isEqualToString:NSAccessibilityValueAttribute])
>      return [self isReadOnly];
> +  if ([attribute isEqualToString: NSAccessibilitySelectedTextAttribute] ||

ok, then I'll add that implementation I have in a different patch.

@@ +412,5 @@
> +- (NSValue*)visibleCharacterRange
> +{
> +  return [NSValue valueWithRange:
> +		    NSMakeRange(0, mGeckoTextAccessible ? 
> +				mGeckoTextAccessible->CharacterCount() : 0)];

That's why this is part 1 :-) I'll add a todo mention instead.

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +202,5 @@
>        [nativeAcc valueDidChange];
>        break;
> +    case nsIAccessibleEvent::EVENT_TEXT_CARET_MOVED:
> +    case nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED:
> +      [nativeAcc selectedTextDidChange];

I could check the type and call the method if needed, but this is actually more efficient.

Comment 13

[Hubert Figuiere [:hub]]

Attachment: Part 1: Autocomplete is to be ignored. Use TextField directly.

Comment 14

[Hubert Figuiere [:hub]]

Comment on attachment 607734 [details] [diff] [review]
Part 1: Autocomplete is to be ignored. Use TextField directly.

This is the new part 1. Split from the previous patch. With all the feedback taken into account.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #12)

> ::: accessible/src/mac/mozTextAccessible.h
> @@ -17,5 @@
> > -// equivalent to pressing return key in this textfield.
> > -- (void)confirm;
> > -
> > -// shows the menu for this combobox.
> > -- (void)showMenu;
> 
> I removed that type of accessible. See the change in roles.

right, that's what I see but I don't understand the nature of the change. Why we don't need comboboxes any more or why comboobxes don't need to expose combobox specific actions like showMenu?

Comment 16

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #15)

> right, that's what I see but I don't understand the nature of the change.
> Why we don't need comboboxes any more or why comboobxes don't need to expose
> combobox specific actions like showMenu?

They have, like the URL bar, a specific button for that.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #12)

> > +    case nsIAccessibleEvent::EVENT_TEXT_CARET_MOVED:
> > +    case nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED:
> > +      [nativeAcc selectedTextDidChange];
> 
> I could check the type and call the method if needed, but this is actually
> more efficient.

or you could have hypertextwrap instead to handle these events

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #16)
> (In reply to alexander :surkov from comment #15)
> 
> > right, that's what I see but I don't understand the nature of the change.
> > Why we don't need comboboxes any more or why comboobxes don't need to expose
> > combobox specific actions like showMenu?
> 
> They have, like the URL bar, a specific button for that.

it sounds like webcore still provides it for something http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm?rev=82687#L2322

Comment 19

[Hubert Figuiere [:hub]]

> it sounds like webcore still provides it for something
> http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/mac/
> AccessibilityObjectWrapper.mm?rev=82687#L2322

We have NSAccessibilityPopUpButtonRole for that which is already dealt with.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #19)
> > it sounds like webcore still provides it for something
> > http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/mac/
> > AccessibilityObjectWrapper.mm?rev=82687#L2322
> 
> We have NSAccessibilityPopUpButtonRole for that which is already dealt with.

this should be ok for ordinal comboboxes like HTML select, what about editable comboboxes and different kinds of autocomplete.

anyway I need a good reason why approach taken in bug 362079 is incorrect and bug 363697 is partially invalid.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #12)

> ->CaretLineNumber() as it is the line number we are on. Not count.

I see

> ::: accessible/src/mac/mozAccessible.h
> @@ +50,5 @@
> > + * a document view. when we hand an object off to an AT, we always want
> > + * to give it the represented view, in the latter case.
> > + */
> > +id <mozAccessible>
> > +GetObjectOrRepresentedView(id <mozAccessible> anObject);
> 
> I don't think it is warranted.

I didn't catch.

> ::: accessible/src/mac/mozTextAccessible.mm
> @@ +92,5 @@
> >  {
> >    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
> >  
> > +  if ([attribute isEqualToString:NSAccessibilityVisitedAttribute])
> > +    return [NSNumber numberWithBool:NO];
> 
> they are more than just autocompletes. the Mac accessibility stuff expects
> it too.

I realize that's this class is not for autocompletes only. But does Mac needs this attribute implemented on any object or only for some objects having specific role? For example, if it's applicable to links then it sounds we need to do that for links only.

Comment 22

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #20)
> (In reply to Hub Figuiere [:hub] from comment #19)
> > > it sounds like webcore still provides it for something
> > > http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/mac/
> > > AccessibilityObjectWrapper.mm?rev=82687#L2322
> > 
> > We have NSAccessibilityPopUpButtonRole for that which is already dealt with.
> 
> this should be ok for ordinal comboboxes like HTML select, what about
> editable comboboxes and different kinds of autocomplete.

The problem is that the current implemetation made AUTOCOMPLETE a combox box that would contain, amongst other things the text editable. Therefor it wouldn't be reachable. That's why it is needed

Here is how it appears before the change.

      AXToolbar - "Navigation Toolbar"
        AXComboBox - "Go to a Website"
          AXButton - "Location"
          AXTextField - "Go to a Website"
          AXMenu
          AXButton - "Edit this bookmark"
          AXButton - "Location"
          AXButton - "Reload current page"

After the change, it is 

      AXToolbar - "Navigation Toolbar"
        AXButton - "Location"
        AXTextField - "Go to a Website"
        AXMenu
        AXButton - "Edit this bookmark"
        AXButton - "Location"
        AXButton - "Reload current page"

> anyway I need a good reason why approach taken in bug 362079 is incorrect
> and bug 363697 is partially invalid.

I think the whole approach has to be rethought. :-/

All I can say is that it works better with my changes.

Comment 23

[alexander :surkov (:asurkov)]

ok, what about HTML autocompletes?

Comment 24

[Hubert Figuiere [:hub]]

This open a can of worm. 

I tested with the example at http://public.yahoo.com/kloots/aria/autocomplete.html

The autocomplete actually has a role of ComboBox but the object is represented by an AXPopupButton instead of an AXComboBox (in Safari).

I believe this is just a minor change.

Using the XUL autocomplete test, with the patch we get accessible AXEditTextField, like for the URL bar. I believe this is the right way, even that lead to rethink the autocomplete interaction the way it should be.

Comment 25

[Hubert Figuiere [:hub]]

Attachment: Part 1: Autocomplete is to be ignored. Use TextField directly. ComboBox are not PopupMenuButton.

Comment 26

[Hubert Figuiere [:hub]]

Comment on attachment 608114 [details] [diff] [review]
Part 1: Autocomplete is to be ignored. Use TextField directly. ComboBox are not PopupMenuButton.

this has been tested with the example in comment 24 as well.

Comment 27

[Hubert Figuiere [:hub]]

Attachment: Part 2: implement the text protocol for text accessible.

Comment 28

[Hubert Figuiere [:hub]]

Comment on attachment 608115 [details] [diff] [review]
Part 2: implement the text protocol for text accessible.

This is part 2 of the patch.

Comment 29

[Hubert Figuiere [:hub]]

Attachment: Part 3: fix the missing implementations of the text protocol. r=

Comment 30

[Hubert Figuiere [:hub]]

Comment on attachment 608149 [details] [diff] [review]
Part 3: fix the missing implementations of the text protocol. r=

Some know missing bits from Part 2.

I'm spinning up a new build for Marco.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #26)
> Comment on attachment 608114 [details] [diff] [review]
> Part 1: Autocomplete is to be ignored. Use TextField directly. ComboBox are
> not PopupMenuButton.
> 
> this has been tested with the example in comment 24 as well.

I still don't understand. Safari exposes combobxes as popupbuttons and provides press and showmenu actions. HTML5 autocompletes (like input@type="email") are exposed as textfields and provides showmenu action. I'm not sure how to invoke an action so that showmenu action might be just a context menu available for every element. No way to try xul:menulist@editable but that's something you should keep in mind as well. ARIA widgets are special thing. Usually they wrap textfield (and sometimes a button for menu) by combobox or autocomplete widget, similarly to xul:menulist@editable.

In either case it doesn't look valid that you make comboboxes and autocompletes fall into generic accessible class. Could you please go case by case and figure out hierarchies for each type? I just concern that you could be removing the code that we need.

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 608115 [details] [diff] [review]
Part 2: implement the text protocol for text accessible.

Review of attachment 608115 [details] [diff] [review]:
-----------------------------------------------------------------

it'd be really great if you can split this patch into few ones and file bugs for that (for example extract parametrized attributes logic). I have many questions and many nits for each round, this bug gets bloat and it's harder to see was the concern addressed or not. We deal here with comboboxes, hierarchies, text implementation, core changes -- too much for one bug.

::: accessible/src/html/nsHyperTextAccessible.h
@@ +266,5 @@
>  
> +  /**
> +   * Get the bounds of the text between start and end.
> +   */
> +  void GetTextBounds(PRInt32 start, PRInt32 end, nsIntRect* bounds)

nsIntRect GetTextBounds(PRint32 aStartOffset, PRInt32 aEndOffset);

@@ +273,5 @@
> +  }
> +
> +  /**
> +   * Provide the line number for the caret, relative to the
> +   * current DOM node.

not sure what "relative to the current DOM node" means. But you could just say "Return the line number for the caret"

::: accessible/src/mac/mozAccessible.h
@@ +50,5 @@
> + * a document view. When we hand an object off to an AT, we always want
> + * to give it the represented view, in the latter case.
> + */
> +id <mozAccessible>
> +GetObjectOrRepresentedView(id <mozAccessible> anObject);

I still don't get why we can't keep in inline

::: accessible/src/mac/mozTextAccessible.mm
@@ +78,2 @@
>  #endif
> +                       nil];

why don't you copy object attributes from mozAccessible class?

@@ +87,5 @@
> +{
> +  if (mGeckoTextAccessible)
> +    return mGeckoTextAccessible->CaretLineNumber() - 1;
> +
> +  return 0;

does it really 0 when there's no caret?

@@ +95,5 @@
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
>  
> +  if ([attribute isEqualToString:NSAccessibilityVisitedAttribute])
> +    return [NSNumber numberWithBool:NO];

not sure I have a clear answer about this

@@ +106,5 @@
>    if ([attribute isEqualToString:NSAccessibilitySelectedTextAttribute])
>      return [self selectedText];
> +  if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
> +    // Apple's SpeechSynthesisServer expects AXValue to return an AXStaticText
> +    // object's AXSelectedText attribute.  See bug 674612.

It sounds like two spaces after dot.
I prefer to say 'See bug XXX for details', otherwise 'See bug' makes me think we have a bug.

@@ +122,5 @@
>  
>    NS_OBJC_END_TRY_ABORT_BLOCK_NIL;
>  }
>  
> +- (NSArray* )accessibilityParameterizedAttributeNames

excess space after *

@@ +124,5 @@
>  }
>  
> +- (NSArray* )accessibilityParameterizedAttributeNames
> +{
> +  static NSArray *supportedParametrizedAttributes = nil;

NSArray*

@@ +129,5 @@
> +  if (!supportedParametrizedAttributes) {
> +    // standard attributes that are shared and supported by all generic elements.
> +    supportedParametrizedAttributes = [[NSArray alloc] 
> +                                        initWithObjects:NSAccessibilityStringForRangeParameterizedAttribute,
> +                                        kAXLineForIndexParameterizedAttribute,

maybe we could do something like:
attributes = [[NSArray alloc] initWithObject:
  NSAccessibilityBla,
  NSAccessibleBla2
];

@@ +130,5 @@
> +    // standard attributes that are shared and supported by all generic elements.
> +    supportedParametrizedAttributes = [[NSArray alloc] 
> +                                        initWithObjects:NSAccessibilityStringForRangeParameterizedAttribute,
> +                                        kAXLineForIndexParameterizedAttribute,
> +                                        kAXRangeForLineParameterizedAttribute,

not sure I got about the CoreFoundation side of the API. But they say there's NSAccessibilityLineForIndexParameterizedAttribute in NSAccessibility.h. Is it different from kAXRangeForLineParameterizedAttribute or it doesn't work due to some reason?

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 608149 [details] [diff] [review]
Part 3: fix the missing implementations of the text protocol. r=

Review of attachment 608149 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +269,5 @@
> +
> +    nsString text;
> +    nsCocoaUtils::GetStringForNSString(stringValue, text);
> +    rv = mGeckoTextAccessible->InsertText(text, start);
> +    NS_ENSURE_SUCCESS(rv,);

is it requirement of API? insert the text into selection?

Comment 34

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #31)

> In either case it doesn't look valid that you make comboboxes and
> autocompletes fall into generic accessible class. Could you please go case
> by case and figure out hierarchies for each type? I just concern that you
> could be removing the code that we need.

hm but if actions on cross platform layer are the same or if we do a magic mapping then yes we can support it on generic accessible.

Comment 35

[alexander :surkov (:asurkov)]

Comment on attachment 608114 [details] [diff] [review]
Part 1: Autocomplete is to be ignored. Use TextField directly. ComboBox are not PopupMenuButton.

Review of attachment 608114 [details] [diff] [review]:
-----------------------------------------------------------------

so as far I have two concerns:

1) autocompletes claimed to expose two actions confirm and showmenu but they never were implemented. so we can remove them but we need to work on generic actions mapping mechanism and maintain it on generic accessible

2) NSAccessibilityExpandedAttribute is specific to comboboxes and autocompletes, it's missed (but anyway it was commented out), since OS X attributes of this kind are different from states (accessible can say whether it's applicable or not) then I think we need to have a classes for comboboxes, treeitems and etc to expose this attribute

3) Safari exposes comboboxes with NSAccessibilityPopUpButtonRole role (and we used to), the patch make it exposed with NSAccessibilityComboBoxRole.

So if you are really hot to remove this code then go ahead but I think one day we need to get back this class anyway (at least per 2nd item). r=me with 3d addressed.

Comment 36

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #35)
> Comment on attachment 608114 [details] [diff] [review]
> Part 1: Autocomplete is to be ignored. Use TextField directly. ComboBox are
> not PopupMenuButton.

btw, autocomplete is not ignored because generic accessible is created for it at least until you put some different meaning into 'ignored' word.

Comment 37

[Marco Zehe (:MarcoZ)]

I'll make sure to test this thoroughly. All comboboxes in the classic sense are popup menus in Mac OS X, be they html:select @size="1" or comboboxes in native COCOA applications. AutoCompletes are 2combobox" roles, with the possitility to enter. An example can be seen in the System Preferences, Date and Time, Timezone tab. The field that allows to enter the "nearest city" is such an autocomplete.

Comment 38

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #36)
> (In reply to alexander :surkov from comment #35)
> > Comment on attachment 608114 [details] [diff] [review]
> > Part 1: Autocomplete is to be ignored. Use TextField directly. ComboBox are
> > not PopupMenuButton.
> 
> btw, autocomplete is not ignored because generic accessible is created for
> it at least until you put some different meaning into 'ignored' word.

Ignored is the right word:

-[mozAccessible accessibilityIsIgnored] will return YES if the the role is NSAccessibilityUnknownRole.

Comment 39

[Hubert Figuiere [:hub]]

(In reply to alexander :surkov from comment #35)
> Comment on attachment 608114 [details] [diff] [review]
> Part 1: Autocomplete is to be ignored. Use TextField directly. ComboBox are
> not PopupMenuButton.

> 1) autocompletes claimed to expose two actions confirm and showmenu but they
> never were implemented. so we can remove them but we need to work on generic
> actions mapping mechanism and maintain it on generic accessible
> 
> 2) NSAccessibilityExpandedAttribute is specific to comboboxes and
> autocompletes, it's missed (but anyway it was commented out), since OS X
> attributes of this kind are different from states (accessible can say
> whether it's applicable or not) then I think we need to have a classes for
> comboboxes, treeitems and etc to expose this attribute
> 
> 3) Safari exposes comboboxes with NSAccessibilityPopUpButtonRole role (and
> we used to), the patch make it exposed with NSAccessibilityComboBoxRole.
> 
> So if you are really hot to remove this code then go ahead but I think one
> day we need to get back this class anyway (at least per 2nd item). r=me with
> 3d addressed.

Reverting 3) for now. There is some confusion as to what is what as in some case I actually want a NSAccessibilityComboBoxRole. This will be subject to a different bug.

Comment 1 and 2 should be addressed as a different bug /me think.

Comment 40

[Hubert Figuiere [:hub]]

Comment on attachment 608115 [details] [diff] [review]
Part 2: implement the text protocol for text accessible.

Review of attachment 608115 [details] [diff] [review]:
-----------------------------------------------------------------

It is hard to actually slice this patch more. It already does the minimum required to get things to work.

::: accessible/src/mac/mozAccessible.h
@@ +50,5 @@
> + * a document view. When we hand an object off to an AT, we always want
> + * to give it the represented view, in the latter case.
> + */
> +id <mozAccessible>
> +GetObjectOrRepresentedView(id <mozAccessible> anObject);

before it was just static. (static inline can be ignored by the compiler).

I see no benefit of making this inline.

::: accessible/src/mac/mozTextAccessible.mm
@@ +78,2 @@
>  #endif
> +                       nil];

we don't necessarily inherit the same subset.

@@ +87,5 @@
> +{
> +  if (mGeckoTextAccessible)
> +    return mGeckoTextAccessible->CaretLineNumber() - 1;
> +
> +  return 0;

maybe I can return -1.

@@ +95,5 @@
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
>  
> +  if ([attribute isEqualToString:NSAccessibilityVisitedAttribute])
> +    return [NSNumber numberWithBool:NO];

I see where it comes from now. Removing.

@@ +130,5 @@
> +    // standard attributes that are shared and supported by all generic elements.
> +    supportedParametrizedAttributes = [[NSArray alloc] 
> +                                        initWithObjects:NSAccessibilityStringForRangeParameterizedAttribute,
> +                                        kAXLineForIndexParameterizedAttribute,
> +                                        kAXRangeForLineParameterizedAttribute,

oops. for some reason I thought they were missing (part of the inconsistencies). changing.

Comment 41

[Hubert Figuiere [:hub]]

Comment on attachment 608149 [details] [diff] [review]
Part 3: fix the missing implementations of the text protocol. r=

Review of attachment 608149 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +269,5 @@
> +
> +    nsString text;
> +    nsCocoaUtils::GetStringForNSString(stringValue, text);
> +    rv = mGeckoTextAccessible->InsertText(text, start);
> +    NS_ENSURE_SUCCESS(rv,);

Replace the selected text with the new one. I do Delete and Insert.

Yes this is a mandated parameter, that should be settable.

Comment 42

[Hubert Figuiere [:hub]]

Attachment: Part 2: implement the text protocol for text accessible.

Comment 43

[Hubert Figuiere [:hub]]

Comment on attachment 608468 [details] [diff] [review]
Part 2: implement the text protocol for text accessible.

Updated patch following review comments.

Comment 44

[Hubert Figuiere [:hub]]

Comment on attachment 608468 [details] [diff] [review]
Part 2: implement the text protocol for text accessible.

cancelling the review for this patch. Will reupload a new one.

Comment 45

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #38)
> (In reply to alexander :surkov from comment #36)
> > (In reply to alexander :surkov from comment #35)
> > > Comment on attachment 608114 [details] [diff] [review]
> > > Part 1: Autocomplete is to be ignored. Use TextField directly. ComboBox are
> > > not PopupMenuButton.
> > 
> > btw, autocomplete is not ignored because generic accessible is created for
> > it at least until you put some different meaning into 'ignored' word.
> 
> Ignored is the right word:
> 
> -[mozAccessible accessibilityIsIgnored] will return YES if the the role is
> NSAccessibilityUnknownRole.

oh, right, I missed you use NSAccessibilityUnknownRole

Comment 46

[Hubert Figuiere [:hub]]

Attachment: Part 2: implement the text protocol for text accessible.

Comment 47

[Hubert Figuiere [:hub]]

Comment on attachment 608544 [details] [diff] [review]
Part 2: implement the text protocol for text accessible.

with all the feedback addressed (I hope)

Comment 48

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #40)

> It is hard to actually slice this patch more. It already does the minimum
> required to get things to work.

At this point I don't care much about working things :) Marco cares. I care about implementation. As far as I can see implementation can be separated into pars as I said above.

> > +id <mozAccessible>
> > +GetObjectOrRepresentedView(id <mozAccessible> anObject);
> 
> before it was just static. (static inline can be ignored by the compiler).
> 
> I see no benefit of making this inline.

It's incredibly short 
[obj hasRepresentedView] ? [obj representedView] : obj

it doesn't make sense to generate function call for this so either make it inline or remove it at all

but since latest your patch doesn't seem to switch everything to GetObjectOrRepresentedView then file a follow up bug so we can clear things later

> ::: accessible/src/mac/mozTextAccessible.mm
> @@ +78,2 @@
> >  #endif
> > +                       nil];
> 
> we don't necessarily inherit the same subset.

example?

Comment 49

[alexander :surkov (:asurkov)]

Comment on attachment 608544 [details] [diff] [review]
Part 2: implement the text protocol for text accessible.

Review of attachment 608544 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/nsHyperTextAccessible.h
@@ +264,5 @@
>      return GetChildAt(GetChildIndexAtOffset(aOffset));
>    }
>  
> +  /**
> +   * Get the bounds of the text between start and end.

Return boundaries rect of the text between given start and end offsets

@@ +266,5 @@
>  
> +  /**
> +   * Get the bounds of the text between start and end.
> +   */
> +  nsIntRect GetTextBounds(PRInt32 start, PRInt32 end)

again: start -> aStartOffset same for end too

::: accessible/src/mac/mozTextAccessible.mm
@@ +2,5 @@
>  
>  #include "nsAccessibleWrap.h"
> +#include "nsRootAccessible.h"
> +#include "nsIAccessibleText.h"
> +#include "nsIAccessibleTypes.h"

it doesn't sound like you need to include text and types idls

@@ +51,5 @@
>    if (!supportedAttributes) {
>      // standard attributes that are shared and supported by all generic elements.
> +    supportedAttributes = 
> +      [[NSArray alloc] initWithObjects:NSAccessibilityParentAttribute, // required
> +                       NSAccessibilityRoleAttribute,   // required

it seems comment #35 about styling is unaddressed

@@ +62,5 @@
> +                       NSAccessibilityWindowAttribute, // required
> +                       NSAccessibilityFocusedAttribute, // required
> +                       NSAccessibilityEnabledAttribute, // required
> +                       NSAccessibilityTopLevelUIElementAttribute, // required
> +                       NSAccessibilityDescriptionAttribute, // required

note to myself: comment #48 about inheritance should be addressed still

@@ +95,5 @@
>    if ([attribute isEqualToString:NSAccessibilityNumberOfCharactersAttribute])
>      return [NSNumber numberWithInt:[self textLength]];
> +  if ([attribute isEqualToString:NSAccessibilityInsertionPointLineNumberAttribute]) {
> +    PRInt32 caretLineNumber = [self caretLineNumber];
> +    return caretLineNumber != -1 ? [NSNumber numberWithInt:caretLineNumber] : nil;

I'd say caretLineNumber should take care about this

@@ +105,5 @@
> +  if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
> +    // Apple's SpeechSynthesisServer expects AXValue to return an AXStaticText
> +    // object's AXSelectedText attribute. See bug 674612 for details.
> +    // Also if there is no selected text, we return the full text. 
> +    // See bug 369710 for details

missed dot

@@ +124,5 @@
> +- (NSArray*)accessibilityParameterizedAttributeNames
> +{
> +  static NSArray* supportedParametrizedAttributes = nil;
> +  if (!supportedParametrizedAttributes) {
> +    // standard attributes that are shared and supported by all generic elements.

these are text specific parametrized attributes

@@ +136,5 @@
> +      NSAccessibilityRangeForPositionParameterizedAttribute,
> +      NSAccessibilityRangeForIndexParameterizedAttribute,
> +      NSAccessibilityRTFForRangeParameterizedAttribute,
> +      NSAccessibilityStyleRangeForIndexParameterizedAttribute,
> +#endif

why do keep those under debug?

Comment 50

[Hubert Figuiere [:hub]]

Comment on attachment 608544 [details] [diff] [review]
Part 2: implement the text protocol for text accessible.

Review of attachment 608544 [details] [diff] [review]:
-----------------------------------------------------------------

will resubmit the patches, resliced to have smaller chunks. With all the feedback in.

::: accessible/src/mac/mozTextAccessible.mm
@@ +2,5 @@
>  
>  #include "nsAccessibleWrap.h"
> +#include "nsRootAccessible.h"
> +#include "nsIAccessibleText.h"
> +#include "nsIAccessibleTypes.h"

gone

@@ +124,5 @@
> +- (NSArray*)accessibilityParameterizedAttributeNames
> +{
> +  static NSArray* supportedParametrizedAttributes = nil;
> +  if (!supportedParametrizedAttributes) {
> +    // standard attributes that are shared and supported by all generic elements.

bad cut&paste. fixed.

@@ +136,5 @@
> +      NSAccessibilityRangeForPositionParameterizedAttribute,
> +      NSAccessibilityRangeForIndexParameterizedAttribute,
> +      NSAccessibilityRTFForRangeParameterizedAttribute,
> +      NSAccessibilityStyleRangeForIndexParameterizedAttribute,
> +#endif

so I can track them down if they are queried by the accessibility client software. Debug purpose.

Comment 51

[Hubert Figuiere [:hub]]

Attachment: Part 2: Expose CaretLineNumber() and GetTextBounds() from nsHyperTextAccessible.

Comment 52

[Hubert Figuiere [:hub]]

Attachment: Part 3: Make helper GetObjectOrRepresentedView() public in mozAccessible.h.

Comment 53

[alexander :surkov (:asurkov)]

Comment on attachment 608558 [details] [diff] [review]
Part 3: Make helper GetObjectOrRepresentedView() public in mozAccessible.h.

Review of attachment 608558 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozAccessible.h
@@ +50,5 @@
> + * a document view. When we hand an object off to an AT, we always want
> + * to give it the represented view, in the latter case.
> + */
> +inline id <mozAccessible>
> +GetObjectOrRepresentedView(id <mozAccessible> anObject)

anObject is sort of funny, because 'a' means argument, not article

Comment 54

[Hubert Figuiere [:hub]]

Comment on attachment 608558 [details] [diff] [review]
Part 3: Make helper GetObjectOrRepresentedView() public in mozAccessible.h.

Review of attachment 608558 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozAccessible.h
@@ +50,5 @@
> + * a document view. When we hand an object off to an AT, we always want
> + * to give it the represented view, in the latter case.
> + */
> +inline id <mozAccessible>
> +GetObjectOrRepresentedView(id <mozAccessible> anObject)

doh. I'll fix this on commit. thx.

Comment 55

[Hubert Figuiere [:hub]]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9658fae44b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f72ba91d63a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6336183aab44

More patches to come

Comment 56

[Hubert Figuiere [:hub]]

> @@ +62,5 @@
> > +                       NSAccessibilityWindowAttribute, // required
> > +                       NSAccessibilityFocusedAttribute, // required
> > +                       NSAccessibilityEnabledAttribute, // required
> > +                       NSAccessibilityTopLevelUIElementAttribute, // required
> > +                       NSAccessibilityDescriptionAttribute, // required
> 
> note to myself: comment #48 about inheritance should be addressed still
> 

The "AXTitle" attribute: it is not for AXTextField roles. That's just one example.
So I found easier to just put the list we are supposed to handle.

We inherit the implementation for the attribute retrieval most of the time.

Comment 57

[Hubert Figuiere [:hub]]

Strike that last comment. I'll just inherit, it does not matter much (ie the system is tolerant) as it seems that the attributes are even more specific than that.

Comment 58

[Hubert Figuiere [:hub]]

Attachment: Part 4: implement the text protocol for text accessible.

Comment 59

[Hubert Figuiere [:hub]]

Comment on attachment 608887 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

with feedback addressed.

Comment 60

[Hubert Figuiere [:hub]]

Attachment: Part 5: fix the missing implementations of the text protocol.

Comment 61

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fc9658fae44b
https://hg.mozilla.org/mozilla-central/rev/8f72ba91d63a
https://hg.mozilla.org/mozilla-central/rev/6336183aab44

Comment 62

[alexander :surkov (:asurkov)]

Comment on attachment 608887 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 608887 [details] [diff] [review]:
-----------------------------------------------------------------

I might be not able to finish review today, so asking Trevor just in case

::: accessible/src/mac/mozTextAccessible.mm
@@ +47,5 @@
>    static NSArray *supportedAttributes = nil;
>    if (!supportedAttributes) {
>      // standard attributes that are shared and supported by all generic elements.
> +    supportedAttributes = [[NSMutableArray alloc] initWithObjects:
> +      /* text-specific attributes */

nit: please combine this comment with incorrect comment above

@@ +67,5 @@
> +{
> +  PRInt32 lineNumber = mGeckoTextAccessible ?
> +    mGeckoTextAccessible->CaretLineNumber() - 1 : -1;
> +
> +  return [NSNumber numberWithInt:lineNumber];

just to make sure: does it produce nil for -1 or previous patch was incorrect?

@@ +127,5 @@
> +
> +- (NSString*)stringFromRange:(NSRange*)range
> +{
> +  if (!mGeckoTextAccessible)
> +    return nil;

empty line please

@@ +132,5 @@
> +  nsAutoString text;
> +  mGeckoTextAccessible->GetText(range->location, 
> +				range->location + range->length, text);
> +  return text.IsEmpty() ? nil : [NSString stringWithCharacters:text.BeginReading() 
> +							length:text.Length()];

is it normal style for mac? I would expect something like
[NSString stringWithCharacters:text.BeginReading()
          length:text.Length()

actually you could try:
return text.IsEmpty() ?
  nil : [NSString stringWithCharacters:text.BeginReading() length:text.Length()];
or if it doesn't fit
return text.IsEmpty() ?
  nil :
  [NSString stringWithCharacters:text.BeginReading() length:text.Length()];

@@ +133,5 @@
> +  mGeckoTextAccessible->GetText(range->location, 
> +				range->location + range->length, text);
> +  return text.IsEmpty() ? nil : [NSString stringWithCharacters:text.BeginReading() 
> +							length:text.Length()];
> +}

btw, maybe you shouldn't mix NSAccessibility protocol implementation with implementation details. I mean group things like accessibilityParameterizedAttributeNames and group things like stringFromRange

Comment 63

[alexander :surkov (:asurkov)]

Comment on attachment 608887 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 608887 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +146,5 @@
> +      strcmp([(NSValue*)parameter objCType], @encode(NSRange)) == 0) {
> +    rangeSet = true;
> +    range = [(NSValue*)parameter rangeValue];
> +  } else if ([parameter isKindOfClass:[NSNumber class]])
> +    numberParam = parameter;

I see you do similar things to webkit http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/mac/AccessibilityObjectWrapper.mm?rev=82687#L2492 but I don't like to keep local variables and do extra ifs for things that never hit.

I'd suggest to move the logic where it's used. If you like to repeat unnice things like
if ([parameter isKindOfClass:[NSValue class]] &&
		strcmp([(NSValue*)parameter objCType], @encode(NSRange)) == 0) {
then it makes sense to provide set of inlines like
bool ToNSRange(parameter, NSRange& range) (not sure about object c syntax)

another benefit on inlines you can share them between methods

@@ +170,5 @@
> +      return @"";
> +    }
> +
> +    return [[[NSAttributedString alloc] initWithString:[self stringFromRange:&range]] autorelease];
> +  }

nit: it'd be nicer if you can separate these ifs by new lines

@@ +275,5 @@
>  
> +- (BOOL)canBeFocused
> +{
> +  return YES;
> +}

not sure if you answered it (comment #11)

@@ +394,5 @@
>  
> +- (NSValue*)visibleCharacterRange
> +{
> +  // XXX this won't work with Textarea and such as we actually don't give
> +  // the visible character range. Fix this.

Fix this is excess since XXX points to that already

@@ +399,5 @@
> +  return [NSValue valueWithRange:NSMakeRange(0, 
> +                                             mGeckoTextAccessible ? 
> +                                             mGeckoTextAccessible->
> +                                               CharacterCount() :
> +                                             0)];

some weird indentation, maybe
return [NSValue valueWithRange:
  NSMakeRange(0, mGeckoTextAccessible ?
                 mGeckoTextAccessible->CharacterCount() : 0];

@@ +415,5 @@
>  
> +- (void)selectedTextDidChange
> +{
> +  NSAccessibilityPostNotification(GetObjectOrRepresentedView(self),
> +                                 NSAccessibilitySelectedTextChangedNotification);

wrong indentation

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +187,1 @@
>      return NS_OK;

it doesn't sound like you really need to double check event types (this if and switch below)

Comment 64

[Hubert Figuiere [:hub]]

Comment on attachment 608887 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 608887 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +67,5 @@
> +{
> +  PRInt32 lineNumber = mGeckoTextAccessible ?
> +    mGeckoTextAccessible->CaretLineNumber() - 1 : -1;
> +
> +  return [NSNumber numberWithInt:lineNumber];

oops. what was I thinking.

@@ +132,5 @@
> +  nsAutoString text;
> +  mGeckoTextAccessible->GetText(range->location, 
> +				range->location + range->length, text);
> +  return text.IsEmpty() ? nil : [NSString stringWithCharacters:text.BeginReading() 
> +							length:text.Length()];

Emacs and Xcode indent aligning the ":".

On the other hand, I think I should instead use nsCocoaUtils::ToNSString().

@@ +133,5 @@
> +  mGeckoTextAccessible->GetText(range->location, 
> +				range->location + range->length, text);
> +  return text.IsEmpty() ? nil : [NSString stringWithCharacters:text.BeginReading() 
> +							length:text.Length()];
> +}

ok, I'll move it down.

@@ +275,5 @@
>  
> +- (BOOL)canBeFocused
> +{
> +  return YES;
> +}

I thought I did.

Actually this is wrong because non editable text can't have this set to true (it determines whether the a11y API can set the attributes or not).

Now the question is why do the editable text don't have mGeckoAccessible->State() & states::FOCUSABLE ?

I'll remove this function as it works without. Doc says "may be settable"

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +187,1 @@
>      return NS_OK;

I think the original intent was to avoid creating accessible for event we don't handle as GetNativeInterface() is doing it on the fly.

The comment needs update though.

Comment 65

[Hubert Figuiere [:hub]]

Attachment: Part 4: implement the text protocol for text accessible.

Comment 66

[Hubert Figuiere [:hub]]

Comment on attachment 608887 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 608887 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +146,5 @@
> +      strcmp([(NSValue*)parameter objCType], @encode(NSRange)) == 0) {
> +    rangeSet = true;
> +    range = [(NSValue*)parameter rangeValue];
> +  } else if ([parameter isKindOfClass:[NSNumber class]])
> +    numberParam = parameter;

ok. will do.

I indeed looked did a bit like in WebKit.

Comment 67

[Hubert Figuiere [:hub]]

Comment on attachment 610384 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

with comments addressed.

Comment 68

[Hubert Figuiere [:hub]]

Attachment: Part 5: fix the missing implementations of the text protocol.

Comment 69

[Hubert Figuiere [:hub]]

Attachment: Part 6: Cleanup string conversion. Make sure to return enmpty string for text instead of nil.

Comment 70

[Hubert Figuiere [:hub]]

Comment on attachment 610392 [details] [diff] [review]
Part 6: Cleanup string conversion. Make sure to return enmpty string for text instead of nil.

This should be the last patch.

Comment 71

[alexander :surkov (:asurkov)]

Comment on attachment 610384 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 610384 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, but I wouldn't take another look

::: accessible/src/mac/mozTextAccessible.mm
@@ +9,5 @@
>  
>  using namespace mozilla::a11y;
>  
> +inline bool
> +ToNSRange(id aValue, NSRange* aRange)

don't you need these methods for non text attributes implementation? If you need then it's worth to move them into mac utils I think

@@ +11,5 @@
>  
> +inline bool
> +ToNSRange(id aValue, NSRange* aRange)
> +{
> +  NS_ENSURE_TRUE(aRange, false);

you control call paths, so you don't need a null check here. But you can add NS_PRECONDITION debug macros so you see an assertion before crash

@@ +14,5 @@
> +{
> +  NS_ENSURE_TRUE(aRange, false);
> +
> +  if ([aValue isKindOfClass:[NSValue class]] 
> +      && strcmp([(NSValue*)aValue objCType], @encode(NSRange)) == 0) {

&& in the end of line

@@ +25,5 @@
> +
> +inline bool
> +ToNSString(id aValue, NSString** aString)
> +{
> +  NS_ENSURE_TRUE(aString, false);

same here

@@ +82,5 @@
> +      NSAccessibilityVisibleCharacterRangeAttribute, // required
> +      NSAccessibilityInsertionPointLineNumberAttribute,
> +      nil
> +    ];
> +    [supportedAttributes addObjectsFromArray:[super accessibilityAttributeNames]];

I assume attributes ordering doesn't play any role

@@ +89,5 @@
>  
>    NS_OBJC_END_TRY_ABORT_BLOCK_NIL;
>  }
>  
> +-(NSNumber*)caretLineNumber

please keep implementation internals away from protocol implementation, possibly after (BOOL)isReadOnly method

to make it visible you need to add to
@interface mozTextAccessible (Private)

@@ +120,5 @@
> +
> +    return [self text];
> +  }
> +  if ([attribute isEqualToString:NSAccessibilityVisibleCharacterRangeAttribute])
> +    return [self visibleCharacterRange];

a general style nit: it sounds having new lines between ifs make code a little more readable

@@ +162,5 @@
> +      strcmp([(NSValue*)parameter objCType], @encode(NSRange)) == 0) {
> +    rangeSet = true;
> +    range = [(NSValue*)parameter rangeValue];
> +  } else if ([parameter isKindOfClass:[NSNumber class]])
> +    numberParam = parameter;

well, you introduced inlines but you don't use them

@@ +176,5 @@
> +    return [self stringFromRange:&range];
> +  }
> +  if ([attribute isEqualToString:NSAccessibilityRangeForLineParameterizedAttribute])
> +    // XXX: actually get the integer value for the line #
> +    return [NSValue valueWithRange:NSMakeRange(0,[self textLength])];

nit: it's nicer to use { } here, a comment makes feel that there is two lines

@@ +178,5 @@
> +  if ([attribute isEqualToString:NSAccessibilityRangeForLineParameterizedAttribute])
> +    // XXX: actually get the integer value for the line #
> +    return [NSValue valueWithRange:NSMakeRange(0,[self textLength])];
> +
> +  if ([attribute isEqualToString:NSAccessibilityAttributedStringForRangeParameterizedAttribute]) {

Donaudampfschiffahrtsgesellschaftskapitän :)

@@ +242,5 @@
>  
>    if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
> +    NSString* text = nil;
> +    if (ToNSString(value, &text)) 
> +      [self setText:text];

return? and below too (get rid if elses)

@@ +247,5 @@
> +  } else if ([attribute isEqualToString:NSAccessibilitySelectedTextAttribute]) {
> +    // XXX to do
> +  } else if ([attribute isEqualToString:NSAccessibilitySelectedTextRangeAttribute]) {
> +    NSRange range;
> +    if (!ToNSRange(value, &range) || !mGeckoTextAccessible)

it sounds like you can check mGeckoTextAccessible at the method beginning

@@ +398,2 @@
>    }
> +  return [NSValue valueWithRange:NSMakeRange(0, 0)];

no caret means (0, 0) selection. just to be on safe side: that's what mac expects, right? No things like nil?

@@ +406,5 @@
> +  // XXX this won't work with Textarea and such as we actually don't give
> +  // the visible character range.
> +  return [NSValue valueWithRange:
> +    NSMakeRange(0, mGeckoTextAccessible ? 
> +            mGeckoTextAccessible->CharacterCount() : 0)];

wrong indent of last line

Comment 72

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #71)
> > +  if ([attribute isEqualToString:NSAccessibilityAttributedStringForRangeParameterizedAttribute]) {
> 
> Donaudampfschiffahrtsgesellschaftskapitän :)

Donaudampfschiffahrtsgesellschaftskapitänsmützenknopfhersteller

Thanks for this good laugh in the morning!

Comment 73

[Hubert Figuiere [:hub]]

Comment on attachment 610384 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 610384 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +9,5 @@
>  
>  using namespace mozilla::a11y;
>  
> +inline bool
> +ToNSRange(id aValue, NSRange* aRange)

not at the moment.

@@ +11,5 @@
>  
> +inline bool
> +ToNSRange(id aValue, NSRange* aRange)
> +{
> +  NS_ENSURE_TRUE(aRange, false);

yeah OK

@@ +82,5 @@
> +      NSAccessibilityVisibleCharacterRangeAttribute, // required
> +      NSAccessibilityInsertionPointLineNumberAttribute,
> +      nil
> +    ];
> +    [supportedAttributes addObjectsFromArray:[super accessibilityAttributeNames]];

nope it does not.

@@ +89,5 @@
>  
>    NS_OBJC_END_TRY_ABORT_BLOCK_NIL;
>  }
>  
> +-(NSNumber*)caretLineNumber

actually it shouldn't @interface mozTextAccessible (Private) but @interface mozTextAccessible () - anonymous category.

Will move the method impl.

@@ +120,5 @@
> +
> +    return [self text];
> +  }
> +  if ([attribute isEqualToString:NSAccessibilityVisibleCharacterRangeAttribute])
> +    return [self visibleCharacterRange];

ok

@@ +162,5 @@
> +      strcmp([(NSValue*)parameter objCType], @encode(NSRange)) == 0) {
> +    rangeSet = true;
> +    range = [(NSValue*)parameter rangeValue];
> +  } else if ([parameter isKindOfClass:[NSNumber class]])
> +    numberParam = parameter;

argh. will fix that too. my bad.

@@ +176,5 @@
> +    return [self stringFromRange:&range];
> +  }
> +  if ([attribute isEqualToString:NSAccessibilityRangeForLineParameterizedAttribute])
> +    // XXX: actually get the integer value for the line #
> +    return [NSValue valueWithRange:NSMakeRange(0,[self textLength])];

uh oh.
done.

@@ +242,5 @@
>  
>    if ([attribute isEqualToString:NSAccessibilityValueAttribute]) {
> +    NSString* text = nil;
> +    if (ToNSString(value, &text)) 
> +      [self setText:text];

ok. all the way down.

@@ +247,5 @@
> +  } else if ([attribute isEqualToString:NSAccessibilitySelectedTextAttribute]) {
> +    // XXX to do
> +  } else if ([attribute isEqualToString:NSAccessibilitySelectedTextRangeAttribute]) {
> +    NSRange range;
> +    if (!ToNSRange(value, &range) || !mGeckoTextAccessible)

ok

@@ +398,2 @@
>    }
> +  return [NSValue valueWithRange:NSMakeRange(0, 0)];

yes. it means no selection, caret at the beginning.

@@ +406,5 @@
> +  // XXX this won't work with Textarea and such as we actually don't give
> +  // the visible character range.
> +  return [NSValue valueWithRange:
> +    NSMakeRange(0, mGeckoTextAccessible ? 
> +            mGeckoTextAccessible->CharacterCount() : 0)];

fixed

Comment 74

[Hubert Figuiere [:hub]]

Attachment: Part 4: implement the text protocol for text accessible.

Comment 75

[Hubert Figuiere [:hub]]

Comment on attachment 610696 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

since you wanted to see that last version

Comment 76

[alexander :surkov (:asurkov)]

Comment on attachment 610696 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 610696 [details] [diff] [review]:
-----------------------------------------------------------------

ok r=me, you might want to upload new patch so me or Trevor could skim it again

::: accessible/src/mac/mozTextAccessible.mm
@@ +23,5 @@
> +  return false;
> +}
> +
> +inline bool
> +ToNSString(id aValue, NSString** aString)

consider to change it to
inline NSString* ToNSString(id aValue)

if you can return nil for NSRange (and if it's different from (0,0) range) then it makes sense to do that for ToNSRange too.

@@ +159,5 @@
> +    NSRange range;
> +    if (!ToNSRange(parameter, &range)) {
> +#if DEBUG
> +      NSLog(@"%@: range not set", attribute);
> +#endif

curious if you can use NS_WARNING macros here

@@ +168,5 @@
> +  }
> +
> +  if ([attribute isEqualToString:NSAccessibilityRangeForLineParameterizedAttribute]) {
> +    // XXX: actually get the integer value for the line #
> +    return [NSValue valueWithRange:NSMakeRange(0,[self textLength])];

space after 0,

@@ +171,5 @@
> +    // XXX: actually get the integer value for the line #
> +    return [NSValue valueWithRange:NSMakeRange(0,[self textLength])];
> +  }
> +
> +  if ([attribute isEqualToString:NSAccessibilityAttributedStringForRangeParameterizedAttribute]) {

curious how different from NSAccessibilityStringForRangeParameterizedAttribute

@@ +185,5 @@
> +  }
> +
> +  if ([attribute isEqualToString:NSAccessibilityLineForIndexParameterizedAttribute])
> +    // XXX: actually return the line #
> +    return [NSNumber numberWithInt:0];

please wrap if statement by {}

@@ +261,5 @@
> +  }
> +
> +  if ([attribute isEqualToString:NSAccessibilityVisibleCharacterRangeAttribute]) {
> +    NSRange range;
> +    if (!ToNSRange(value, &range) || !mGeckoTextAccessible)

you have mGeckoTextAccessilbe check on the top of method

@@ +405,5 @@
> +
> +      return [NSValue valueWithRange:NSMakeRange(start, end - start)];
> +    }
> +
> +    rv = mGeckoTextAccessible->GetCaretOffset(&start);

please file a bug to add a method that returns selection including selection for caret. Iirc GetSelectionBounds removes selection range for caret so you do extra work by calling the GetCaretOffset to get selection for caret. But also GetCaretOffset itself is not performant for you because it traverse the tree to check the focus to return -1 when no focus in the tree.

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +188,1 @@
>      return NS_OK;

iirc, I said that already but don't recall if you answered (maybe I missed it): you don't need to check eventsType since you have a switch below. You're going to add more and more events and one day you might check dozens of events twice.

Note, one day will do aEvent->GetAccessible() and accessible->GetNativeInterface() are really light. And that day is not too far :)

Comment 77

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #76)
> Comment on attachment 610696 [details] [diff] [review]
> Part 4: implement the text protocol for text accessible.
> ok r=me, you might want to upload new patch so me or Trevor could skim it
> again

it makes sense to do for part5 and part6 since they are dependent on part4 (iirc I didn't commented some stuffs because they were needed to be addressed in part4 first)

Comment 78

[Hubert Figuiere [:hub]]

Comment on attachment 610696 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 610696 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +23,5 @@
> +  return false;
> +}
> +
> +inline bool
> +ToNSString(id aValue, NSString** aString)

NSRange is a struct, not an Objective-C class. I could use (0,0) as a placeholder, but I'd rather not.

I'll change ToNSString

@@ +168,5 @@
> +  }
> +
> +  if ([attribute isEqualToString:NSAccessibilityRangeForLineParameterizedAttribute]) {
> +    // XXX: actually get the integer value for the line #
> +    return [NSValue valueWithRange:NSMakeRange(0,[self textLength])];

ok

@@ +171,5 @@
> +    // XXX: actually get the integer value for the line #
> +    return [NSValue valueWithRange:NSMakeRange(0,[self textLength])];
> +  }
> +
> +  if ([attribute isEqualToString:NSAccessibilityAttributedStringForRangeParameterizedAttribute]) {

a NSAttributedString contain style. We just make a plain one.

@@ +261,5 @@
> +  }
> +
> +  if ([attribute isEqualToString:NSAccessibilityVisibleCharacterRangeAttribute]) {
> +    NSRange range;
> +    if (!ToNSRange(value, &range) || !mGeckoTextAccessible)

oh I missed that one.

@@ +405,5 @@
> +
> +      return [NSValue valueWithRange:NSMakeRange(start, end - start)];
> +    }
> +
> +    rv = mGeckoTextAccessible->GetCaretOffset(&start);

Done. https://bugzilla.mozilla.org/show_bug.cgi?id=740892

::: accessible/src/mac/nsAccessibleWrap.mm
@@ +188,1 @@
>      return NS_OK;

GetNativeInterface() isn't light on Mac, as I said before as we lazily create the mozAccessible objects on demand.

Comment 79

[Hubert Figuiere [:hub]]

Comment on attachment 610696 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 610696 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +159,5 @@
> +    NSRange range;
> +    if (!ToNSRange(parameter, &range)) {
> +#if DEBUG
> +      NSLog(@"%@: range not set", attribute);
> +#endif

The problem with NS_WARNING is that I don't have the easy %@ formatting to display Objective-C object (standard in Cocoa, NSLog supports it).
Also I'm not sure NS_WARNING even support format strings.

I actually plan on getting rid of these at one point.

Comment 80

[Hubert Figuiere [:hub]]

Attachment: Part 4: implement the text protocol for text accessible.

Comment 81

[Hubert Figuiere [:hub]]

Comment on attachment 611039 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

with the latest comments.

Comment 82

[Hubert Figuiere [:hub]]

Attachment: Part 5: fix the missing implementations of the text protocol.

Comment 83

[Hubert Figuiere [:hub]]

Attachment: Part 6: Cleanup string conversion. Make sure to return enmpty string for text instead of nil.

Comment 84

[Hubert Figuiere [:hub]]

Comment on attachment 611045 [details] [diff] [review]
Part 5: fix the missing implementations of the text protocol.

rebased patch

Comment 85

[alexander :surkov (:asurkov)]

Comment on attachment 611039 [details] [diff] [review]
Part 4: implement the text protocol for text accessible.

Review of attachment 611039 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 86

[alexander :surkov (:asurkov)]

Comment on attachment 611046 [details] [diff] [review]
Part 6: Cleanup string conversion. Make sure to return enmpty string for text instead of nil.

Review of attachment 611046 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +349,5 @@
>      return nil;
>      
>    nsAutoString text;
> +  nsresult rv = mGeckoTextAccessible->GetText(0, 
> +    nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT, text);

why do you change indentation, the previous version was correct

Comment 87

[Hubert Figuiere [:hub]]

Comment on attachment 611046 [details] [diff] [review]
Part 6: Cleanup string conversion. Make sure to return enmpty string for text instead of nil.

Review of attachment 611046 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +349,5 @@
>      return nil;
>      
>    nsAutoString text;
> +  nsresult rv = mGeckoTextAccessible->GetText(0, 
> +    nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT, text);

wrapping at 80 column. Trevor usually request it.

Comment 88

[Hubert Figuiere [:hub]]

Patches 4 and 5 landed in inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/57e51e0c0c78
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d499f685a61

Comment 89

[Hubert Figuiere [:hub]]

Comment on attachment 611046 [details] [diff] [review]
Part 6: Cleanup string conversion. Make sure to return enmpty string for text instead of nil.

Review of attachment 611046 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/mac/mozTextAccessible.mm
@@ +349,5 @@
>      return nil;
>      
>    nsAutoString text;
> +  nsresult rv = mGeckoTextAccessible->GetText(0, 
> +    nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT, text);

After talking to Trevor he tells me you are the one that like the wrap at 80 column. Let me know if you want me to revert this change before committing. I have held the patch back for now.

Comment 90

[alexander :surkov (:asurkov)]

(In reply to Hub Figuiere [:hub] from comment #89)
> >    nsAutoString text;
> > +  nsresult rv = mGeckoTextAccessible->GetText(0, 
> > +    nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT, text);
> 
> After talking to Trevor he tells me you are the one that like the wrap at 80
> column. Let me know if you want me to revert this change before committing.
> I have held the patch back for now.

thank you. sometimes we do:

nsresult rv = mGeckoTextAccessible->
  GetText(0, nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT, text);

Comment 91

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/57e51e0c0c78
https://hg.mozilla.org/mozilla-central/rev/1d499f685a61

Comment 92

[Hubert Figuiere [:hub]]

Last patch inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c95b597b5f08

Comment 93

[alexander :surkov (:asurkov)]

Hub, do you wanna keep it open for further investigations (note this comment has 93 number) or file another one?

Comment 94

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/c95b597b5f08

considered comment 92, I'm assuming the WB entry is old, and resolving.

Comment 95

[Hubert Figuiere [:hub]]

yes, it is good to be closed now. I forgot to remove the WB.

thanks !