Closed Bug 612331 Opened 14 years ago Closed 11 years ago

mochitest multiple line for nsIAccessibleText

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: fherrera, Assigned: surkov)

References

Details

Attachments

(1 file, 9 obsolete files)

patch10
27.96 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
Attached patch patch1 (obsolete) — — Splinter Review 
      No description provided.
Comment on attachment 490618 [details] [diff] [review]
patch1

I have modified the API for testText as we have different results on different elements.
Also I'm assuming here that

"first line\nsecond line"

"\n" is the line break and then it is the end of the first line and the start of the second one.
Attachment #490618 - Flags: review?(surkov.alexander)
(In reply to comment #1)
> Comment on attachment 490618 [details] [diff] [review]
> patch1
> 
> I have modified the API for testText as we have different results on different
> elements.
> Also I'm assuming here that
> 
> "first line\nsecond line"
> 
> "\n" is the line break and then it is the end of the first line and the start
> of the second one.

it's the end of the current line but it's not the start of next line. And then why do keep '\n' in the start of next line?

You have a big test string here as well. Does it make sense of this? At the first glance it might be interesting to have string like
oneword\n
two words\n
three 3 words

if you're going to test offsets between words.

I really prefer to keep test string simple as possible.
Assignee: nobody → fherrera
Blocks: 452769
(In reply to comment #2)

> > "\n" is the line break and then it is the end of the first line and the start
> > of the second one.
> 
> it's the end of the current line but it's not the start of next line. And then
> why do keep '\n' in the start of next line?

So if it is not the start of the line GetTextAfterOffset(0, BOUNDARY_LINE_END) should return (the string from the line end at or after the offset to the next line start):
"\nthis is the second one\n"


> You have a big test string here as well. Does it make sense of this? At the
> first glance it might be interesting to have string like
> oneword\n
> two words\n
> three 3 words
> 
> if you're going to test offsets between words.
> 
> I really prefer to keep test string simple as possible.

Well, actually we are testing here mostly line boundaries like 0, 21, 22, 23, 44, 45, 46 and just something in the middle like 7 or 31, so it really doesn't make any difference how long are the lines or how many words they have. Anyway I can change it if you prefer so.
(In reply to comment #3)
> (In reply to comment #2)
> 
> > > "\n" is the line break and then it is the end of the first line and the start
> > > of the second one.
> > 
> > it's the end of the current line but it's not the start of next line. And then
> > why do keep '\n' in the start of next line?
> 
> So if it is not the start of the line GetTextAfterOffset(0, BOUNDARY_LINE_END)
> should return (the string from the line end at or after the offset to the next
> line start):
> "\nthis is the second one\n"

sounds correct but weird. But what does it make you think the line start/end are different from word start/end? I'm confused if line start and line end are the same offset.

> Well, actually we are testing here mostly line boundaries like 0, 21, 22, 23,
> 44, 45, 46 and just something in the middle like 7 or 31, so it really doesn't
> make any difference how long are the lines or how many words they have. Anyway
> I can change it if you prefer so.

yes, please, I don't like complex structures that aren't tested actually. Btw, it makes sense to test empty line.
(In reply to comment #3)

> So if it is not the start of the line GetTextAfterOffset(0, BOUNDARY_LINE_END)
> should return (the string from the line end at or after the offset to the next
> line start):
> "\nthis is the second one\n"

perhaps the spec should say "the returned string is from the line end at or after the offset to the next line __end__"? 
Then we should return ""\nthis is the second one" and it seems to be consistent to word boundaries.
(In reply to comment #4)
> > So if it is not the start of the line GetTextAfterOffset(0, BOUNDARY_LINE_END)
> > should return (the string from the line end at or after the offset to the next
> > line start):
> > "\nthis is the second one\n"
> 
> sounds correct but weird. But what does it make you think the line start/end
> are different from word start/end? I'm confused if line start and line end are
> the same offset.

As we don't have a clear definition for WORD_START/END nor LINE_START_END I took a look at the atk/gail implementation to check their results. For this example they return ('\nthis is the second one', 22, 45) and looking to the code it seems that they follow pango, the follows Unicode definitions. Unicode Anex#14 at http://unicode.org/reports/tr14/ says about line break:

LD2  Line Break:  The position in the text where one line ends and the next one starts.

so if we say that the position of "\n" is the line break that should be the line end and line start.

 
> > Well, actually we are testing here mostly line boundaries like 0, 21, 22, 23,
> > 44, 45, 46 and just something in the middle like 7 or 31, so it really doesn't
> > make any difference how long are the lines or how many words they have. Anyway
> > I can change it if you prefer so.
> 
> yes, please, I don't like complex structures that aren't tested actually. Btw,
> it makes sense to test empty line.

Ok, I'll use then:
oneword\n
\n
two words\n
three 3 words
btw, results of tests for div should depend on div size (screen resolution, firefox window size). Could we get rid this factor?
(In reply to comment #7)
> btw, results of tests for div should depend on div size (screen resolution,
> firefox window size). Could we get rid this factor?

Humm, they shouldn't as they are inside a <pre> so lines would not break because of window size
(In reply to comment #8)
> (In reply to comment #7)
> > btw, results of tests for div should depend on div size (screen resolution,
> > firefox window size). Could we get rid this factor?
> 
> Humm, they shouldn't as they are inside a <pre> so lines would not break
> because of window size

It makes sense.
(In reply to comment #6)

> LD2  Line Break:  The position in the text where one line ends and the next one
> starts.
> 
> so if we say that the position of "\n" is the line break that should be the
> line end and line start.

doesn't it make BOUNDARY_LINE_START/END return the same result because there's no difference between line start and line/end?

> Ok, I'll use then:
> oneword\n
> \n
> two words\n
> three 3 words

if you don't have special tests for in-the-middle of words then you can skip 'three 3 words'.
(In reply to comment #10)
> (In reply to comment #6)
> 
> > LD2  Line Break:  The position in the text where one line ends and the next one
> > starts.
> > 
> > so if we say that the position of "\n" is the line break that should be the
> > line end and line start.
> and 
> doesn't it make BOUNDARY_LINE_START/END return the same result because there's
> no difference between line start and line/end?
> 

Humm, true, and gail is returning here:
('this is the second one\n', 23, 46)

So maybe it makes sense to "\n" as line end and then assume a bug in atk documentation for atk_get_text_after_offset:
where it says:

"If the boundary_type is ATK_TEXT_BOUNDARY_LINE_END the returned string is from the line end at or after the offset to the next line start."

should say:
"If the boundary_type is ATK_TEXT_BOUNDARY_LINE_END the returned string is from the line end at or after the offset to the next line end."


that also makes perfect sense, because get_text_at_offset and get_text_before offset are both from start to start for BOUNDARY_LINE_STAR and from end to end for BOUNDARY_LINE_END

> > Ok, I'll use then:
> > oneword\n
> > \n
> > two words\n
> > three 3 words
> 
> if you don't have special tests for in-the-middle of words then you can skip
> 'three 3 words'.

ok.
(In reply to comment #11)

> 
> So maybe it makes sense to "\n" as line end and then assume a bug in atk
> documentation for atk_get_text_after_offset:
> where it says:
> 
> "If the boundary_type is ATK_TEXT_BOUNDARY_LINE_END the returned string is from
> the line end at or after the offset to the next line start."
> 
> should say:
> "If the boundary_type is ATK_TEXT_BOUNDARY_LINE_END the returned string is from
> the line end at or after the offset to the next line end."

yes, that's what I said in comment #5. Please discuss this with Li in that atk bug.

> Humm, true, and gail is returning here:
> ('this is the second one\n', 23, 46)

but then it should be '\nthis is the second one'.
(In reply to comment #12)

> 
> > Humm, true, and gail is returning here:
> > ('this is the second one\n', 23, 46)
> 
> but then it should be '\nthis is the second one'.

'\nthis is the second one' is the result for LINE_END
'this is the second one\n' is the result for LINE_START
Documentation bug filled at:
https://bugzilla.gnome.org/show_bug.cgi?id=635069
(In reply to comment #13)

> '\nthis is the second one' is the result for LINE_END
> 'this is the second one\n' is the result for LINE_START

correct.
Attached patch patch2 (obsolete) — — Splinter Review 
Updated patch testing simpler strings and empty lines
Attachment #490618 - Attachment is obsolete: true
Attachment #491224 - Flags: review?(surkov.alexander)
Attachment #490618 - Flags: review?(surkov.alexander)
please add testTextAfterOffset(19, BOUNDARY_LINE_START, "", 19, 19). Technically we have four lines in the example and we should test all of them.
testTextAfterOffset(0, BOUNDARY_LINE_END, "\n", 7, 9,

should return 7, 8
the same for testTextAfterOffset(7, BOUNDARY_LINE_END, "\n", 7, 9,

testTextAfterOffset(8, BOUNDARY_LINE_END, "\ntwo words", 8, 18,
should return (\n, 8, 9) because 8 is line end of empty line

please add tests for testTextAfterOffset(18 and 19, BOUNDARY_LINE_END) 

please add test for testTextBeforeOffset(19, BOUNDARY_WORD_START,

please add test for testTextBeforeOffset(18, BOUNDARY_LINE_START

testTextBeforeOffset(8, BOUNDARY_LINE_END, "", 0, 0,
should return 'oneword'

// FIXME: Should 19 be undefined as it is out of the boundaries?
testTextBeforeOffset(19, BOUNDARY_LINE_END, "\ntwo words", 8, 18,
that's correct, 19 is offset of empty line

 testTextAtOffset(13, BOUNDARY_WORD_END, " words", 7, 18,
wrong start offset

add tests for 
testTextAtOffset(7, BOUNDARY_LINE_START,
testTextAtOffset(18 and 19, BOUNDARY_LINE_START
testTextAtOffset(19, BOUNDARY_LINE_END,

href="https://bugzilla.mozilla.org/show_bug.cgi?id=452769">Mozilla Bug 452769</a>
wrong bug number

we should finish singleline test before or put signleline test on top of this one.
Comment on attachment 491224 [details] [diff] [review]
patch2

please file next patch
Attachment #491224 - Flags: review?(surkov.alexander)
(In reply to comment #19)

> testTextAfterOffset(8, BOUNDARY_LINE_END, "\ntwo words", 8, 18,
> should return (\n, 8, 9) because 8 is line end of empty line

so "from the line end at or after the offset to the next line end" should be from 8 (line end at the offset) to 18 (the next line end)


> we should finish singleline test before or put signleline test on top of this
> one.

you mean whitespaces?
(In reply to comment #21)
> (In reply to comment #19)
> 
> > testTextAfterOffset(8, BOUNDARY_LINE_END, "\ntwo words", 8, 18,
> > should return (\n, 8, 9) because 8 is line end of empty line
> 
> so "from the line end at or after the offset to the next line end" should be
> from 8 (line end at the offset) to 18 (the next line end)

you're right.

> 
> > we should finish singleline test before or put signleline test on top of this
> > one.
> 
> you mean whitespaces?

yes, the patch that precedes this one
Attached patch patch3 (obsolete) — — Splinter Review 
Updated with previous comments
Attachment #491224 - Attachment is obsolete: true
Attachment #491541 - Flags: review?(surkov.alexander)
Attached patch patch4 (obsolete) — — Splinter Review 
Updated patch: rebase on top of the latest patch at bug #610568 and fix kOk/kTodo results.
Attachment #491541 - Attachment is obsolete: true
Attachment #491600 - Flags: review?(surkov.alexander)
Attachment #491541 - Flags: review?(surkov.alexander)
Comment on attachment 491600 [details] [diff] [review]
patch4


>+/**
>+ * Test getText function over different elements

please put '.' in the end of sentence.

>+ *
>+ * @param aStartOffset    [in] the start offset for the text to get
>+ * @param aEndOffset      [in] the end offset for the text to get
>+ * @param aText           [in] expected return text for getTextAtOffset
>+ * @param ...             [in] list of pairs made of:
>+ *                              element identifier
>+ *                              kTodo or kOk for returned text
>+ *          

you don't need empty line in comment, at least it shouldn't contain whitespaces in the end

r=me
Attachment #491600 - Flags: review?(surkov.alexander) → review+
you know I think 

testTextAtOffset(19, BOUNDARY_LINE_START, "two words\n", 9, 19,

should return "", 19, 19 because 19 offset is line start of new empty line.

but the spec is confusing "the returned string is from the line start at or before the offset to the line start after the offset. " because there's no offset after 19 offset.
(In reply to comment #26)
> you know I think 
> 
> testTextAtOffset(19, BOUNDARY_LINE_START, "two words\n", 9, 19,
> 
> should return "", 19, 19 because 19 offset is line start of new empty line.
> 
> but the spec is confusing "the returned string is from the line start at or
> before the offset to the line start after the offset. " because there's no
> offset after 19 offset.

Yeah, the spec doesn't say anything about positions after the boundaries. I have been interpreting them as ends but not as starts, that why I put "two words\n"
But you can have a caret after \n, i.e. on new line or at 22 offset. If you get textAtOffset then I would expect to get the line the caret is in this case. Would you?
Attached patch patch5 (obsolete) — — Splinter Review 
Updated with last minor comments.
Attachment #491600 - Attachment is obsolete: true
(In reply to comment #28)
> But you can have a caret after \n, i.e. on new line or at 22 offset. If you get
> textAtOffset then I would expect to get the line the caret is in this case.
> Would you?

19 offset, 22 number I took from different patch I guess :)
(In reply to comment #30)
> (In reply to comment #28)
> > But you can have a caret after \n, i.e. on new line or at 22 offset. If you get
> > textAtOffset then I would expect to get the line the caret is in this case.
> > Would you?
> 
> 19 offset, 22 number I took from different patch I guess :)


Humm, if this is the last element, you cannot move the caret beyond the last \n, so it is a tricky answer
(In reply to comment #31)

> Humm, if this is the last element, you cannot move the caret beyond the last
> \n, so it is a tricky answer

Why not. Let's you have textarea where there is only '\n', the caret can be before it and after it, i.e. on the first line and on second. Isn't it?
Well, you can do it only in a textarea, and that is because textarea add an extra \n to the accessible text. For example with this html:

<html>
<head>
</head>
<body>
<pre><textarea id="div">b
</textarea></pre>
</body>
</html>

what we really have inside the text area is "b\n", however inspecting via AT APIs:
In [2]: t = acc.queryText()
In [3]: t.getText(0,-1)
Out[3]: 'b\n\n'

and that why we can go after the first \n, but not in other situations (like a plain div)
I'm not sure we talk about the same thing.

Let's you have content editable div, then type letter and press enter then you get two lines. This editable div should have two characters. If you move caret on second line then the caret offset should be 2 (0 - before letter, 1 - after letter on first line, 2 - on second line). So when you're on second line then you suggest to return first line as result of getTextAt for line boundary, I think this is not expected and the second line should be returned.
Yes, and this is where textaread and editable divs do the trick.

* Having this initial html:
<pre><div contenteditable="true">start</div></pre>
* Query its text:
In [1]: t = acc.queryText()
In [2]: t.getText(0,-1)
Out[2]: 'start'
* place the cursor after 't' and press enter
* query again its text:
In [3]: t.getText(0,-1)
Out[3]: 'start\n\n'

so in order to be able to move after the last char (the initial \n) gecko adds an extra \n and the end of the element, to have the cursor between them.
Ok, if that's not about accessible text only, i.e. if it's appended by layout/content modules then isn't it applied to your testcase?
ok, so I'll test separately those cases (textarea and editable) where extra '\n' is added and we are checking total text, char counts and end boundaries
(In reply to comment #37)
> ok, so I'll test separately those cases 

what do you mean? in this bug/patch or somewhere else? if somewhere else then what about testTextAtOffset(19, BOUNDARY_LINE_START, "two words\n", 9, 19) ?
I mean, with this test case, for the div element we get:
In [2]: t.getText(0,-1)
Out[2]: 'oneword\n\ntwo words\n

and for the textarea get get:
In [10]: t.getText(0,-1)
Out[10]: 'oneword\n\ntwo words\n\n'

so we need to split out test like this:

-     testText(0, 19, "oneword\n\ntwo words\n",
-               "div", kOk,
-               "divbr", kOk,
-               "editable", kOk,
-               "editablebr", kOk,
-               "textarea", kOk);

+     testText(0, 19, "oneword\n\ntwo words\n",
+               "div", kOk,
+               "divbr", kOk,
+               "editable", kOk,
+               "editablebr", kOk);
+
+     testText(0, 20, "oneword\n\ntwo words\n\n",
+              "textarea", kOk);

...

-      testTextAfterOffset(9, BOUNDARY_WORD_START, "words\n", 13, 19,
-                          "div", kTodo, kTodo, kTodo,
-                          "divbr", kTodo, kTodo, kTodo,
-                          "editable", kTodo, kTodo, kTodo,
-                          "editablebr", kTodo, kTodo, kTodo,
-                          "textarea", kTodo, kTodo, kTodo);

+      testTextAfterOffset(9, BOUNDARY_WORD_START, "words\n", 13, 19,
+                          "div", kTodo, kTodo, kTodo,
+                          "divbr", kTodo, kTodo, kTodo,
+                          "editable", kTodo, kTodo, kTodo,
+                          "editablebr", kTodo, kTodo, kTodo)
+
+      testTextAfterOffset(9, BOUNDARY_WORD_START, "words\n\n", 13, 20,
+                          "textarea", kTodo, kTodo, kTodo);

and so on.
then for testTextAtOffset(19, BOUNDARY_LINE_START) I woud say it is:
 testTextAtOffset(19, BOUNDARY_LINE_START,"two words\n", 9, 19,
                  "div", kTodo, kTodo, kTodo,
                  "divbr", kTodo, kTodo, kTodo,
                  "editable", kTodo, kTodo, kTodo,
                  "editablebr", kTodo, kTodo, kTodo);
and
 testTextAtOffset(19, BOUNDARY_LINE_START,"\n", 19, 20,
                  "textarea", kTodo, kTodo, kTodo);
looks like you're right
Attached patch patch6 (obsolete) — — Splinter Review 
Updated patch splitting textarea tests for those texts where the end boundary matters (as it add an extra \n)
Attachment #491835 - Attachment is obsolete: true
Attachment #492447 - Flags: review?(surkov.alexander)
Comment on attachment 492447 [details] [diff] [review]
patch6


>+      testText(0, 19, "oneword\n\ntwo words\n",
>+               "div", kOk,
>+               "divbr", kOk,
>+               "editable", kOk,
>+               "editablebr", kOk,
>+               "textarea", kOk);
>+      testText(0, 19, "oneword\n\ntwo words\n",
>+               "div", kOk,
>+               "divbr", kOk,
>+               "editable", kOk,
>+               "editablebr", kOk,
>+               "textarea", kOk);


should be 0, 20 test for textarea, right?

>+      testTextBeforeOffset(19, BOUNDARY_LINE_START, "\n", 8, 9,
>+                           "div", kTodo, kTodo, kTodo,
>+                           "divbr", kTodo, kTodo, kTodo,
>+                           "editable", kTodo, kTodo, kTodo,
>+                           "editablebr", kTodo, kTodo, kTodo,
>+                           "textarea", kOk, kTodo, kTodo);

testTextBeforeOffset(20 for textarea should return "twowords" line?


>+      testTextAtOffset(9, BOUNDARY_LINE_START, "two words\n", 9, 19,
>+                       "div", kTodo, kOk, kTodo,
>+                       "divbr", kOk, kOk, kOk,
>+                       "editable", kTodo, kOk, kTodo,
>+                       "editablebr", kOk, kOk, kOk);
>+      testTextAtOffset(9, BOUNDARY_LINE_START, "two words", 9, 18,
>+		       "textarea", kOk, kOk, kOk);

>+      testTextAtOffset(13, BOUNDARY_LINE_START, "two words\n", 9, 19,
>+                       "div", kTodo, kOk, kTodo,
>+                       "divbr", kOk, kOk, kOk,
>+                       "editable", kTodo, kOk, kTodo,
>+                       "editablebr", kOk, kOk, kOk)
>+      testTextAtOffset(13, BOUNDARY_LINE_START, "two words", 9, 18,
>+		       "textarea", kOk, kOk, kOk);

Why don't you include trail '\n', since '\n' isn't a new line start, right?

>+      testTextAtOffset(19, BOUNDARY_LINE_START, "two words\n", 9, 19,
>+                       "div", kTodo, kOk, kTodo,
>+                       "divbr", kOk, kOk, kOk,
>+                       "editable", kTodo, kOk, kTodo,
>+                       "editablebr", kOk, kOk, kOk);
>+      testTextAtOffset(19, BOUNDARY_LINE_START, "\n", 19, 20,
>+		       "textarea", kOk, kOk, kOk);

19 is after \n character, so there's no line at 19 offset, it should return (19, 19, ""). I think the similar case for "word " string, textAtOffset(WORD_START), offset after " " char - I think it shouldn't return "word ".

>       ////////////////////////////////////////////////////////////////////////
>       // getText
> 
>-      testText(IDs, 0, 1, "h");
>-      testText(IDs, 1, 3, "el");
>-      testText(IDs, 14, 15, "d");
>-      testText(IDs, 0, 15, "hello my friend");
>+      testText(0, 1, "h",
>+               "input", kOk,
>+               "div", kOk,
>+               "editable", kOk,
>+               "textarea", kOk);

could we have two version of testText, I like IDs as an array while we don't need kTodo/kOk stuffs?

I think I need to take another pass.
Attachment #492447 - Flags: review?(surkov.alexander)
Attached patch patch7 (obsolete) — — Splinter Review 
I have updated the patch fixing the stuff.

I've kept a single testText funtion taking an array of elements and the kOk as optional as we can do textarea case like this:
testTest(["textarea"], ..., kOk);
Attachment #492447 - Attachment is obsolete: true
Attachment #493048 - Flags: review?(surkov.alexander)
var IDs = ["div", "divbr", "editable", "editablebr"];

move this where it's used, i.e. before testText.

// Textarea elements adds an extra "\n" at the end of the text
// at the layount/content level. That is pos 19.

you should move this comment after test string before any test, since it's common.

testTextAfterOffset(19, BOUNDARY_LINE_END, "\n", 20, 20,

should be 19 start offset

>+      testTextBeforeOffset(19, BOUNDARY_LINE_START, "\n", 8, 9,
issue from previous comment isn't addressed. At least it isn't true for textarea since 19 is start offset of forth line there.

Btw, the similar case we have in singleline test:

__h__e__l__l__o__ __m__y__ __f__r__i__e__n__d__
testTextBeforeOffset(15, BOUNDARY_WORD_START, "my ", 6, 9,
testTextBeforeOffset(6, BOUNDARY_WORD_END, "hello ", 0, 6,

which makes me feel as they're inconsistent since 15 offset isn't treated as word start but 0 offset is treated as word end.

the same we have here
> testTextBeforeOffset(19, BOUNDARY_LINE_START, "\n", 8, 9,
19 isn't line start but
testTextBeforeOffset(8, BOUNDARY_LINE_END, "oneword", 0, 7,
0 offset is line end

Note, the test testTextAtOffset(9, BOUNDARY_LINE_START, "two words\n", 9, 19,

treats 19 as line start since "to the line start after the offset."
> testText(IDs, 0, 1, "h", kOk);

I suggest to use kOk if 4th argument is missed.
Attachment #493048 - Flags: review?(surkov.alexander)
(In reply to comment #44)

> __h__e__l__l__o__ __m__y__ __f__r__i__e__n__d__
> testTextBeforeOffset(15, BOUNDARY_WORD_START, "my ", 6, 9,
> testTextBeforeOffset(6, BOUNDARY_WORD_END, "hello ", 0, 6,
> 
> which makes me feel as they're inconsistent since 15 offset isn't treated as
> word start but 0 offset is treated as word end.
> 
> the same we have here
> > testTextBeforeOffset(19, BOUNDARY_LINE_START, "\n", 8, 9,
> 19 isn't line start but
> testTextBeforeOffset(8, BOUNDARY_LINE_END, "oneword", 0, 7,
> 0 offset is line end
> 
> Note, the test testTextAtOffset(9, BOUNDARY_LINE_START, "two words\n", 9, 19,
> 
> treats 19 as line start since "to the line start after the offset."

Humm, so could we say here that for statements like:
"From the char/word start/word_end/line_start/line_end at/before/after the offset"
we can assume:
"From the char/word start/word_end/line_start/line_end at/before/after the offset or the beginning of the text if there is not such boundary"

but not for the "to" part of the statments?
Attached patch patch8 (obsolete) — — Splinter Review 
Update patch addressing all but the last (open question) comment
Attachment #493048 - Attachment is obsolete: true
Attachment #493349 - Flags: review?(surkov.alexander)
(In reply to comment #46)

> Humm, so could we say here that for statements like:
> "From the char/word start/word_end/line_start/line_end at/before/after the
> offset"
> we can assume:
> "From the char/word start/word_end/line_start/line_end at/before/after the
> offset or the beginning of the text if there is not such boundary"
> 
> but not for the "to" part of the statments?

hm, at some point it should be applicable to "to" part, for example, if you're going to apply this for
testTextBeforeOffset(19, BOUNDARY_LINE_START, "", 19, 19)

otherwise there's no "to". But you should discuss this with Li I think.
Fernando, ping on this, what's result of discussion?
Comment on attachment 493349 [details] [diff] [review]
patch8

cancelling review until comment 48-49 are addressed
Attachment #493349 - Flags: review?(surkov.alexander)
Attached patch patch9 (obsolete) — — Splinter Review 
Updated patch including test for softline breaks.

Notice that I can control where the lines are broken for the textarea case using cols="XX" but breaking lines on regular div elements using style="size:XXem" would depend on font and window size. Hopefuly on the test machines would be the same.
Attachment #493349 - Attachment is obsolete: true
Assignee: fherrera → nobody
Attached patch patch10 — — Splinter Review 
be careful
Assignee: nobody → surkov.alexander
Attachment #536679 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #722132 - Flags: review?(trev.saunders)
Comment on attachment 722132 [details] [diff] [review]
patch10

It might be nice if the char boundary tests covered something other than new lines so we could be more confident they were etting the text at the right position, but otherwise this all looks fine.

I'm hoping you fix all the todos, which should give us even more confidence that the tests match the spec if the implementation is simple and clear.
Attachment #722132 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #53)
> Comment on attachment 722132 [details] [diff] [review]
> patch10
> 
> It might be nice if the char boundary tests covered something other than new
> lines so we could be more confident they were etting the text at the right
> position, but otherwise this all looks fine.

do you mean lines other than first line (since it's covered by test_singleline.hml)?
(In reply to alexander :surkov from comment #54)
> (In reply to Trevor Saunders (:tbsaunde) from comment #53)
> > Comment on attachment 722132 [details] [diff] [review]
> > patch10
> > 
> > It might be nice if the char boundary tests covered something other than new
> > lines so we could be more confident they were etting the text at the right
> > position, but otherwise this all looks fine.
> 
> do you mean lines other than first line (since it's covered by
> test_singleline.hml)?

that might be useful, but probably not terribly important accept maybe textfield case where some content might not be visible?
(In reply to Trevor Saunders (:tbsaunde) from comment #56)

> that might be useful, but probably not terribly important accept maybe
> textfield case where some content might not be visible?

a long and winding road that leads to sane text implementation will never disappear.

Yes, I'll keep that in mind making a text surgery.
https://hg.mozilla.org/mozilla-central/rev/0c6630dfce4c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
> +      SimpleTest.expectAssertions(60);

Please include a bug number when you disable or weaken a test.
(In reply to Jesse Ruderman from comment #59)
> > +      SimpleTest.expectAssertions(60);
> 
> Please include a bug number when you disable or weaken a test.

that's a totally new test file, so its improving things :-)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: