880159 - Character/word offsets for caret when at end of line should not return offsets for next line

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

The caret offset reported at the end of a wrapped line is the same as that reported at the start of the next line. To work around this, clients can pass IA2_TEXT_OFFSET_CARET to get the correct line offsets. However, when fetching character or word offsets, Gecko returns offsets for the character or word at the start of the next line. For a screen reader user using speech, this means that when you move to the end of a wrapped line, you hear the character at the start of the next line.

For character, Gecko should probably just return blank and a collapsed range; i.e. (x, x) where x is the caret offset. For word, it should probably either return blank or the previous word.

Thoughts welcome!

Comment 1

[alexander :surkov (:asurkov)]

Don't you like the idea to expose '\n' for soft line breaks? You mentioned that we loose semantics of soft line breaks and it might be important for AT. In that case we could expose 'softlinebreak' text attribute if it works.

Comment 2

[James Teh [:Jamie]]

I think I'd prefer not to have '\n' for soft line breaks. Most importantly, it isn't backwards compatible and will probably regress older versions of ATs. It's also not something I've ever seen in any other implementation, which isn't necessarily a show-stopper but is certainly a cause for caution.

Mick, I'd appreciate your thoughts too.

Comment 3

[Michael Curran]

I agree that expanding to word at this position should return the previous word, as this is what any whitespace would do. I'm not too sure how positive I am about  character being collapsed (i.e. not expanding) as I think this has the possibility of confusing ATs just as much as the \n. Is there a soft line break in the unicode standard? But then on the other hand if we're talking about the offset being -2, then I guess expanded would be -2,-1... which is also rather odd, and in deed miss-leading as -1 in IA2 means end of text. 
Therefore thinking on this, I think I'm most comfortable myself with Jamie's suggestion of "" and collapsed character. I'm assuming that the method would still return success though?

Comment 4

[alexander :surkov (:asurkov)]

it seems 0x2028 character is used as a soft line break (https://en.wikipedia.org/wiki/Word_wrap#Unicode)

Comment 5

[James Teh [:Jamie]]

As I understand it, line separator is used to unambiguously break a line but not a paragraph, rather like <br>. I don't think it is meant to be used for line wrap.

Comment 6

[James Teh [:Jamie]]

(In reply to Michael Curran from comment #3)
> But then on the other hand if we're
> talking about the offset being -2, then I guess expanded would be -2,-1...
> which is also rather odd, and in deed miss-leading as -1 in IA2 means end of
> text.
-2 and -1 are never provided in output, only in input. The output would be either, for example, (5, 5) orf (5, 6), but never (-2, -2) or (-2, -1).

Comment 7

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #5)
> As I understand it, line separator is used to unambiguously break a line but
> not a paragraph, rather like <br>. I don't think it is meant to be used for
> line wrap.

so would you need to differentiate soft line breaks from line separator characters?

Comment 8

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #7)
> so would you need to differentiate soft line breaks from line separator
> characters?
It's probably best to say "line wrap break" rather than "soft line break" to be clearer, since it seems <br> is sometimes also called a "soft line break". I think we do need to be able to differentiate between these two cases.

Comment 9

[James Teh [:Jamie]]

So, to summarise our thoughts on this after discussion, we think the behaviour should be as I specified in comment 0.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #9)
> So, to summarise our thoughts on this after discussion, we think the
> behaviour should be as I specified in comment 0.

which option do you want?

1) make IA2_TEXT_OFFSET_CARET working for char and words as well or 
2) make line/word/char working for offset equals to the caret offset?

Comment 11

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #10)
> which option do you want?
> 1) make IA2_TEXT_OFFSET_CARET working for char and words as well or 
> 2) make line/word/char working for offset equals to the caret offset?
I think option 1 is safest and clearest. It requires a change to NVDA, but I already have code for it.

Comment 13

[Joanmarie Diggs]

In terms of what option we'd like for ATK/AT-SPI2: Ideally, consistency with Gtk+ and WebKitGtk. ;) See the results I included in bug 919508 for each of those toolkits.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Joanmarie Diggs from comment #13)
> In terms of what option we'd like for ATK/AT-SPI2: Ideally, consistency with
> Gtk+ and WebKitGtk. ;) See the results I included in bug 919508 for each of
> those toolkits.

so is it 2nd option, right?

Comment 15

[Joanmarie Diggs]

