Arthit Suriyawongkul Reporter
	Description • 22 years ago

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.0rc3) Gecko/20020614
Netscape/7.0b1
BuildID:    2002071422

do editing Thai text in any text edit area.

when press <delete>, a display cell after (right to) caret should be deleted -
by WHOLE display cell.

Reproducible: Always

Steps to Reproduce:
1. open attached testcase (delcell.html) in Composer.
   make sure charset setted to TIS-620,
   you will see one line of Thai text.
2. when a caret is at the beginning of text.
   press <delete> seven times


Actual Results:
some text still remains.

when press <delete>, Thai text was not deleted by display cell,
it was deleted by character.



Expected Results:
all the text should be deleted out, and left nothing.

when press <delete>, Thai text should be deleted by display cell.


----

fyi,
in Thai writing system, there're 4 display-levels of character.
from highest to lowest: top, above, base, below.

several Thai characters with different display-levels can be composed together.
we call the composed unit a "display cell".

----

for Thai text editing, we expected

<delete> = delete whole display cell after (right to) caret
<backspace> = delete only one character before (left to) caret

----

bug occurs in Mozilla nightly build 2002071422, Netscape 7.0_03
on Solaris 8

	Roy Yokoyama
	Comment 2 • 22 years ago

katakai-san: can you help us out ?

	Arthit Suriyawongkul Reporter
	Comment 3 • 22 years ago

also occurs in Windows XP

	Arthit Suriyawongkul
	Comment 4 • 22 years ago

since bug 133212 "--enable-ctl bustage" has been fixed.

could anyone help review Theppitak's patch pls? thx :)

	Arthit Suriyawongkul
	Comment 5 • 22 years ago

pls ignore comment #4, it's a wrong post. Sorry.

	Roland Mainz
	Comment 6 • 22 years ago

Over to new bugzilla component "Complex Text Layout" ...

	Masaki Katakai
	Comment 7 • 22 years ago

Confirmed. Actually the same behavior with backspace.

Prabhat/Theppitak,

Can you work on this bug?

	James Clark
	Comment 8 • 22 years ago

Just to reiterate what Artit said above, backspace is correct as it is. 
Backspace should NOT delete the whole cell, only the last combining character.

This might seem strangely asymmetrical, but in fact it makes perfect sense when
you consider the reason why delete should delete the entire cell (i.e. the base
char plus following combining chars).  The fundamental reason is that the caret
must always be on cluster boundaries (i.e. not between a base char and a
following combining char).  Suppose you have

 b1 | b2 c

where b1, b2 are base chars and c is a combining char and | is the caret, and
you hit delete.  If you just delete b2 and not c, then you have to end up with
one of

(1) b1 | c
(2) | b1 c
(3) b1 c |

(1) is no good because the caret is not on a cluster boundary; (2) and (3) are
no good because delete has moved the caret (i.e. the char to the left of | has
changed). With backspace this reasoning does not apply: the natural, simple
behaviour of deleting the last combining char does not cause any problems.

I don't think there's anything Thai-specific about this.  I believe delete
should always delete the entire following cluster whenever you are maintaining
the caret on cluster boundaries.  And I think you have to maintain the caret on
cluster boundaries unless you have some way to visually indicate that you are in
the middle of a cluster. (Maybe in Arabic you need this?)

On Windows, Uniscribe exhibits the behaviour I've described not just for Thai,
but also for Western chars. You can see this if you use Wordpad and then paste
in a combining char (e.g. 0x301 combining acute) with the character map accessory.

	Masaki Katakai
	Comment 9 • 22 years ago

Prabhat,

Could you review James' comment?

	Arthit Suriyawongkul
	Comment 10 • 20 years ago

still not fixed in Mozilla nightly 200405
(as well as Firefox 0.8)

	Karl Hong
	Comment 11 • 20 years ago

This patch is to handle 'delete' key to delete CTL, like thai, cell. As
mentioned in above from other people, delete key should delet whole cell, it is
similar to delete word, we have to find cell's boundary.

The patch adds a case, similar to delete word by WordMove(), delete cell by
CharacterMove(), which contains CTL logic to find cell's boundary.

	Karl Hong
	Comment 12 • 20 years ago

This patch is same with previous one, but the change is inclosed in SUNCTL
macro, since the CTL logic in CharacterMove() is also inclosed in same macro.
That will reduce the impact on other distributions.

	Arthit Suriyawongkul
	Comment 13 • 17 years ago

On Firefox 2.0.0.11 and Firefox 3.0b3pre, this bug still remains.

	Arthit Suriyawongkul
	Comment 14 • 17 years ago

