1011496 - complete the TextRange interface design

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Next step on UIA text implementation:

1) complete XPCOM/internal TextRange interface to follow UIA TextRange interface
2) implement Text(), Container() and compare methods

Comment 1

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

Comment on attachment 8423852 [details] [diff] [review]
patch

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

Woot! We're on our way! :)

I'm rusty. I'll need some back and forth. This isn't a full first review yet, just sharing some notes and questions but would like to whiteboard a bit tomorrow.

::: accessible/public/nsIAccessibleTextRange.idl
@@ +42,5 @@
> +  /**
> +   * Compare end points of this and given ranges.
> +   *
> +   * @return -1/0/1 if the end point of this range is before/equal/after of end
> +   *          point of the given range.

nit: I'd remove 'of'.

@@ +126,5 @@
> +  const unsigned long UnderlineStyleAttr = 39;
> +
> +  /**
> +   * Return range enslosing the text having requested attribute.
> +   */

nit: "enclosing"

::: accessible/src/base/TextRange.cpp
@@ +58,5 @@
>  }
>  
> +Accessible*
> +TextRange::Container() const
> +{

(Trying to think how you can nicely share the similar logic in these methods...)

@@ +125,5 @@
> +  for (uint32_t len = std::min(pos1, pos2); len > 0; --len) {
> +    Accessible* child1 = parents1.ElementAt(--pos1);
> +    Accessible* child2 = parents2.ElementAt(--pos2);
> +    if (child1 != child2)
> +      break;

It took me a moment to realize this works because we are essentially starting at the doc root. BTW can you add a brief comment at the start of this block? Something like "// find deepest common container"

@@ +133,5 @@
> +
> +  // Traverse the tree up to the container and collect embedded objects.
> +  Accessible* child = mStartContainer->GetChildAtOffset(mStartOffset);
> +  while (child != container) {
> +  for (int32_t idx = 0; idx < static_cast<int32_t>(pos1 - 1); idx++) {

Good to avoid static casts from unsigned to signed but I guess worrying about the parent chain exceeding INT32_MAX is silly. I am confused however because I think I can think of an example where pos1 is 0 so this loop would never fly for this case... ?

::: accessible/src/base/TextRange.h
@@ +80,5 @@
> +  TextPoint StartPoint() const { return TextPoint(mStartContainer, mStartOffset); }
> +  TextPoint EndPoint() const { return TextPoint(mEndContainer, mEndOffset); }
> +
> +  /**
> +   * Retrun a container containing both start and end points.

nit: "Return"

@@ +96,5 @@
>     */
>    void Text(nsAString& aText) const;
>  
>    /**
> +   * Return list of bounding rects of the text range by lines.

What does "by lines" mean?

@@ +188,5 @@
> +  /**
> +   * Return range enclosing text having requested attribute.
> +   */
> +  void FindAttr(EAttr aAttr, nsIVariant* aValue, EDirection aDirection,
> +                TextRange* aFoundRange) const;

Do you anticipate more than two directions? If at all likely then this is fine, otherwise we should just go with bool...

Note to self: http://msdn.microsoft.com/en-us/library/windows/desktop/ee671383%28v=vs.85%29.aspx

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1610,5 @@
> +  while ((parent = child->Parent()) && !(ht = parent->AsHyperText()))
> +    child = parent;
> +
> +  // If no text then return collapsed text range, otherwise return a range
> +  // containing the text enclosed by the giving child.

nit: "given child"

@@ +1632,5 @@
> +
> +  // Return collapsed text range for the point.
> +  if (parent) {
> +    HyperTextAccessible* ht = parent->AsHyperText();
> +    int32_t offset = ht->GetChildOffset(child);

Could ht be null here? I guess this question comes down to, will we ever have an accessible tree that includes accessible containers without mGenericTypes |= eHyperText?

::: accessible/src/xpcom/xpcAccessibleTextRange.cpp
@@ +84,5 @@
> +  uint32_t len = objects.Length();
> +  for (uint32_t idx = 0; idx < len; idx++)
> +    xpcList->AppendElement(static_cast<nsIAccessible*>(objects[idx]), false);
> +
> +  xpcList.forget(aList);

(note to self: left off here)

@@ +115,5 @@
> +
> +  TextPoint p = (aEndPoint == EndPoint_Start) ?
> +    mRange.StartPoint() : mRange.EndPoint();
> +  TextPoint otherPoint = (aOtherRangeEndPoint == EndPoint_Start) ?
> +    xpcRange->mRange.StartPoint() : xpcRange->mRange.EndPoint();

(note to self: this currently doesn't make sense to me yet...)

Comment 2

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #1)

> > +Accessible*
> > +TextRange::Container() const
> > +{
> 
> (Trying to think how you can nicely share the similar logic in these
> methods...)

agree but those are short and slightly different, no good ideas as far

> @@ +133,5 @@
> > +
> > +  // Traverse the tree up to the container and collect embedded objects.
> > +  Accessible* child = mStartContainer->GetChildAtOffset(mStartOffset);
> > +  while (child != container) {
> > +  for (int32_t idx = 0; idx < static_cast<int32_t>(pos1 - 1); idx++) {
> 
> Good to avoid static casts from unsigned to signed but I guess worrying
> about the parent chain exceeding INT32_MAX is silly. I am confused however
> because I think I can think of an example where pos1 is 0 so this loop would
> never fly for this case... ?

I think we can go with uint32_t and add pos1 == 0 check later if we crash

> >    /**
> > +   * Return list of bounding rects of the text range by lines.
> 
> What does "by lines" mean?

if text range is spanned more than on one lines then return value is an array of rects of those lines. How to reword it better?

> > +  void FindAttr(EAttr aAttr, nsIVariant* aValue, EDirection aDirection,
> > +                TextRange* aFoundRange) const;
> 
> Do you anticipate more than two directions? If at all likely then this is
> fine, otherwise we should just go with bool...

no only two, just code is easier to read, you don't need to remember what bool argument is when you see the caller.

> > +  // Return collapsed text range for the point.
> > +  if (parent) {
> > +    HyperTextAccessible* ht = parent->AsHyperText();
> > +    int32_t offset = ht->GetChildOffset(child);
> 
> Could ht be null here? I guess this question comes down to, will we ever
> have an accessible tree that includes accessible containers without
> mGenericTypes |= eHyperText?

loop above takes care that parent->IsHyperText() is true

Comment 3

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

(In reply to alexander :surkov from comment #2)
> (In reply to David Bolter [:davidb] from comment #1)

OK

> > >    /**
> > > +   * Return list of bounding rects of the text range by lines.
> > 
> > What does "by lines" mean?
> 
> if text range is spanned more than on one lines then return value is an
> array of rects of those lines. How to reword it better?

Maybe "Return list of bounding rects of the lines comprising the text range"

Comment 4

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

Comment on attachment 8423852 [details] [diff] [review]
patch

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

For nsIAccessibleTextRange.idl... the gecko C++ style guide for idl files wants all-caps (i.e. 'ENDPOINT_START') for constants. I'd strive to adhere to the guide... possibly resorting to the 'k' prefix but I'm not sure that is desired for idl...

Comment 5

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

Comment on attachment 8423852 [details] [diff] [review]
patch

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

r=me. Woot! I didn't find anything horrible but I am very rusty and it might be worth getting Trevor or Neil to take a look. (I of course trust you to do the right things with my nits)

::: accessible/public/nsIAccessibleTextRange.idl
@@ +36,5 @@
> +   */
> +  boolean compare(in nsIAccessibleTextRange aOtherRange);
> +
> +  const unsigned long EndPoint_Start = 1;
> +  const unsigned long EndPoint_End = 2;

Please add a comment.
Note these names confused me for a while (I have to think of them as starting_endpoint and ending_endpoint), but I realize they sort of match the MS doc wording, so ok.
Maybe the comment should say something like:
// The two endpoints for the range (starting and ending)

::: accessible/src/base/TextRange.h
@@ +142,5 @@
> +  void FindText(const nsAString& aText, EDirection aDirection,
> +                nsCaseTreatment aCaseSensitive, TextRange* aFoundRange) const;
> +
> +  enum EAttr {
> +    eAnimationStyleAttr,

Please add a comment - maybe say where these come from.

::: accessible/tests/mochitest/textrange/test_general.html
@@ +77,5 @@
> +      res = p2Range.compareEndPoints(EndPoint_Start, p1Range, EndPoint_End);
> +      is(res, 1, "p2 range must be greater than p1 range");
> +
> +      res = p1Range.compareEndPoints(EndPoint_Start, p1Range, EndPoint_Start);
> +      is(res, 0, "p1 range must be equal than p1 range");

nit: than/with

Comment 6

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #5)

> r=me. Woot! I didn't find anything horrible but I am very rusty and it might
> be worth getting Trevor or Neil to take a look. (I of course trust you to do
> the right things with my nits)

sure, I'd love to get Trev's feedback.

> > +  const unsigned long EndPoint_Start = 1;
> > +  const unsigned long EndPoint_End = 2;
> 
> Please add a comment.
> Note these names confused me for a while (I have to think of them as
> starting_endpoint and ending_endpoint), but I realize they sort of match the
> MS doc wording, so ok.
> Maybe the comment should say something like:
> // The two endpoints for the range (starting and ending)

I share the feeling the names are ugly but nothing came better than copying the UIA

> > +  enum EAttr {
> > +    eAnimationStyleAttr,
> 
> Please add a comment - maybe say where these come from.

put a generic comment, I think each of them should be documented when implemented

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX B2G desktop bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ade5772804d

https://tbpl.mozilla.org/php/getParsedLog.php?id=40212887&tree=Mozilla-Inbound

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b59bad4038d