(In reply to alexander :surkov from comment #14)
> (In reply to Joanmarie Diggs from comment #13)
> > In terms of what option we'd like for ATK/AT-SPI2: Ideally, consistency with
> > Gtk+ and WebKitGtk. ;) See the results I included in bug 919508 for each of
> > those toolkits.
> 
> so is it 2nd option, right?

Because I was a tad confused about the options, and because I am still lazy, c&p from our IRC convo just now. :)

<@joanie> alexsurkov: sorry to be dense, but I don't quite get the two options as described in the bug
<@joanie> but the -2 magic offset is not something I like
<alexsurkov> joanie: 1st option is it works when you give magic offset - 2, 2nd option is it works when you pass a caret offset 
<alexsurkov> 2nd option includes -2 magic offset as well
<@joanie> can I pass in a real offset, though? :)
<@joanie> without having to use magical ones? :)
<alexsurkov> that's why Jamie wanted to
<alexsurkov> but we agreed that -2 magic offset is more "safe"
<alexsurkov> I think we can go with real offsets
<alexsurkov> but give it a try with other ATs
<@joanie> that would be super
<alexsurkov> ok
<@joanie> my goal is for Orca to not have to care what toolkit (or rendering engine) something came from just to get a chunk of text. :)

Comment 16

[alexander :surkov (:asurkov)]

Attachment: part1: words

Comment 17

[James Teh [:Jamie]]

Option 2 means that a review cursor (such as that in NVDA) will get very confused when at the start of a wrapped line, so I don't think this is a good idea. Besides, I'm not convinced bug 919508 is a duplicate; I've commented there.

Comment 18

[Trevor Saunders (:tbsaunde)]

Comment on attachment 808728 [details] [diff] [review]
part1: words

>+  if (aBoundaryType == BOUNDARY_CHAR) {
>+    GetCharAt(aOffset, eGetBefore, aText, aStartOffset, aEndOffset);
>+    return NS_OK;
>+  }
>+
>   int32_t offset = ConvertMagicOffset(aOffset);
>   if (offset < 0)
>     return NS_ERROR_INVALID_ARG;
> 
>+  if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
>+    offset = AdjustCaretOffset(offset);

wouldn't this be clearer (and maybe faster) as

int32_t offset;
if (aOffset == CARET_OFFSET)
  offset = AdjustCaretOffset(aOffset);
else
  offset = ConvertMagicOffset(aOffset);

if (offset < 0)
  return NS_ERROR_FAILURE;