(In reply to comment #13)
> On Firefox 2.0.0.11 and Firefox 3.0b3pre, this bug still remains.

Tested on Ubuntu "Hardy Heron" 8.04 Alpha 4.

	Theppitak Karoonboonyanan Assignee
	Comment 15 • 17 years ago

On Linux, even with 1.8 branch, I find caret movements are already cell-based, that is, combining characters are already skipped. Probably, what we need is just a small adjustment in the "<DEL>" key handling?

	Theppitak Karoonboonyanan Assignee
	Comment 16 • 17 years ago

Some changes in trunk have obsoleted Karl's proposed patches in comment #11 and comment #12.

Looking at nsISelectionController implementation in nsFramdSelection [layout/generic/nsSelection.cpp] via delegation from nsTextInputSelectionImpl [layout/forms/nsTextControlFrame.cpp], I find the meanings of DELETE and BACKSPACE have been made different from LEFT and RIGHT. So, should we add cellExtendForDelete() method, to do stuffs like wordExtendForDelete() for word deletion?

(Another class affected by this adding would be PresShell [layout/base/nsPresShell.cpp].)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 17 years ago

I haven't really thought about what to do here, sorry. I presume the remaining issue is to make backspace and delete delete a cluster-at-a-time?

	Arthit Suriyawongkul
	Comment 18 • 17 years ago

(In reply to comment #17)
> I haven't really thought about what to do here, sorry. I presume the remaining
> issue is to make backspace and delete delete a cluster-at-a-time?

Only DELETE key deletes the whole cluster, Robert.

BACKSPACE key deletes the last combining character.

(see bug description and James Clark's comment #8)

	Karl Hong
	Comment 19 • 17 years ago

I am no longer working on this area, no access the code, could someone else takes over the bug?

	Theppitak Karoonboonyanan Assignee
	Comment 20 • 17 years ago

So, nsPlainTextEditor::DeleteSelection() will only extend selection for aAction == eNext, but not for ePrevious. The added method may not take the direction parameter.

Should I regenerate uuid for nsISelectionController as well?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 21 • 17 years ago

yes, definitely

	Theppitak Karoonboonyanan Assignee
	Comment 23 • 17 years ago

Comment on attachment 302766 [details] [diff] [review]
Patch with regenerated uuid

May I request for a review?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 24 • 17 years ago

Comment on attachment 302766 [details] [diff] [review]
Patch with regenerated uuid

a=beltzner for 1.9

	Reed Loden [:reed]
	Comment 25 • 17 years ago

Checking in content/base/public/nsISelectionController.idl;
/cvsroot/mozilla/content/base/public/nsISelectionController.idl,v  <--  nsISelectionController.idl
new revision: 3.24; previous revision: 3.23
done
Checking in editor/libeditor/text/nsPlaintextEditor.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsPlaintextEditor.cpp,v  <--  nsPlaintextEditor.cpp
new revision: 1.110; previous revision: 1.109
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.1088; previous revision: 3.1087
done
Checking in layout/forms/nsTextControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsTextControlFrame.cpp,v  <--  nsTextControlFrame.cpp
new revision: 3.277; previous revision: 3.276
done
Checking in layout/generic/nsFrameSelection.h;
/cvsroot/mozilla/layout/generic/nsFrameSelection.h,v  <--  nsFrameSelection.h
new revision: 1.22; previous revision: 1.21
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.298; previous revision: 3.297
done

	Steve England [:stevee]
	Comment 26 • 17 years ago

I think this caused bug 417745

	Reed Loden [:reed]
	Comment 27 • 17 years ago

Backed out due to the regression in bug 417745.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 28 • 17 years ago

If/when this gets re-fixed, we need a mochitest for this bug and also bug 417745.

	Theppitak Karoonboonyanan Assignee
	Comment 29 • 17 years ago

I couldn't reproduce bug 417745 on Linux. But I believe the line removed in this patch was the culprit.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 30 • 17 years ago

Could you make some mochitests for this bug and bug 417745? You could use layout/generic/test/test_word_movement.html
as an example to base the tests on.

	Theppitak Karoonboonyanan Assignee
	Comment 31 • 17 years ago

Sorry, I have no idea what mochitests mean. And, looking at the sample file, it requires JavaScript skills, which I don't have.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 17 years ago

It's not that hard, really :-). If you can figure out content/ and layout/ you can figure out Mochitests :-).

	Theppitak Karoonboonyanan Assignee
	Comment 33 • 17 years ago

OK. I finally come up with this mochitest. It's my first mochitest as well as my first JavaScript coding. So, I submit it for your comment first, before including it into the patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 34 • 17 years ago

It mostly looks great, except that you're testing things a bit indirectly ... if something went wrong in testDelete or testBackspace, you'd only detect it some time later when you found the caret at the wrong offset in the text. I think it would be better to have testDelete and testBackspace take an extra parameter which is the expected value of editor.innerHTML after the delete operation.

	Theppitak Karoonboonyanan Assignee
	Comment 35 • 17 years ago

Thanks for the comment. So, I have included the modified mochikit into the patch, for your checking convenience.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 36 • 17 years ago

+  testWordSelRight(editor.firstChild, 0, afterEditorNode, 0);
+  testDelete(editor.firstChild, 0, "ox");

Why doesn't this test delete all the text? Seems like it should.

+  testWordSelRight(editor.firstChild, 0, editor.firstChild, 3);
+  testDelete(editor.firstChild, 0, "fox");

Same here.

	Theppitak Karoonboonyanan Assignee
	Comment 37 • 17 years ago

(In reply to comment #36)
> +  testWordSelRight(editor.firstChild, 0, afterEditorNode, 0);
> +  testDelete(editor.firstChild, 0, "ox");
> 
> Why doesn't this test delete all the text? Seems like it should.

For this one, it's because the eatSpace flag makes the selection spill over to the "Catch-all" node, which is not editable. I think the last test should not be done at all. 

> +  testWordSelRight(editor.firstChild, 0, editor.firstChild, 3);
> +  testDelete(editor.firstChild, 0, "fox");
> 
> Same here.

This is a real bug. DeleteSelection() just blindly extends the selection even if the selection is not collapsed. So, this causes the selection to cross to the "Catch-all" node again, and not deleted.

This bug also causes the wierd check strings like "ellow" and "ox". I'll fix this in the next patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 39 • 17 years ago

+  testDelete(editor, 0, "<br>");

I think we're testing too much here. We might want to change the editor to not add a <br> in this situation. If you test editor.textContent instead of editor.innerHTML, can you just test against "" OK?

	Theppitak Karoonboonyanan Assignee
	Comment 40 • 17 years ago

OK. Updated.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 41 • 17 years ago

Comment on attachment 303981 [details] [diff] [review]
Patch testing textContent instead of innerHTML in mochitest

great!

	Reed Loden [:reed]
	Comment 42 • 17 years ago

Checking in content/base/public/nsISelectionController.idl;
/cvsroot/mozilla/content/base/public/nsISelectionController.idl,v  <--  nsISelectionController.idl
new revision: 3.26; previous revision: 3.25
done
Checking in editor/libeditor/text/nsPlaintextEditor.cpp;
/cvsroot/mozilla/editor/libeditor/text/nsPlaintextEditor.cpp,v  <--  nsPlaintextEditor.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.1092; previous revision: 3.1091
done
Checking in layout/forms/nsTextControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsTextControlFrame.cpp,v  <--  nsTextControlFrame.cpp
new revision: 3.279; previous revision: 3.278
done
Checking in layout/generic/nsFrameSelection.h;
/cvsroot/mozilla/layout/generic/nsFrameSelection.h,v  <--  nsFrameSelection.h
new revision: 1.24; previous revision: 1.23
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.300; previous revision: 3.299
done
Checking in layout/generic/test/Makefile.in;
/cvsroot/mozilla/layout/generic/test/Makefile.in,v  <--  Makefile.in
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/layout/generic/test/test_backspace_delete.html,v
done
Checking in layout/generic/test/test_backspace_delete.html;
/cvsroot/mozilla/layout/generic/test/test_backspace_delete.html,v  <--  test_backspace_delete.html
initial revision: 1.1
done

	Reed Loden [:reed]
	Comment 43 • 17 years ago

Backed out due to test failures.

*** 19209 ERROR FAIL | Delete broken in "สัสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "สัสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "สสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19214 ERROR FAIL | Delete broken in "สัดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "สัดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "สสพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19216 ERROR FAIL | Right movement broken in "สัดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 3, expected 4 | /tests/layout/generic/test/test_backspace_delete.html
*** 19218 ERROR FAIL | Delete broken in "สัดพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 3, expected 4 | /tests/layout/generic/test/test_backspace_delete.html
*** 19219 ERROR FAIL | Delete broken in "สัดพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "สัดพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "สสพ่à¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19221 ERROR FAIL | Right movement broken in "สัดพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 4, expected 5 | /tests/layout/generic/test/test_backspace_delete.html
*** 19223 ERROR FAIL | Delete broken in "สัดพอà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 4, expected 5 | /tests/layout/generic/test/test_backspace_delete.html
*** 19224 ERROR FAIL | Delete broken in "สัดพอà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "สัดพอà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "สสพ่à¹à¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19226 ERROR FAIL | Right movement broken in "สัดพอà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 5, expected 8 | /tests/layout/generic/test/test_backspace_delete.html
*** 19228 ERROR FAIL | Delete broken in "สัดพอม่พี่น้อง" | got 5, expected 8 | /tests/layout/generic/test/test_backspace_delete.html
*** 19229 ERROR FAIL | Delete broken in "สัดพอม่พี่น้อง" | got "สัดพอม่พี่น้อง", expected "สสพ่à¹à¸žà¸µà¹ˆà¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19231 ERROR FAIL | Right movement broken in "สัดพอม่พี่น้อง" | got 6, expected 9 | /tests/layout/generic/test/test_backspace_delete.html
*** 19233 ERROR FAIL | Delete broken in "สัดพอมพี่น้อง" | got 6, expected 9 | /tests/layout/generic/test/test_backspace_delete.html
*** 19234 ERROR FAIL | Delete broken in "สัดพอมพี่น้อง" | got "สัดพอมพี่น้อง", expected "สสพ่à¹à¸žà¸µà¹ˆà¸­" | /tests/layout/generic/test/test_backspace_delete.html
*** 19241 ERROR FAIL | Right movement broken in "วัสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 2 | /tests/layout/generic/test/test_backspace_delete.html
*** 19243 ERROR FAIL | Backspace broken in "ัสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 0, expected 1 | /tests/layout/generic/test/test_backspace_delete.html
*** 19244 ERROR FAIL | Backspace broken in "ัสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "ัสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "วสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19246 ERROR FAIL | Right movement broken in "ัสดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 2 | /tests/layout/generic/test/test_backspace_delete.html
*** 19248 ERROR FAIL | Backspace broken in "สดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 0, expected 1 | /tests/layout/generic/test/test_backspace_delete.html
*** 19249 ERROR FAIL | Backspace broken in "สดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "สดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "วดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19251 ERROR FAIL | Right movement broken in "สดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 3 | /tests/layout/generic/test/test_backspace_delete.html
*** 19253 ERROR FAIL | Backspace broken in "ดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 0, expected 2 | /tests/layout/generic/test/test_backspace_delete.html
*** 19254 ERROR FAIL | Backspace broken in "ดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "ดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "วดพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19256 ERROR FAIL | Right movement broken in "ดีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 4 | /tests/layout/generic/test/test_backspace_delete.html
*** 19258 ERROR FAIL | Backspace broken in "ีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 0, expected 3 | /tests/layout/generic/test/test_backspace_delete.html
*** 19259 ERROR FAIL | Backspace broken in "ีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "ีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "วดพอà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19261 ERROR FAIL | Right movement broken in "ีพ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 4 | /tests/layout/generic/test/test_backspace_delete.html
*** 19263 ERROR FAIL | Backspace broken in "พ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 0, expected 3 | /tests/layout/generic/test/test_backspace_delete.html
*** 19264 ERROR FAIL | Backspace broken in "พ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "พ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "วดพà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | /tests/layout/generic/test/test_backspace_delete.html
*** 19266 ERROR FAIL | Right movement broken in "พ่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 4 | /tests/layout/generic/test/test_backspace_delete.html
*** 19268 ERROR FAIL | Backspace broken in "่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 0, expected 3 | /tests/layout/generic/test/test_backspace_delete.html
*** 19269 ERROR FAIL | Backspace broken in "่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "วดพม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html
*** 19271 ERROR FAIL | Right movement broken in "่อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 5 | /tests/layout/generic/test/test_backspace_delete.html
*** 19273 ERROR FAIL | Backspace broken in "อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 0, expected 4 | /tests/layout/generic/test/test_backspace_delete.html
*** 19274 ERROR FAIL | Backspace broken in "อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "วดพมพี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html
*** 19276 ERROR FAIL | Right movement broken in "อà¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 7 | /tests/layout/generic/test/test_backspace_delete.html
*** 19278 ERROR FAIL | Backspace broken in "à¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 0, expected 6 | /tests/layout/generic/test/test_backspace_delete.html
*** 19279 ERROR FAIL | Backspace broken in "à¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got "à¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡", expected "วดพมพีน้อง" | /tests/layout/generic/test/test_backspace_delete.html
*** 19281 ERROR FAIL | Right movement broken in "à¹à¸¡à¹ˆà¸žà¸µà¹ˆà¸™à¹‰à¸­à¸‡" | got 1, expected 8 | /tests/layout/generic/test/test_backspace_delete.html
*** 19283 ERROR FAIL | Backspace broken in "ม่พี่น้อง" | got 0, expected 7 | /tests/layout/generic/test/test_backspace_delete.html
*** 19284 ERROR FAIL | Backspace broken in "ม่พี่น้อง" | got "ม่พี่น้อง", expected "วดพมพีนอง" | /tests/layout/generic/test/test_backspace_delete.html
*** 19286 ERROR FAIL | Right movement broken in "ม่พี่น้อง" | got 1, expected 8 | /tests/layout/generic/test/test_backspace_delete.html
*** 19288 ERROR FAIL | Backspace broken in "่พี่น้อง" | got 0, expected 7 | /tests/layout/generic/test/test_backspace_delete.html
*** 19289 ERROR FAIL | Backspace broken in "่พี่น้อง" | got "่พี่น้อง", expected "วดพมพีนง" | /tests/layout/generic/test/test_backspace_delete.html
*** 19291 ERROR FAIL | Right movement broken in "่พี่น้อง" | got 1, expected 8 | /tests/layout/generic/test/test_backspace_delete.html
*** 19293 ERROR FAIL | Backspace broken in "พี่น้อง" | got 0, expected 7 | /tests/layout/generic/test/test_backspace_delete.html
*** 19294 ERROR FAIL | Backspace broken in "พี่น้อง" | got "พี่น้อง", expected "วดพมพีน" | /tests/layout/generic/test/test_backspace_delete.html

	Theppitak Karoonboonyanan Assignee
	Comment 44 • 17 years ago

Could you paste the error message in UTF-8, please?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 45 • 17 years ago

The test failed on Mac, qm-xserve01. Here's the log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1203503100.1203505420.29020.gz&fulltext=1

The errors (hope my paste works)

*** 19204 INFO Running /tests/layout/generic/test/test_backspace_delete.html...
*** 19205 INFO PASS | Right movement broken in "สวัสดีพ่อแม่พี่น้อง"
*** 19206 INFO PASS | Right movement broken in "สวัสดีพ่อแม่พี่น้อง"
*** 19207 INFO PASS | Delete broken in "สัสดีพ่อแม่พี่น้อง"
*** 19208 INFO PASS | Delete broken in "สัสดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19209 ERROR FAIL | Delete broken in "สัสดีพ่อแม่พี่น้อง" | got "สัสดีพ่อแม่พี่น้อง", expected "สสดีพ่อแม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19210 INFO PASS | Right movement broken in "สัสดีพ่อแม่พี่น้อง"
*** 19211 INFO PASS | Right movement broken in "สัสดีพ่อแม่พี่น้อง"
*** 19212 INFO PASS | Delete broken in "สัดีพ่อแม่พี่น้อง"
*** 19213 INFO PASS | Delete broken in "สัดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19214 ERROR FAIL | Delete broken in "สัดีพ่อแม่พี่น้อง" | got "สัดีพ่อแม่พี่น้อง", expected "สสพ่อแม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19215 INFO PASS | Right movement broken in "สัดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19216 ERROR FAIL | Right movement broken in "สัดีพ่อแม่พี่น้อง" | got 3, expected 4 | /tests/layout/generic/test/test_backspace_delete.html

*** 19217 INFO PASS | Delete broken in "สัดพ่อแม่พี่น้อง"
NEXT ERROR *** 19218 ERROR FAIL | Delete broken in "สัดพ่อแม่พี่น้อง" | got 3, expected 4 | /tests/layout/generic/test/test_backspace_delete.html

*** 19219 ERROR FAIL | Delete broken in "สัดพ่อแม่พี่น้อง" | got "สัดพ่อแม่พี่น้อง", expected "สสพ่แม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19220 INFO PASS | Right movement broken in "สัดพ่อแม่พี่น้อง"
NEXT ERROR *** 19221 ERROR FAIL | Right movement broken in "สัดพ่อแม่พี่น้อง" | got 4, expected 5 | /tests/layout/generic/test/test_backspace_delete.html

*** 19222 INFO PASS | Delete broken in "สัดพอแม่พี่น้อง"
NEXT ERROR *** 19223 ERROR FAIL | Delete broken in "สัดพอแม่พี่น้อง" | got 4, expected 5 | /tests/layout/generic/test/test_backspace_delete.html

*** 19224 ERROR FAIL | Delete broken in "สัดพอแม่พี่น้อง" | got "สัดพอแม่พี่น้อง", expected "สสพ่แพี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19225 INFO PASS | Right movement broken in "สัดพอแม่พี่น้อง"
NEXT ERROR *** 19226 ERROR FAIL | Right movement broken in "สัดพอแม่พี่น้อง" | got 5, expected 8 | /tests/layout/generic/test/test_backspace_delete.html

*** 19227 INFO PASS | Delete broken in "สัดพอม่พี่น้อง"
NEXT ERROR *** 19228 ERROR FAIL | Delete broken in "สัดพอม่พี่น้อง" | got 5, expected 8 | /tests/layout/generic/test/test_backspace_delete.html

*** 19229 ERROR FAIL | Delete broken in "สัดพอม่พี่น้อง" | got "สัดพอม่พี่น้อง", expected "สสพ่แพี่อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19230 INFO PASS | Right movement broken in "สัดพอม่พี่น้อง"
NEXT ERROR *** 19231 ERROR FAIL | Right movement broken in "สัดพอม่พี่น้อง" | got 6, expected 9 | /tests/layout/generic/test/test_backspace_delete.html

*** 19232 INFO PASS | Delete broken in "สัดพอมพี่น้อง"
NEXT ERROR *** 19233 ERROR FAIL | Delete broken in "สัดพอมพี่น้อง" | got 6, expected 9 | /tests/layout/generic/test/test_backspace_delete.html

*** 19234 ERROR FAIL | Delete broken in "สัดพอมพี่น้อง" | got "สัดพอมพี่น้อง", expected "สสพ่แพี่อ" | /tests/layout/generic/test/test_backspace_delete.html

*** 19235 INFO PASS | Right movement broken in "สวัสดีพ่อแม่พี่น้อง"
*** 19236 INFO PASS | Right movement broken in "สวัสดีพ่อแม่พี่น้อง"
*** 19237 INFO PASS | Backspace broken in "วัสดีพ่อแม่พี่น้อง"
*** 19238 INFO PASS | Backspace broken in "วัสดีพ่อแม่พี่น้อง"
*** 19239 INFO PASS | Backspace broken in "วัสดีพ่อแม่พี่น้อง"
*** 19240 INFO PASS | Right movement broken in "วัสดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19241 ERROR FAIL | Right movement broken in "วัสดีพ่อแม่พี่น้อง" | got 1, expected 2 | /tests/layout/generic/test/test_backspace_delete.html

*** 19242 INFO PASS | Backspace broken in "ัสดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19243 ERROR FAIL | Backspace broken in "ัสดีพ่อแม่พี่น้อง" | got 0, expected 1 | /tests/layout/generic/test/test_backspace_delete.html

*** 19244 ERROR FAIL | Backspace broken in "ัสดีพ่อแม่พี่น้อง" | got "ัสดีพ่อแม่พี่น้อง", expected "วสดีพ่อแม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19245 INFO PASS | Right movement broken in "ัสดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19246 ERROR FAIL | Right movement broken in "ัสดีพ่อแม่พี่น้อง" | got 1, expected 2 | /tests/layout/generic/test/test_backspace_delete.html

*** 19247 INFO PASS | Backspace broken in "สดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19248 ERROR FAIL | Backspace broken in "สดีพ่อแม่พี่น้อง" | got 0, expected 1 | /tests/layout/generic/test/test_backspace_delete.html

*** 19249 ERROR FAIL | Backspace broken in "สดีพ่อแม่พี่น้อง" | got "สดีพ่อแม่พี่น้อง", expected "วดีพ่อแม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19250 INFO PASS | Right movement broken in "สดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19251 ERROR FAIL | Right movement broken in "สดีพ่อแม่พี่น้อง" | got 1, expected 3 | /tests/layout/generic/test/test_backspace_delete.html

*** 19252 INFO PASS | Backspace broken in "ดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19253 ERROR FAIL | Backspace broken in "ดีพ่อแม่พี่น้อง" | got 0, expected 2 | /tests/layout/generic/test/test_backspace_delete.html

*** 19254 ERROR FAIL | Backspace broken in "ดีพ่อแม่พี่น้อง" | got "ดีพ่อแม่พี่น้อง", expected "วดพ่อแม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19255 INFO PASS | Right movement broken in "ดีพ่อแม่พี่น้อง"
NEXT ERROR *** 19256 ERROR FAIL | Right movement broken in "ดีพ่อแม่พี่น้อง" | got 1, expected 4 | /tests/layout/generic/test/test_backspace_delete.html

*** 19257 INFO PASS | Backspace broken in "ีพ่อแม่พี่น้อง"
NEXT ERROR *** 19258 ERROR FAIL | Backspace broken in "ีพ่อแม่พี่น้อง" | got 0, expected 3 | /tests/layout/generic/test/test_backspace_delete.html

*** 19259 ERROR FAIL | Backspace broken in "ีพ่อแม่พี่น้อง" | got "ีพ่อแม่พี่น้อง", expected "วดพอแม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19260 INFO PASS | Right movement broken in "ีพ่อแม่พี่น้อง"
NEXT ERROR *** 19261 ERROR FAIL | Right movement broken in "ีพ่อแม่พี่น้อง" | got 1, expected 4 | /tests/layout/generic/test/test_backspace_delete.html

*** 19262 INFO PASS | Backspace broken in "พ่อแม่พี่น้อง"
NEXT ERROR *** 19263 ERROR FAIL | Backspace broken in "พ่อแม่พี่น้อง" | got 0, expected 3 | /tests/layout/generic/test/test_backspace_delete.html

*** 19264 ERROR FAIL | Backspace broken in "พ่อแม่พี่น้อง" | got "พ่อแม่พี่น้อง", expected "วดพแม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19265 INFO PASS | Right movement broken in "พ่อแม่พี่น้อง"
NEXT ERROR *** 19266 ERROR FAIL | Right movement broken in "พ่อแม่พี่น้อง" | got 1, expected 4 | /tests/layout/generic/test/test_backspace_delete.html

*** 19267 INFO PASS | Backspace broken in "่อแม่พี่น้อง"
NEXT ERROR *** 19268 ERROR FAIL | Backspace broken in "่อแม่พี่น้อง" | got 0, expected 3 | /tests/layout/generic/test/test_backspace_delete.html

*** 19269 ERROR FAIL | Backspace broken in "่อแม่พี่น้อง" | got "่อแม่พี่น้อง", expected "วดพม่พี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19270 INFO PASS | Right movement broken in "่อแม่พี่น้อง"
NEXT ERROR *** 19271 ERROR FAIL | Right movement broken in "่อแม่พี่น้อง" | got 1, expected 5 | /tests/layout/generic/test/test_backspace_delete.html

*** 19272 INFO PASS | Backspace broken in "อแม่พี่น้อง"
NEXT ERROR *** 19273 ERROR FAIL | Backspace broken in "อแม่พี่น้อง" | got 0, expected 4 | /tests/layout/generic/test/test_backspace_delete.html

*** 19274 ERROR FAIL | Backspace broken in "อแม่พี่น้อง" | got "อแม่พี่น้อง", expected "วดพมพี่น้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19275 INFO PASS | Right movement broken in "อแม่พี่น้อง"
NEXT ERROR *** 19276 ERROR FAIL | Right movement broken in "อแม่พี่น้อง" | got 1, expected 7 | /tests/layout/generic/test/test_backspace_delete.html

*** 19277 INFO PASS | Backspace broken in "แม่พี่น้อง"
NEXT ERROR *** 19278 ERROR FAIL | Backspace broken in "แม่พี่น้อง" | got 0, expected 6 | /tests/layout/generic/test/test_backspace_delete.html

*** 19279 ERROR FAIL | Backspace broken in "แม่พี่น้อง" | got "แม่พี่น้อง", expected "วดพมพีน้อง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19280 INFO PASS | Right movement broken in "แม่พี่น้อง"
NEXT ERROR *** 19281 ERROR FAIL | Right movement broken in "แม่พี่น้อง" | got 1, expected 8 | /tests/layout/generic/test/test_backspace_delete.html

*** 19282 INFO PASS | Backspace broken in "ม่พี่น้อง"
NEXT ERROR *** 19283 ERROR FAIL | Backspace broken in "ม่พี่น้อง" | got 0, expected 7 | /tests/layout/generic/test/test_backspace_delete.html

*** 19284 ERROR FAIL | Backspace broken in "ม่พี่น้อง" | got "ม่พี่น้อง", expected "วดพมพีนอง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19285 INFO PASS | Right movement broken in "ม่พี่น้อง"
NEXT ERROR *** 19286 ERROR FAIL | Right movement broken in "ม่พี่น้อง" | got 1, expected 8 | /tests/layout/generic/test/test_backspace_delete.html

*** 19287 INFO PASS | Backspace broken in "่พี่น้อง"
NEXT ERROR *** 19288 ERROR FAIL | Backspace broken in "่พี่น้อง" | got 0, expected 7 | /tests/layout/generic/test/test_backspace_delete.html

*** 19289 ERROR FAIL | Backspace broken in "่พี่น้อง" | got "่พี่น้อง", expected "วดพมพีนง" | /tests/layout/generic/test/test_backspace_delete.html

*** 19290 INFO PASS | Right movement broken in "่พี่น้อง"
NEXT ERROR *** 19291 ERROR FAIL | Right movement broken in "่พี่น้อง" | got 1, expected 8 | /tests/layout/generic/test/test_backspace_delete.html

*** 19292 INFO PASS | Backspace broken in "พี่น้อง"
NEXT ERROR *** 19293 ERROR FAIL | Backspace broken in "พี่น้อง" | got 0, expected 7 | /tests/layout/generic/test/test_backspace_delete.html

*** 19294 ERROR FAIL | Backspace broken in "พี่น้อง" | got "พี่น้อง", expected "วดพมพีน" | /tests/layout/generic/test/test_backspace_delete.html

	Theppitak Karoonboonyanan Assignee
	Comment 46 • 17 years ago

So, caret movements on Mac are still character-wise, not cell-wise. (How about Windows?)

That means we need to fix nsFrameSelection::MoveCaret() for broken platforms.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 47 • 17 years ago

Windows tests passed so I guess Windows is OK.

The Mac bug is in gfxAtsuiFonts. I'll attach a patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 48 • 17 years ago

I attached a fix in bug 418754. When that's landed (hopefully very soon) we can reland this patch. Theppitak, if you have Mac access it would be great if you can test that patch with your patch.

	Theppitak Karoonboonyanan Assignee
	Comment 49 • 17 years ago

I don't have Mac. Probably, somebody in Cc: list (Isriya?) can help.

	Isriya Paireepairit
	Comment 50 • 17 years ago

I'm on the way to FOSDEM so I'm not convenient. Anyway, I forward this issue to other who also have Mac.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 51 • 17 years ago

I will land 418754 and then test this myself and reland it if it passes.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 52 • 17 years ago

I tested this on Mac after landing 418754. Everything was fine except the last two tests ... editor makes the leading spaces into  , which was causing failures comparing them against real spaces. I changed the test to check for   instead and it works. I don't know why that passed on Linux/Windows before, they should behave the same. I'll check in and see.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 53 • 17 years ago

checked in again

	Theppitak Karoonboonyanan Assignee
	Comment 54 • 17 years ago

In fact, I used nbsp in the patch since the beginning. Probably, it's got changed somehow on the way.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 55 • 17 years ago

Ah, OK.

All tests passsed, so we're done here! Thanks!!!

	Keng Susumpow
	Comment 56 • 17 years ago

Tested on Mac OS X 10.5.2 with Minefield  3.0b4pre build 2008022104 

This bug still remains.

	Theppitak Karoonboonyanan Assignee
	Comment 57 • 17 years ago

Patipat, please update CVS, or just wait for next build. It's just been committed. :)

	Arthit Suriyawongkul
	Comment 58 • 17 years ago

from the first testcase (attachment #91354 [details]), "testcase for reproduce the bug" 2002-07-15 09:16 PST.

กี่กั้ก๊กู๋ก็กฺก์ 

the last group of characters is:  กฺก์ 

when the caret is in front of กฺก์

.. |กฺก์

and we hit <delete>

should it delete everything

.. |


or should it delete just  กฺ ?

.. |ก์


----

for caret movement behavior at this moment (Firefox 3 Beta 3, Ubuntu Linux 8.04 Alpha 4),
if the caret position is after กฺก์

.. กฺก์|

and we hit <left-arrow>

the caret will skip to the position before  กฺก์

.. |กฺก์

and vice versa for <right-arrow>

.. |กฺก์
<right-arrow>
.. กฺก์|

	Arthit Suriyawongkul
	Comment 59 • 17 years ago

the testcase "กฺก์" in comment #58 composed of 4 characters.

(Ko Kai) (Phinthu) (Ko Kai) (Thanthakhat)

u+0e01 u+0e3a u+0e01 u+0e4c

the second character Phinthu may not be very clear to see, it should be rendered as a small dot (.) under first character Ko Kai.

	Theppitak Karoonboonyanan Assignee
	Comment 60 • 17 years ago

You have found a Pango's bug, I think. This also occurs in other GNOME apps too.

	Arthit Suriyawongkul
	Comment 61 • 17 years ago

(In reply to comment #60)
> You have found a Pango's bug, I think. This also occurs in other GNOME apps
> too.

You mean Windows and Mac OS X users will experience different behavior ?
(as Firefox on those platform use no Pango)

	Theppitak Karoonboonyanan Assignee
	Comment 62 • 17 years ago

(In reply to comment #61)
> (In reply to comment #60)
> > You have found a Pango's bug, I think. This also occurs in other GNOME apps
> > too.
> 
> You mean Windows and Mac OS X users will experience different behavior ?
> (as Firefox on those platform use no Pango)

Probably. Confirmation needed.

	Keng Susumpow
	Comment 63 • 17 years ago

กี่กั้ก๊กู๋ก็กฺก์

here is the result for above test case on the lastest nightly build on trunk (2008022204) on Mac OS X 10.5.2

-- Moving Caret --
1. if the caret position is after กฺก์
..กฺก์|
<left-arrow>
..กฺ|ก์

2. if the caret position is before กฺก์
..|กฺก์
<right-arrow>
..กฺ|ก์

these 2 results it seems different from Linux

-- Deleting --
3. if the caret position is before กฺก์
..|กฺก์
<delete>
..|ก์
<delete>
..|

by all of these results, apparently that it is work properly on Mac OS X. 

	Theppitak Karoonboonyanan Assignee
	Comment 64 • 17 years ago

(In reply to comment #60)
> You have found a Pango's bug, I think. This also occurs in other GNOME apps
> too.

Pango bug filed with patch:
  http://bugzilla.gnome.org/show_bug.cgi?id=518084

With this patch, Minefield works properly with your test case.

	Arthit Suriyawongkul
	Comment 65 • 17 years ago

(In reply to comment #60)
> You have found a Pango's bug, I think. This also occurs in other GNOME apps
> too.

I have tested this again with your testcase given in GNOME Bugzilla
http://bugzilla.gnome.org/show_bug.cgi?id=518084

and found that the behavior in Firefox and GNOME app (gedit) are different.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3) Gecko/2008021416 Firefox/3.0b3

libpango1.0-0 1.19.3-1ubuntu1


Firefox (characters are delete one by one):
ธ|มฺมํ
<delete-key>
ธ|ฺมํ
<delete-key>
ธ|มํ
<delete-key>
ธ|ํ
<delete-key>
ธ|


gedit (the whole cluster มฺมํ are deleted at once):
ธ|มฺมํ
<delete-key>
ธ|

	Theppitak Karoonboonyanan Assignee
	Comment 66 • 17 years ago

(In reply to comment #65)
> Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3) Gecko/2008021416
> Firefox/3.0b3

Hey, why testing the unpatched version? The fix won't reach distro in one day!

	David Baron :dbaron:
	Comment 67 • 17 years ago

The current plan is to back this patch out to fix blocking1.9+ regressions bug 419217 and bug 419406.

	David Baron :dbaron:
	Comment 68 • 17 years ago

This backout patch was generated from the bonsai backout script from http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=roc%2B%25cs.cmu.edu&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-21+18%3A10&maxdate=2008-02-21+21%3A00&cvsroot=%2Fcvsroot

This script generated only one merge conflict against current trunk, which was the IID of nsISelectionController.  I generated a new IID.

Furthermore, instead of removing the test that the patch added, this backout patch marks the now-failing tests as todo.

	David Baron :dbaron:
	Comment 69 • 17 years ago

Reopening this bug since the patch is planned to be backed out.  Normally I'd wait until it was, but I don't think the approval request will be seen unless I reopen it now.

	Damon Sicore (:damons)
	Comment 70 • 17 years ago

Comment on attachment 315340 [details] [diff] [review]
patch to back out
[Checkin: Comment 72]

a1.9+=damons

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 71 • 17 years ago

David backed this out.

	David Baron :dbaron:
	Comment 72 • 17 years ago

Backed out 2008-04-14 18:04.

We still want this fixed, we just don't want the regressions that the original patch caused, and we're getting short on time for Firefox 3.  Hopefully you can still get the fix in (with the regressions fixed) soon afterwards.

(It would be good to get the regressions in automated tests as well.)

	David Baron :dbaron:
	Comment 73 • 17 years ago

(And, in fact, if both of those regression bugs have reviewed patches soon, they should be able to land quickly in mozilla-central once it opens.)

	Theppitak Karoonboonyanan Assignee
	Comment 74 • 16 years ago

I'm preparing a single patch that merges the three patches (formerly proposed for this bug and the other two) to begin with. However, one approach has to be chosen from Bug #419406. Some comments on this would be helpful.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 75 • 16 years ago

Peter, see comment #74. Your guidance is still needed...

	Theppitak Karoonboonyanan Assignee
	Comment 76 • 16 years ago

To start with, I've chosen the last proposed patches for Bug #419406 (attatchment #305904), Bug #419217 (attachment #315612 [details] [diff] [review]), and the backed out patch (attachment #303981 [details] [diff] [review]), and merge them. Mochitest is also re-enabled.

This might be a base to continue with this bug.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 77 • 16 years ago

Ask peterv for review if you think it's ready. Can we have mochitests for bug 419406 and bug 419217 too please? :-)

	Theppitak Karoonboonyanan Assignee
	Comment 78 • 16 years ago

Mochitests added.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 79 • 16 years ago

I think it's probably too late to put this in for Firefox 3. We're so very close to frozen solid and we wouldn't want to find any new editing regressions now.

But please get review from peterv so we can get this on trunk as soon as possible and make sure it's good for the next release. Thanks!!!

	Theppitak Karoonboonyanan Assignee
	Comment 80 • 16 years ago

Comment on attachment 317517 [details] [diff] [review]
Merged patch with required mochitests included

I can't resolve the disparity between nsISelection and nsISelectionController as noted in bug 419406 comment #11 by myself. So, may I request for a review?

	Peter Van der Beken [:peterv]
	Comment 81 • 16 years ago

Comment on attachment 317517 [details] [diff] [review]
Merged patch with required mochitests included

> Index: content/base/public/nsISelectionController.idl
> ===================================================================

> +   /** CharacterExtendForDelete will extend the selection one character cell
> +   *  forward in the document.

Line this up correctly:

  /**
   *

> Index: editor/libeditor/html/nsHTMLEditRules.cpp
> ===================================================================

> @@ -1993,20 +1995,32 @@ nsHTMLEditRules::WillDeleteSelection(nsI
>        PRInt32 so = visOffset;
>        PRInt32 eo = visOffset+1;
>        if (aAction == nsIEditor::ePrevious) 
>        { 
>          if (so == 0) return NS_ERROR_UNEXPECTED;
>          so--; 
>          eo--; 
>        }
> +      else
> +      {
> +        res = mHTMLEditor->ExtendSelectionForDelete(aSelection, &aAction);
> +        NS_ENSURE_SUCCESS(res, res);
> +
> +        nsIDOMRange *pRange;

Use "range" as the variable name.
Use a nsCOMPtr, you're leaking pRange.

> +        res = aSelection->GetRangeAt(0, &pRange);
> +        NS_ENSURE_SUCCESS(res, res);
> +
> +        res = pRange->GetEndOffset(&eo);
> +        NS_ENSURE_SUCCESS(res, res);
> +      }

Should probably make ExtendSelectionForDelete handle ePrevious too. That seems at least needed to fix bug 332636, which smontagu just pointed out to me. Just have to make sure that we don't jump over the whole cluster in that case. I'm fine with leaving that for bug 332636, but please add a FIXME comment then and make a note in bug 332636.
I think the range's start/end containers will be equal to visNode, but should probably assert that here.

>        res = nsWSRunObject::PrepareToDeleteRange(mHTMLEditor, address_of(visNode), &so, address_of(visNode), &eo);
>        if (NS_FAILED(res)) return res;
>        nsCOMPtr<nsIDOMCharacterData> nodeAsText(do_QueryInterface(visNode));
> -      res = mHTMLEditor->DeleteText(nodeAsText,so,1);
> +      res = mHTMLEditor->DeleteText(nodeAsText, PR_MIN(so,eo), PR_ABS(eo - so));

Space after comma.

I'm wondering if it would work to not call ExtendSelectionForDelete in nsPlaintextEditor::DeleteSelection, but instead call it in nsTextEditRules::WillDeleteSelection.

> Index: editor/libeditor/text/nsPlaintextEditor.h
> ===================================================================

> +  /** Extends the selection for given deletion operation
> +   * If done, also update aAction to what's actually left to do after the

  /**
   * Extends ...

> Index: editor/libeditor/text/nsTextEditRulesBidi.cpp
> ===================================================================

> +  levelOfDeletion = (nsIEditor::eNext==aAction
> +                     || nsIEditor::eNextWord==aAction) ? levelAfter : levelBefore;

  levelOfDeletion =
    (nsIEditor::eNext==aAction || nsIEditor::eNextWord==aAction) ?
    levelAfter : levelBefore;

> Index: layout/forms/nsTextControlFrame.cpp
> ===================================================================

> +  if (mFrameSelection)
> +    return mFrameSelection->CharacterExtendForDelete();
> +  return NS_ERROR_NULL_POINTER;

  return mFrameSelection ? mFrameSelection->CharacterExtendForDelete() : NS_ERROR_NULL_POINTER;

	Theppitak Karoonboonyanan Assignee
	Comment 82 • 16 years ago

Thanks for the review.

(In reply to comment #81)

> > Index: content/base/public/nsISelectionController.idl
> > ===================================================================
> 
> > +   /** CharacterExtendForDelete will extend the selection one character cell
> > +   *  forward in the document.
> 
> Line this up correctly:
> 
>   /**
>    *

It's the style of the file. Do I need to adjust other functions as well?

> > Index: editor/libeditor/html/nsHTMLEditRules.cpp
> > ===================================================================
> 
> > @@ -1993,20 +1995,32 @@ nsHTMLEditRules::WillDeleteSelection(nsI
> >        PRInt32 so = visOffset;
> >        PRInt32 eo = visOffset+1;
> >        if (aAction == nsIEditor::ePrevious) 
> >        { 
> >          if (so == 0) return NS_ERROR_UNEXPECTED;
> >          so--; 
> >          eo--; 
> >        }
> > +      else
> > +      {
> > +        res = mHTMLEditor->ExtendSelectionForDelete(aSelection, &aAction);
> > +        NS_ENSURE_SUCCESS(res, res);
> > +
> > +        nsIDOMRange *pRange;
> 
> Use "range" as the variable name.
> Use a nsCOMPtr, you're leaking pRange.

OK.

> > +        res = aSelection->GetRangeAt(0, &pRange);
> > +        NS_ENSURE_SUCCESS(res, res);
> > +
> > +        res = pRange->GetEndOffset(&eo);
> > +        NS_ENSURE_SUCCESS(res, res);
> > +      }
> 
> Should probably make ExtendSelectionForDelete handle ePrevious too. That seems
> at least needed to fix bug 332636, which smontagu just pointed out to me. Just
> have to make sure that we don't jump over the whole cluster in that case. I'm
> fine with leaving that for bug 332636, but please add a FIXME comment then and
> make a note in bug 332636.

OK, I'll add FIXME comments where possible.

> I think the range's start/end containers will be equal to visNode, but should
> probably assert that here.

OK.

> >        res = nsWSRunObject::PrepareToDeleteRange(mHTMLEditor, address_of(visNode), &so, address_of(visNode), &eo);
> >        if (NS_FAILED(res)) return res;
> >        nsCOMPtr<nsIDOMCharacterData> nodeAsText(do_QueryInterface(visNode));
> > -      res = mHTMLEditor->DeleteText(nodeAsText,so,1);
> > +      res = mHTMLEditor->DeleteText(nodeAsText, PR_MIN(so,eo), PR_ABS(eo - so));
> 
> Space after comma.

OK.

> I'm wondering if it would work to not call ExtendSelectionForDelete in
> nsPlaintextEditor::DeleteSelection, but instead call it in
> nsTextEditRules::WillDeleteSelection.

I'm rather wishing toward a reverse direction: moving the call to ExtendSelectionForDelete() out of nsHTMLEditRules::WillDeleteSelection(), and letting it "not handle", so that the call is delayed until getting back to nsPlaintextEditor::DeleteSelection() again. This might resolve the layer crossing to access nsEditor::mSelConWeak as mentioned in bug 419406 comment #11. But this would mean the missing of the ending call to InsertBRIfNeeded(), which may not be desirable. And this may be why nsHTMLEditRules has to handle it.

So, if the ExtendSelectionForDelete() call is to be moved to nsTextEditRules::WillDeleteSelection(), a similar layer crossing may be caused.

Besides, the existing code in nsTextEditRules::WillDeleteSelection() is doing some nut-and-bolt jobs clearing up the node structure. I'm not sure if replacing it with higher-level operations would be safe. But I'll try it out anyway.

> > Index: editor/libeditor/text/nsPlaintextEditor.h
> > ===================================================================
> 
> > +  /** Extends the selection for given deletion operation
> > +   * If done, also update aAction to what's actually left to do after the
> 
>   /**
>    * Extends ...

Again, it's the style of the file. I just tried to be consistent with existing codes. So, do I need to adjust other functions as well?

> > Index: editor/libeditor/text/nsTextEditRulesBidi.cpp
> > ===================================================================
> 
> > +  levelOfDeletion = (nsIEditor::eNext==aAction
> > +                     || nsIEditor::eNextWord==aAction) ? levelAfter : levelBefore;
> 
>   levelOfDeletion =
>     (nsIEditor::eNext==aAction || nsIEditor::eNextWord==aAction) ?
>     levelAfter : levelBefore;

OK.

> > Index: layout/forms/nsTextControlFrame.cpp
> > ===================================================================
> 
> > +  if (mFrameSelection)
> > +    return mFrameSelection->CharacterExtendForDelete();
> > +  return NS_ERROR_NULL_POINTER;
> 
>   return mFrameSelection ? mFrameSelection->CharacterExtendForDelete() :
> NS_ERROR_NULL_POINTER;

This is also the matter of style, to be consistent with other functions in the file.

	Theppitak Karoonboonyanan Assignee
	Comment 83 • 16 years ago

Patch adjusted as per comments, except for some styles as noted.

I haven't obsoleted the old patch yet, as the moving of ExtendSelectionForDelete() seems so experimental, and may risk causing new bugs, although it passes mochitests so far.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 84 • 16 years ago

Theppitak, what else needs to be done here to get this fixed for real?

	Theppitak Karoonboonyanan Assignee
	Comment 85 • 16 years ago

Some questions in comment #82 get no answer so far. And I have tested the patch in comment #83 for a while. Probably, I can request for a second review although not all comments in the first one have been fullfilled?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 86 • 16 years ago

Yeah, I think that's the best idea.

	Theppitak Karoonboonyanan Assignee
	Comment 87 • 16 years ago

Comment on attachment 331084 [details] [diff] [review]
Adjusted patch, moving ExtendSelectionForDelete call into nsTextEditRules::WillDeleteSelection

Requesting for another review.

	Peter Van der Beken [:peterv]
	Comment 88 • 16 years ago

Comment on attachment 331084 [details] [diff] [review]
Adjusted patch, moving ExtendSelectionForDelete call into nsTextEditRules::WillDeleteSelection

(In reply to comment #82)
> It's the style of the file. Do I need to adjust other functions as well?

> Again, it's the style of the file. I just tried to be consistent with existing
> codes. So, do I need to adjust other functions as well?

Both files are inconsistent. Please adjust the comments you add, but don't touch the existing ones if you're not editing them in some other way.


> > > +  if (mFrameSelection)
> > > +    return mFrameSelection->CharacterExtendForDelete();
> > > +  return NS_ERROR_NULL_POINTER;
> > 
> >   return mFrameSelection ? mFrameSelection->CharacterExtendForDelete() :
> > NS_ERROR_NULL_POINTER;
> 
> This is also the matter of style, to be consistent with other functions in the
> file.

Keep your change, the one I suggested would make us end up with a too long line.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 89 • 16 years ago

Comment on attachment 331084 [details] [diff] [review]
Adjusted patch, moving ExtendSelectionForDelete call into nsTextEditRules::WillDeleteSelection

+        res = range->GetStartContainer(&container);
+	NS_ENSURE_SUCCESS(res, res);
+	NS_ASSERTION(container == visNode, "selection start not in visNode");
+
+	res = range->GetEndContainer(&container);
+	NS_ENSURE_SUCCESS(res, res);
+	NS_ASSERTION(container == visNode, "selection end not in visNode");

Fix indent.

Theppitak, sorry for the delay, but put up a new patch and we'll get this checked in ASAP

	Theppitak Karoonboonyanan Assignee
	Comment 90 • 16 years ago

Thanks for the reviews. I've applied what you commented.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 91 • 16 years ago

Pushed 34967ab14be8.

Thank you very much!!! Sorry it took so long.