157546 - [CTL-Thai] IM: <delete> key should delete WHOLE Thai "display cell"

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Arthit Suriyawongkul]

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

Comment 1

[Arthit Suriyawongkul]

Attachment: testcase for reproduce the bug

Comment 2

[Roy Yokoyama]

katakai-san: can you help us out ?

Comment 3

[Arthit Suriyawongkul]

also occurs in Windows XP

Comment 4

[Arthit Suriyawongkul]

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

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

Comment 5

[Arthit Suriyawongkul]

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

Comment 6

[Roland Mainz]

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

Comment 7

[Masaki Katakai]

Confirmed. Actually the same behavior with backspace.

Prabhat/Theppitak,

Can you work on this bug?

Comment 8

[James Clark]

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.

Comment 9

[Masaki Katakai]

Prabhat,

Could you review James' comment?

Comment 10

[Arthit Suriyawongkul]

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

Comment 11

[Karl Hong]

Attachment: patch for delete key to delete CTL cell

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.

Comment 12

[Karl Hong]

Attachment: Patch for 'delete' key with SUNCTL macro

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.

Comment 13

[Arthit Suriyawongkul]

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

Comment 14

[Arthit Suriyawongkul]

(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.

Comment 15

[Theppitak Karoonboonyanan]

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?

Comment 16

[Theppitak Karoonboonyanan]

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].)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 18

[Arthit Suriyawongkul]

(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)

Comment 19

[Karl Hong]

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

Comment 20

[Theppitak Karoonboonyanan]

Attachment: Adapted patch for cell-wise deletion, by extending nsISelectionController

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?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

yes, definitely

Comment 22

[Theppitak Karoonboonyanan]

Attachment: Patch with regenerated uuid

Comment 23

[Theppitak Karoonboonyanan]

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

May I request for a review?

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a=beltzner for 1.9

Comment 25

[Reed Loden [:reed]]

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

Comment 26

[Steve England [:stevee]]

I think this caused bug 417745

Comment 27

[Reed Loden [:reed]]

Backed out due to the regression in bug 417745.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 29

[Theppitak Karoonboonyanan]

Attachment: Patch which shouldn't cause bug 417745

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

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 31

[Theppitak Karoonboonyanan]

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

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 33

[Theppitak Karoonboonyanan]

Attachment: A first mochitest

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.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 35

[Theppitak Karoonboonyanan]

Attachment: Patch with mochitest included

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

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 37

[Theppitak Karoonboonyanan]

(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.

Comment 38

[Theppitak Karoonboonyanan]

Attachment: Patch with guard against uncollapsed selection

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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?

Comment 40

[Theppitak Karoonboonyanan]

Attachment: Patch testing textContent instead of innerHTML in mochitest

OK. Updated.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

great!

Comment 42

[Reed Loden [:reed]]

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

Comment 43

[Reed Loden [:reed]]

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

Comment 44

[Theppitak Karoonboonyanan]

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

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 46

[Theppitak Karoonboonyanan]

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.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Windows tests passed so I guess Windows is OK.

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

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 49

[Theppitak Karoonboonyanan]

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

Comment 50

[Isriya Paireepairit]

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

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in again

Comment 54

[Theppitak Karoonboonyanan]

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

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, OK.

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

Comment 56

[Keng Susumpow]

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

This bug still remains.

Comment 57

[Theppitak Karoonboonyanan]

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

Comment 58

[Arthit Suriyawongkul]

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>
.. กฺก์|

Comment 59

[Arthit Suriyawongkul]

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.

Comment 60

[Theppitak Karoonboonyanan]

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

Comment 61

[Arthit Suriyawongkul]

(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)

Comment 62

[Theppitak Karoonboonyanan]

(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.

Comment 63

[Keng Susumpow]

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

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. 

Comment 64

[Theppitak Karoonboonyanan]

(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.

Comment 65

[Arthit Suriyawongkul]

(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>
ธ|

Comment 66

[Theppitak Karoonboonyanan]

(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!

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to back out [Checkin: Comment 72]

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.

Comment 69

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 70

[Damon Sicore (:damons)]

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

a1.9+=damons

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

David backed this out.

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 73

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 74

[Theppitak Karoonboonyanan]

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.

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 76

[Theppitak Karoonboonyanan]

Attachment: Patch for the new round, with the three proposed patches merged

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.

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 78

[Theppitak Karoonboonyanan]

Attachment: Merged patch with required mochitests included

Mochitests added.

Comment 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!!!

Comment 80

[Theppitak Karoonboonyanan]

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?

Comment 81

[Peter Van der Beken [:peterv]]

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;

Comment 82

[Theppitak Karoonboonyanan]

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.

Comment 83

[Theppitak Karoonboonyanan]

Attachment: Adjusted patch, moving ExtendSelectionForDelete call into nsTextEditRules::WillDeleteSelection

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.

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 85

[Theppitak Karoonboonyanan]

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?

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I think that's the best idea.

Comment 87

[Theppitak Karoonboonyanan]

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

Requesting for another review.

Comment 88

[Peter Van der Beken [:peterv]]

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.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 90

[Theppitak Karoonboonyanan]

Attachment: Adjusted patch [Checkin: Comment 91]

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

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 34967ab14be8.

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