>   switch (aBoundaryType) {
>-    case BOUNDARY_CHAR:
>-      GetCharAt(offset, eGetBefore, aText, aStartOffset, aEndOffset);
>-      return NS_OK;

I'd prefer to have an assertion we don't hit this case (MOZ_ASSUME_UNREACHABLE()?) than just sighlently  failing.

>@@ -947,16 +945,19 @@ HyperTextAccessible::GetTextAtOffset(int
>     return NS_ERROR_INVALID_ARG;
> 
>   switch (aBoundaryType) {
>     case BOUNDARY_CHAR:
>       return GetCharAt(aOffset, eGetAt, aText, aStartOffset, aEndOffset) ?
>         NS_OK : NS_ERROR_INVALID_ARG;
> 
>     case BOUNDARY_WORD_START:
>+      if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
>+        offset = AdjustCaretOffset(offset);

this one is different because of dissagreement about word end case?

> HyperTextAccessible::GetTextAfterOffset(int32_t aOffset,
>                                         AccessibleTextBoundary aBoundaryType,
>                                         int32_t* aStartOffset,
>                                         int32_t* aEndOffset, nsAString& aText)
> {

same comments as GetTextBeforeOffset

the tests are far harder to understand than the code, but I'm not sure how to easily fix that, maybe check in tests generated with a script or generate the tests with a script at build time?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #18)
> Comment on attachment 808728 [details] [diff] [review]
> part1: words
> 
> >+  if (aBoundaryType == BOUNDARY_CHAR) {
> >+    GetCharAt(aOffset, eGetBefore, aText, aStartOffset, aEndOffset);
> >+    return NS_OK;
> >+  }
> >+
> >   int32_t offset = ConvertMagicOffset(aOffset);
> >   if (offset < 0)
> >     return NS_ERROR_INVALID_ARG;
> > 
> >+  if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
> >+    offset = AdjustCaretOffset(offset);
> 
> wouldn't this be clearer (and maybe faster) as
> 
> int32_t offset;
> if (aOffset == CARET_OFFSET)
>   offset = AdjustCaretOffset(aOffset);
> else
>   offset = ConvertMagicOffset(aOffset);
> 
> if (offset < 0)
>   return NS_ERROR_FAILURE;

that will give you -3 rather than a real offset


> >   switch (aBoundaryType) {
> >-    case BOUNDARY_CHAR:
> >-      GetCharAt(offset, eGetBefore, aText, aStartOffset, aEndOffset);
> >-      return NS_OK;
> 
> I'd prefer to have an assertion we don't hit this case
> (MOZ_ASSUME_UNREACHABLE()?) than just sighlently  failing.

it's reachable for example for 0, thus we hide an error here

> >@@ -947,16 +945,19 @@ HyperTextAccessible::GetTextAtOffset(int
> >     return NS_ERROR_INVALID_ARG;
> > 
> >   switch (aBoundaryType) {
> >     case BOUNDARY_CHAR:
> >       return GetCharAt(aOffset, eGetAt, aText, aStartOffset, aEndOffset) ?
> >         NS_OK : NS_ERROR_INVALID_ARG;
> > 
> >     case BOUNDARY_WORD_START:
> >+      if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
> >+        offset = AdjustCaretOffset(offset);
> 
> this one is different because of dissagreement about word end case?

yes

> the tests are far harder to understand than the code, but I'm not sure how
> to easily fix that, maybe check in tests generated with a script or generate
> the tests with a script at build time?

my tests are auto generated, there's no easy way to see the output but probably a need to see it is not helpful because I think hundreds of tests will be generated. But once we are confident with a test logic then we get a high readable test description format.

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #18)
> > Comment on attachment 808728 [details] [diff] [review]
> > part1: words
> > 
> > >+  if (aBoundaryType == BOUNDARY_CHAR) {
> > >+    GetCharAt(aOffset, eGetBefore, aText, aStartOffset, aEndOffset);
> > >+    return NS_OK;
> > >+  }
> > >+
> > >   int32_t offset = ConvertMagicOffset(aOffset);
> > >   if (offset < 0)
> > >     return NS_ERROR_INVALID_ARG;
> > > 
> > >+  if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
> > >+    offset = AdjustCaretOffset(offset);
> > 
> > wouldn't this be clearer (and maybe faster) as
> > 
> > int32_t offset;
> > if (aOffset == CARET_OFFSET)
> >   offset = AdjustCaretOffset(aOffset);
> > else
> >   offset = ConvertMagicOffset(aOffset);
> > 
> > if (offset < 0)
> >   return NS_ERROR_FAILURE;
> 
> that will give you -3 rather than a real offset

Actually won't it just mean calling AdjustcaretOfset() is useless?  I guess you don't want to just merge AdjustCaretOffset() into the magic offset stff because of things like GetPosAndText()?  In any case what we have is madness and we should make it simpler.

> > >   switch (aBoundaryType) {
> > >-    case BOUNDARY_CHAR:
> > >-      GetCharAt(offset, eGetBefore, aText, aStartOffset, aEndOffset);
> > >-      return NS_OK;
> > 
> > I'd prefer to have an assertion we don't hit this case
> > (MOZ_ASSUME_UNREACHABLE()?) than just sighlently  failing.
> 
> it's reachable for example for 0, thus we hide an error here

huh? we always hit the if aBoundaryType == BOUNDARY_CHAR which unconditionally returns no?

> > >@@ -947,16 +945,19 @@ HyperTextAccessible::GetTextAtOffset(int
> > >     return NS_ERROR_INVALID_ARG;
> > > 
> > >   switch (aBoundaryType) {
> > >     case BOUNDARY_CHAR:
> > >       return GetCharAt(aOffset, eGetAt, aText, aStartOffset, aEndOffset) ?
> > >         NS_OK : NS_ERROR_INVALID_ARG;
> > > 
> > >     case BOUNDARY_WORD_START:
> > >+      if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
> > >+        offset = AdjustCaretOffset(offset);
> > 
> > this one is different because of dissagreement about word end case?
> 
> yes

it leads to more madness, but I guess not much we can do till that's resolved, but lets do that quickly :)

> > the tests are far harder to understand than the code, but I'm not sure how
> > to easily fix that, maybe check in tests generated with a script or generate
> > the tests with a script at build time?
> 
> my tests are auto generated, there's no easy way to see the output but
> probably a need to see it is not helpful because I think hundreds of tests
> will be generated. But once we are confident with a test logic then we get a
> high readable test description format.

yeah, but generating the tests is entagled with running them.  Even if there's a ton I think its easier to see them all if you need to debug why one fial than try to figure out what that one would be based on log and code.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #20)

> Actually won't it just mean calling AdjustcaretOfset() is useless?

we do conversion and then adjustment

>  I guess
> you don't want to just merge AdjustCaretOffset() into the magic offset stff
> because of things like GetPosAndText()?

some cases like getTextAt for word end boundary don't need adjustment

>  In any case what we have is madness
> and we should make it simpler.

ideas are welcome

> > > >   switch (aBoundaryType) {
> > > >-    case BOUNDARY_CHAR:
> > > >-      GetCharAt(offset, eGetBefore, aText, aStartOffset, aEndOffset);
> > > >-      return NS_OK;
> > > 
> > > I'd prefer to have an assertion we don't hit this case
> > > (MOZ_ASSUME_UNREACHABLE()?) than just sighlently  failing.
> > 
> > it's reachable for example for 0, thus we hide an error here
> 
> huh? we always hit the if aBoundaryType == BOUNDARY_CHAR which
> unconditionally returns no?

oh, you meant to do something like?

if (boundary == char) {
  return;
}
switch (boundary) {
  case char:
    MOZ_UNRACHABLE();
}

> yeah, but generating the tests is entagled with running them.  Even if
> there's a ton I think its easier to see them all if you need to debug why
> one fial than try to figure out what that one would be based on log and code.

yes but these test can be commented out easily also if you need

Comment 22

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #17)
> Option 2 means that a review cursor (such as that in NVDA) will get very
> confused when at the start of a wrapped line, so I don't think this is a
> good idea. Besides, I'm not convinced bug 919508 is a duplicate; I've
> commented there.

the logic would be applied if the caret is at that offset as well. Is it your case?

Comment 23

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #22)
> the logic would be applied if the caret is at that offset as well. Is it
> your case?
Yes. For the sake of example, imagine you have a line "abcdef" which wraps to "abc" and "def". You press end on the first line, so your caret offset reports 3 ("d") but it's really at the end of the first line. The user decides they want to read ahead and moves the review cursor (not the caret) to the next line. If you match the caret offset (not just -2), they'll never be able to read the next line because it'll constantly report that offset 3 is on the first line. In fact, this gets really ugly, because offset 2 will report line offsets of (0, 3), which excludes 3, but 3 will report (0, 3).

I really think this needs to be restricted to -2.

Comment 24

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #21)
> (In reply to Trevor Saunders (:tbsaunde) from comment #20)
> 
> > Actually won't it just mean calling AdjustcaretOfset() is useless?
> 
> we do conversion and then adjustment
> 
> >  I guess
> > you don't want to just merge AdjustCaretOffset() into the magic offset stff
> > because of things like GetPosAndText()?
> 
> some cases like getTextAt for word end boundary don't need adjustment
> 
> >  In any case what we have is madness
> > and we should make it simpler.
> 
> ideas are welcome

cry?

> > > > >   switch (aBoundaryType) {
> > > > >-    case BOUNDARY_CHAR:
> > > > >-      GetCharAt(offset, eGetBefore, aText, aStartOffset, aEndOffset);
> > > > >-      return NS_OK;
> > > > 
> > > > I'd prefer to have an assertion we don't hit this case
> > > > (MOZ_ASSUME_UNREACHABLE()?) than just sighlently  failing.
> > > 
> > > it's reachable for example for 0, thus we hide an error here
> > 
> > huh? we always hit the if aBoundaryType == BOUNDARY_CHAR which
> > unconditionally returns no?
> 
> oh, you meant to do something like?

yes

> > yeah, but generating the tests is entagled with running them.  Even if
> > there's a ton I think its easier to see them all if you need to debug why
> > one fial than try to figure out what that one would be based on log and code.
> 
> yes but these test can be commented out easily also if you need

yeah, but then you still have to figure out what the logic around them does to understand what is being tested.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #24)

> > ideas are welcome
> 
> cry?

I hoped to have something more effective

> > > huh? we always hit the if aBoundaryType == BOUNDARY_CHAR which
> > > unconditionally returns no?
> > 
> > oh, you meant to do something like?
> 
> yes

ok

> yeah, but then you still have to figure out what the logic around them does
> to understand what is being tested.

I meant if there's a failure then it's easy to comment out non failures and debug one failing case.

I'm not really passionate about an idea to change the way we test here, especially I'm not sure I clearly see the alternative and its benefits.

Comment 26

[Trevor Saunders (:tbsaunde)]

> I'm not really passionate about an idea to change the way we test here,
> especially I'm not sure I clearly see the alternative and its benefits.

sure, mostly just complaining so hopefully you'll write something nicer in future.

Comment 27

[Trevor Saunders (:tbsaunde)]

Comment on attachment 808728 [details] [diff] [review]
part1: words

I guess this is fine  if you do the unreachable thing.  It might also be nice to change offset to  adjustedOffset to make it easier to tell apart from aOffset

Comment 28

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #27)
> Comment on attachment 808728 [details] [diff] [review]
> part1: words
> 
> I guess this is fine  if you do the unreachable thing.

what do you mean by unreachable thing?

>  It might also be
> nice to change offset to  adjustedOffset to make it easier to tell apart
> from aOffset

so just offset -> adjustedOffset?

Comment 29

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/23c7e67e9188

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23c7e67e9188

Comment 31

[alexander :surkov (:asurkov)]

Attachment: part2: words again

Comment 32

[Trevor Saunders (:tbsaunde)]

Comment on attachment 820986 [details] [diff] [review]
part2: words again

I'm not entirely convinced this really fixes the root problem, but if you do I guess its ok

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 820986 [details] [diff] [review]
part2: words again

landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a03817991da

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7a03817991da

Comment 35

[alexander :surkov (:asurkov)]

Attachment: patch3: clean up CharAt stuff

Comment 36

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8349670 [details] [diff] [review]
patch3: clean up CharAt stuff

>+  // PRUnichar is unsigned short in Mozilla, gnuichar is guint32 in glib.
>+  return static_cast<gunichar>(text->CharAt(aOffset));

technically I think this is wrong for combining characters and whatever where you need to 16 bit code points to describe a character, but its been that way forever so I'm not in a hurry to fix it

>-HyperTextAccessible::GetCharAt(int32_t aOffset, EGetTextType aShift,
>-                               nsAString& aChar, int32_t* aStartOffset,

can we remove the eGetTextType enum now?

>+  bool CharAt(int32_t aOffset, nsAString& aChar,
>+              int32_t* aStartOffset = nullptr, int32_t* aEndOffset = nullptr)
>   {
>     int32_t childIdx = GetChildIndexAtOffset(aOffset);
>     if (childIdx == -1)
>       return false;
> 
>     Accessible* child = GetChildAt(childIdx);
>     child->AppendTextTo(aChar, aOffset - GetChildOffset(childIdx), 1);
>+
>+    if (aStartOffset && aEndOffset) {
>+      *aStartOffset = aOffset;
>+      *aEndOffset = aOffset + aChar.Length();

assert both or neither is null?

Comment 37

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> Comment on attachment 8349670 [details] [diff] [review]
> patch3: clean up CharAt stuff
> 
> >+  // PRUnichar is unsigned short in Mozilla, gnuichar is guint32 in glib.
> >+  return static_cast<gunichar>(text->CharAt(aOffset));
> 
> technically I think this is wrong for combining characters and whatever
> where you need to 16 bit code points to describe a character, but its been
> that way forever so I'm not in a hurry to fix it

file a bug please?

Comment 38

[alexander :surkov (:asurkov)]

Comment on attachment 8349670 [details] [diff] [review]
patch3: clean up CharAt stuff

http://hg.mozilla.org/integration/mozilla-inbound/rev/2a74116d9dbe

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch4: fix char part

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2a74116d9dbe

Comment 41

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8350326 [details] [diff] [review]
patch4: fix char part

>     case BOUNDARY_CHAR:
>-      CharAt(adjustedOffset, aText, aStartOffset, aEndOffset);
>+      if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET && IsCaretAtEndOfLine())
>+        *aStartOffset = *aEndOffset = adjustedOffset;

so no text just out offsets set in this case?

>+      else
>+        CharAt(adjustedOffset, aText, aStartOffset, aEndOffset);

so only difference with case we're at the end of previous line is now we have text?

can you add some explanation of why this makes sense?

>     case BOUNDARY_CHAR:
>-      CharAt(convertedOffset + 1, aText, aStartOffset, aEndOffset);
>+      if (adjustedOffset >= CharacterCount())
>+        *aStartOffset = *aEndOffset = CharacterCount();

so has offset, but no text again seems kind of funny.

>+      else
>+        CharAt(adjustedOffset + 1, aText, aStartOffset, aEndOffset);

again comments would help ;)

Comment 42

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #41)
> Comment on attachment 8350326 [details] [diff] [review]
> patch4: fix char part
> 
> >     case BOUNDARY_CHAR:
> >-      CharAt(adjustedOffset, aText, aStartOffset, aEndOffset);
> >+      if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET && IsCaretAtEndOfLine())
> >+        *aStartOffset = *aEndOffset = adjustedOffset;
> 
> so no text just out offsets set in this case?

that's right

> 
> >+      else
> >+        CharAt(adjustedOffset, aText, aStartOffset, aEndOffset);
> 
> so only difference with case we're at the end of previous line is now we
> have text?

why previous line? it just return a char at adjustedOffset (same as convertedOffset here), if name is confusing then I can rename it to convertedOffset to be consistent

> can you add some explanation of why this makes sense?

does it work?
"Return no char if caret at the end of wrapped line (case of no line end character)"

> >     case BOUNDARY_CHAR:
> >-      CharAt(convertedOffset + 1, aText, aStartOffset, aEndOffset);
> >+      if (adjustedOffset >= CharacterCount())
> >+        *aStartOffset = *aEndOffset = CharacterCount();
> 
> so has offset, but no text again seems kind of funny.

it's like a null char

> 
> >+      else
> >+        CharAt(adjustedOffset + 1, aText, aStartOffset, aEndOffset);
> 
> again comments would help ;)

"If caret at end of wrapped line (case of no line end character) then char after the offset is a first char at next line" ok?

Comment 43

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #42)
> (In reply to Trevor Saunders (:tbsaunde) from comment #41)
> > Comment on attachment 8350326 [details] [diff] [review]
> > patch4: fix char part
> > 
> > >     case BOUNDARY_CHAR:
> > >-      CharAt(adjustedOffset, aText, aStartOffset, aEndOffset);
> > >+      if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET && IsCaretAtEndOfLine())
> > >+        *aStartOffset = *aEndOffset = adjustedOffset;
> > 
> > so no text just out offsets set in this case?
> 
> that's right

I hate that this exists, seems like it would make a lot more sense to return '\n' even if its caused by line wrapping.

> > 
> > >+      else
> > >+        CharAt(adjustedOffset, aText, aStartOffset, aEndOffset);
> > 
> > so only difference with case we're at the end of previous line is now we
> > have text?
> 
> why previous line? it just return a char at adjustedOffset (same as
> convertedOffset here), if name is confusing then I can rename it to
> convertedOffset to be consistent

nah, name is fine just funny how its different from other case.

> > can you add some explanation of why this makes sense?
> 
> does it work?
> "Return no char if caret at the end of wrapped line (case of no line end
> character)"

maybe say something about if its at start of next line, but otherwise fine

> > >     case BOUNDARY_CHAR:
> > >-      CharAt(convertedOffset + 1, aText, aStartOffset, aEndOffset);
> > >+      if (adjustedOffset >= CharacterCount())
> > >+        *aStartOffset = *aEndOffset = CharacterCount();
> > 
> > so has offset, but no text again seems kind of funny.
> 
> it's like a null char

yeah I guess, what does nsString.First() or whatever it is return in this case 0?

> > >+      else
> > >+        CharAt(adjustedOffset + 1, aText, aStartOffset, aEndOffset);
> > 
> > again comments would help ;)
> 
> "If caret at end of wrapped line (case of no line end character) then char
> after the offset is a first char at next line" ok?

editorial only, if the caret is at the end

Comment 44

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #43)

> I hate that this exists, seems like it would make a lot more sense to return
> '\n' even if its caused by line wrapping.

I follow Jamie's suggestion, see bunch of first comments for discussion

> > > can you add some explanation of why this makes sense?
> > 
> > does it work?
> > "Return no char if caret at the end of wrapped line (case of no line end
> > character)"
> 
> maybe say something about if its at start of next line, but otherwise fine

maybe add something like "Otherwise it would return a first char of next line as the caret was at start of next line"?

> > > >     case BOUNDARY_CHAR:
> > > >-      CharAt(convertedOffset + 1, aText, aStartOffset, aEndOffset);
> > > >+      if (adjustedOffset >= CharacterCount())
> > > >+        *aStartOffset = *aEndOffset = CharacterCount();
> > > 
> > > so has offset, but no text again seems kind of funny.
> > 
> > it's like a null char
> 
> yeah I guess, what does nsString.First() or whatever it is return in this
> case 0?

not sure I follow, it just returns an empty string

Comment 45

[alexander :surkov (:asurkov)]

Comment on attachment 8350326 [details] [diff] [review]
patch4: fix char part

http://hg.mozilla.org/integration/mozilla-inbound/rev/23dc9a53d818

Comment 46

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/23dc9a53d818