getTextBeforeOffset line end fails on wrapped lines

RESOLVED FIXED in mozilla25

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla25
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 774867 [details] [diff] [review]
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.
Attachment #774867 - Flags: review?(trev.saunders)
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
Attachment #774867 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 2

5 years ago
(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
(Assignee)

Comment 3

5 years ago
Created attachment 777918 [details] [diff] [review]
patch

it seems we are ok with GetTextAfterOffset lineEnd
Attachment #777918 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #777918 - Flags: review? → review?(trev.saunders)
Attachment #777918 - Flags: review?(trev.saunders) → review+

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/228b12161e1d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.