Closed Bug 1445954 Opened 4 years ago Closed 4 years ago

atk_text_get_get() return value incorrect after multibyte character, getText*Offset(TEXT_BOUNDARY_CHAR) part

Categories

(Core :: Disability Access APIs, defect)

58 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: samuel.thibault, Assigned: samuel.thibault)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180219150722

Steps to reproduce:

This is a follow-up to bug 1346535 , separated out to make patch reviewing simpler.
Attached patch getchar (obsolete) — Splinter Review
Here is the reworked getchar patch.

> it's not forbidden for sure, but I might be missing something, what's wrong with:

Nothing wrong, it is valid C99 code :) I just keep switching between different projects with very varying codestyles and backward-compatibility requirements.
Attachment #8959135 - Flags: review?(surkov.alexander)
Assignee: nobody → samuel.thibault
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch getchar (obsolete) — Splinter Review
Sorry, on this patch I forgot to go over the "nit" part, here it is fixed.
Attachment #8959135 - Attachment is obsolete: true
Attachment #8959135 - Flags: review?(surkov.alexander)
Attachment #8959168 - Flags: review?(surkov.alexander)
Comment on attachment 8959168 [details] [diff] [review]
getchar

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

::: accessible/atk/nsMaiInterfaceText.cpp
@@ +168,5 @@
> +{
> +  gint end = aOffset + 1;
> +  gint count = getCharacterCountCB(aText);
> +
> +  // Note: getTextBefore/At/AfterOffset do not have any magic offset.

nit: the method is ran under assumption that a caller doesn't pass magic offsets here

btw, what is a scope of magic offsets in ATK? are they used at all?

@@ +174,5 @@
> +  if (aOffset >= count) {
> +    aOffset = count - 1;
> +  }
> +  if (end >= count) {
> +    end = count - 1;

curious, why this one is count - 1, say if I have 'a' in text and I call getTextAtOffset(0), in this case end == 1 == count, and you end up with 'end = 0'?

@@ +185,5 @@
> +  }
> +  *aStartOffset = aOffset;
> +  *aEndOffset = end;
> +
> +  return getTextCB(aText, aOffset, end);

getTextCB may change aOffset and end offsets, and aStartOffset/aEndOffset should reflect those?

@@ +196,5 @@
>  {
>      nsAutoString autoStr;
>    int32_t startOffset = 0, endOffset = 0;
> +  if (aBoundaryType == ATK_TEXT_BOUNDARY_CHAR)
> +    return getCharTextAtOffset(aText, aOffset + 1, aStartOffset, aEndOffset);

nit: please new line after if, {} around it and move it before local variables
(In reply to alexander :surkov from comment #3)
> Comment on attachment 8959168 [details] [diff] [review]
> getchar
> 
> Review of attachment 8959168 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/atk/nsMaiInterfaceText.cpp
> @@ +168,5 @@
> > +{
> > +  gint end = aOffset + 1;
> > +  gint count = getCharacterCountCB(aText);
> > +
> > +  // Note: getTextBefore/At/AfterOffset do not have any magic offset.
> 
> nit: the method is ran under assumption that a caller doesn't pass magic
> offsets here

I'm not sure what you mean: do you mean the text should be replaced by what you propose?

> btw, what is a scope of magic offsets in ATK? are they used at all?

I'm not sure what you are asking, but I can say that the only magic offset I have seen in the at-spi documentation is getText's endOffset being -1 to conveniently request for end of text.

> @@ +174,5 @@
> > +  if (aOffset >= count) {
> > +    aOffset = count - 1;
> > +  }
> > +  if (end >= count) {
> > +    end = count - 1;
> 
> curious, why this one is count - 1, say if I have 'a' in text and I call
> getTextAtOffset(0), in this case end == 1 == count, and you end up with 'end
> = 0'?

Mmm, I'm sure I had tested this, but will check again.

> @@ +185,5 @@
> > +  }
> > +  *aStartOffset = aOffset;
> > +  *aEndOffset = end;
> > +
> > +  return getTextCB(aText, aOffset, end);
> 
> getTextCB may change aOffset and end offsets, and aStartOffset/aEndOffset
> should reflect those?

getTextCB does not change start and end offset. The only thing that could happen is that we are looking outside the string boundaries, and that is already handled by the ifs here.
(In reply to Samuel Thibault from comment #4)
> > @@ +174,5 @@
> > > +  if (aOffset >= count) {
> > > +    aOffset = count - 1;
> > > +  }
> > > +  if (end >= count) {
> > > +    end = count - 1;
> > 
> > curious, why this one is count - 1, say if I have 'a' in text and I call
> > getTextAtOffset(0), in this case end == 1 == count, and you end up with 'end
> > = 0'?
> 
> Mmm, I'm sure I had tested this, but will check again.

Ok, now I understand: I didn't realize that the webpage I was testing had space characters, which were bringing confusion. It should indeed be "> count" and "count".
Attached patch getcharSplinter Review
Here it is reworked, with what I assumed you meant about the comment.

Note: this is the version which assumes the multibyte patch already applied.  It is actually independent from it for building & working, it just happens that there is a small fuzz if applied on the current tip.
Attachment #8959168 - Attachment is obsolete: true
Attachment #8959168 - Flags: review?(surkov.alexander)
Attachment #8959509 - Flags: review?(surkov.alexander)
(In reply to Samuel Thibault from comment #4)

> > > +  return getTextCB(aText, aOffset, end);
> > 
> > getTextCB may change aOffset and end offsets, and aStartOffset/aEndOffset
> > should reflect those?
> 
> getTextCB does not change start and end offset. The only thing that could
> happen is that we are looking outside the string boundaries, and that is
> already handled by the ifs here.

I think I was confused by getTextCB which adjusts offsets before calling TextSubstring, so technically you may end up with a string that is larger than original offsets? I mean if I give (a, b) offsets and get 'bla' string, then is it possible to feed (a -1, b + 1) offsets for same string?
(In reply to alexander :surkov from comment #7)
> (In reply to Samuel Thibault from comment #4)
> 
> > > > +  return getTextCB(aText, aOffset, end);
> > > 
> > > getTextCB may change aOffset and end offsets, and aStartOffset/aEndOffset
> > > should reflect those?
> > 
> > getTextCB does not change start and end offset. The only thing that could
> > happen is that we are looking outside the string boundaries, and that is
> > already handled by the ifs here.
> 
> I think I was confused by getTextCB which adjusts offsets before calling
> TextSubstring, so technically you may end up with a string that is larger
> than original offsets? I mean if I give (a, b) offsets and get 'bla' string,
> then is it possible to feed (a -1, b + 1) offsets for same string?

No, because ATKStringConverterHelper::FinishUTF16toUTF8 strips the additional characters, so that getTextCB always exactly returns what was originally asked from at-spi (except on end of text, in which case there will be less).
Component: Untriaged → Disability Access APIs
Product: Firefox → Core
Attachment #8959509 - Flags: review?(surkov.alexander) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e703f7b735
atk: Fix getTextAfter/Before/AtOffset in character boundary case. r=surkov
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f2e703f7b735
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.