893166 - getTextBeforeOffset line end fails on wrapped lines

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

not super nice, obviously doesn't address all related problems, I bet there's similar thing for getTextAfterOffset, we just don't have a proper test.

I want to simplify atCaretOffset text testing to add more tests and I need to fix this todo for this. Later I think we will come up with some nicer code.

Comment 1

[Trevor Saunders (:tbsaunde)]

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

it seems like layout should be able to tell you what you need here with much less work, but this is probably fine for now.

>       *aEndOffset = FindLineBoundary(offset, ePrevLineEnd);
>-      *aStartOffset = FindLineBoundary(*aEndOffset, ePrevLineEnd);
>+      int32_t tmpOffset = // adjust offset if line is wrapped
>+        (*aEndOffset == 0 || IsTerminalCharAt(*aEndOffset)) ?
>+          *aEndOffset : *aEndOffset - 1;

this pretty funny especially the comment not on its own line. maybe

int tmp = *aEndOffset = = FindLineBoundary();
// hnalde soft line break blah blah
if (blah blah)
 tmp -= 1;

>+  bool IsTerminalCharAt(int32_t aOffset)

IsLineEndChar() would be more clear to me, terminal could mean all sorts of things \n \r \f \0 and probably other things

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 774867 [details] [diff] [review]
> patch
> 
> it seems like layout should be able to tell you what you need here with much
> less work, but this is probably fine for now.

same thinking but layout itself relies on \n character (for example http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#1490)

> >       *aEndOffset = FindLineBoundary(offset, ePrevLineEnd);
> >-      *aStartOffset = FindLineBoundary(*aEndOffset, ePrevLineEnd);
> >+      int32_t tmpOffset = // adjust offset if line is wrapped
> >+        (*aEndOffset == 0 || IsTerminalCharAt(*aEndOffset)) ?
> >+          *aEndOffset : *aEndOffset - 1;
> 
> this pretty funny especially the comment not on its own line. maybe
> 
> int tmp = *aEndOffset = = FindLineBoundary();
> // hnalde soft line break blah blah
> if (blah blah)
>  tmp -= 1;
> 
> >+  bool IsTerminalCharAt(int32_t aOffset)
> 
> IsLineEndChar() would be more clear to me, terminal could mean all sorts of
> things \n \r \f \0 and probably other things

ok

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

it seems we are ok with GetTextAfterOffset lineEnd

Comment 4

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/228b12161e1d

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/228b12161e1d