Closed Bug 157546 Opened 17 years ago Closed 11 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: arthit.suriyawongkul, Assigned: thep)

References

(Blocks 1 open bug)

Details

(Keywords: intl, Whiteboard: [not needed for 1.9])

Attachments

(3 files, 12 obsolete files)

278 bytes, text/html
Details
7.55 KB, patch
damons
: approval1.9+
Details | Diff | Splinter Review
27.22 KB, patch
roc
: review+
Details | Diff | Splinter Review
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
Blocks: thai
katakai-san: can you help us out ?
Assignee: yokoyama → katakai
Keywords: intl
also occurs in Windows XP
OS: Solaris → All
Hardware: Sun → All
Summary: Thai IM: <delete> key should delete WHOLE Thai "display cell" → [CTL-Thai] IM: <delete> key should delete WHOLE Thai "display cell"
since bug 133212 "--enable-ctl bustage" has been fixed.

could anyone help review Theppitak's patch pls? thx :)
pls ignore comment #4, it's a wrong post. Sorry.
Over to new bugzilla component "Complex Text Layout" ...
Component: Internationalization → Complex Text Layout
QA Contact: ruixu → ylong
Confirmed. Actually the same behavior with backspace.

Prabhat/Theppitak,

Can you work on this bug?


Status: NEW → ASSIGNED
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.
Prabhat,

Could you review James' comment?
Assignee: katakai → prabhat.hegde
Status: ASSIGNED → NEW
still not fixed in Mozilla nightly 200405
(as well as Firefox 0.8)
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.
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.
Attachment #157105 - Flags: review?(daniel)
Assignee: prabhat.hegde → karl.hong
Depends on: grapheme-breaker
Blocks: 283271
On Firefox 2.0.0.11 and Firefox 3.0b3pre, this bug still remains.
(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.
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?
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].)
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?
(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)
I am no longer working on this area, no access the code, could someone else takes over the bug?
Assignee: karl.hong → prabhat.hegde
QA Contact: amyy → arthit
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?
Attached patch Patch with regenerated uuid (obsolete) — Splinter Review
Attachment #302763 - Attachment is obsolete: true
Comment on attachment 302766 [details] [diff] [review]
Patch with regenerated uuid

May I request for a review?
Attachment #302766 - Flags: review?(roc)
Assignee: prabhat.hegde → thep
Attachment #302766 - Flags: approval1.9?
Comment on attachment 302766 [details] [diff] [review]
Patch with regenerated uuid

a=beltzner for 1.9
Attachment #302766 - Flags: approval1.9? → approval1.9+
Attachment #157105 - Attachment is obsolete: true
Attachment #157105 - Flags: review?(daniel)
Attachment #156838 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Depends on: 417745
I think this caused bug 417745
Backed out due to the regression in bug 417745.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If/when this gets re-fixed, we need a mochitest for this bug and also bug 417745.
Attachment #302766 - Attachment is obsolete: true
I couldn't reproduce bug 417745 on Linux. But I believe the line removed in this patch was the culprit.
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.
Sorry, I have no idea what mochitests mean. And, looking at the sample file, it requires JavaScript skills, which I don't have.
It's not that hard, really :-). If you can figure out content/ and layout/ you can figure out Mochitests :-).
Attached file A first mochitest (obsolete) —
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.
Attachment #303704 - Flags: review?(roc)
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.
Attached patch Patch with mochitest included (obsolete) — Splinter Review
Thanks for the comment. So, I have included the modified mochikit into the patch, for your checking convenience.
Attachment #303649 - Attachment is obsolete: true
Attachment #303704 - Attachment is obsolete: true
Attachment #303937 - Flags: review?(roc)
Attachment #303704 - Flags: review?(roc)
+  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.
(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.
Attachment #303937 - Attachment is obsolete: true
Attachment #303963 - Flags: review?(roc)
Attachment #303937 - Flags: review?(roc)
+  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?
OK. Updated.
Attachment #303963 - Attachment is obsolete: true
Attachment #303981 - Flags: review?(roc)
Attachment #303963 - Flags: review?(roc)
Comment on attachment 303981 [details] [diff] [review]
Patch testing textContent instead of innerHTML in mochitest

great!
Attachment #303981 - Flags: superreview+
Attachment #303981 - Flags: review?(roc)
Attachment #303981 - Flags: review+
Attachment #303981 - Flags: approval1.9?
Attachment #303981 - Flags: approval1.9? → approval1.9+
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Could you paste the error message in UTF-8, please?
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

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.
Windows tests passed so I guess Windows is OK.

The Mac bug is in gfxAtsuiFonts. I'll attach a patch.
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.
Depends on: 418754
I don't have Mac. Probably, somebody in Cc: list (Isriya?) can help.
I'm on the way to FOSDEM so I'm not convenient. Anyway, I forward this issue to other who also have Mac.
I will land 418754 and then test this myself and reland it if it passes.
I tested this on Mac after landing 418754. Everything was fine except the last two tests ... editor makes the leading spaces into &nbsp;, which was causing failures comparing them against real spaces. I changed the test to check for &nbsp; 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.
checked in again
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
In fact, I used nbsp in the patch since the beginning. Probably, it's got changed somehow on the way.
Ah, OK.

All tests passsed, so we're done here! Thanks!!!
Tested on Mac OS X 10.5.2 with Minefield  3.0b4pre build 2008022104 

This bug still remains.
Patipat, please update CVS, or just wait for next build. It's just been committed. :)
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>
.. กฺก์|
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.
You have found a Pango's bug, I think. This also occurs in other GNOME apps too.
(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)
(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.
กี่กั้ก๊กู๋ก็กฺก์

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. 

(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.
(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>
ธ|
(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!
Depends on: 419217
Depends on: 419406
The current plan is to back this patch out to fix blocking1.9+ regressions bug 419217 and bug 419406.
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.
Attachment #315340 - Flags: approval1.9?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 315340 [details] [diff] [review]
patch to back out
[Checkin: Comment 72]

a1.9+=damons
Attachment #315340 - Flags: approval1.9? → approval1.9+
David backed this out.
Flags: wanted-next+
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.)
(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.)
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.
Peter, see comment #74. Your guidance is still needed...
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.
Attachment #303981 - Attachment is obsolete: true
Ask peterv for review if you think it's ready. Can we have mochitests for bug 419406 and bug 419217 too please? :-)
Mochitests added.
Attachment #316669 - Attachment is obsolete: true
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 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?
Attachment #317517 - Flags: review?(peterv)
Whiteboard: [no needed 1.9][not needed for 1.9]
Whiteboard: [no needed 1.9][not needed for 1.9] → [not needed for 1.9]
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;
Attachment #317517 - Flags: review?(peterv) → review-
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.
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.
Component: Layout: CTL → Layout: Text
QA Contact: arthit → layout.fonts-and-text
Theppitak, what else needs to be done here to get this fixed for real?
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?
Yeah, I think that's the best idea.
Comment on attachment 331084 [details] [diff] [review]
Adjusted patch, moving ExtendSelectionForDelete call into nsTextEditRules::WillDeleteSelection

Requesting for another review.
Attachment #331084 - Flags: review?(peterv)
Attachment #331084 - Flags: review?(peterv) → review+
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 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
Attachment #331084 - Flags: superreview+
Thanks for the reviews. I've applied what you commented.
Attachment #331084 - Attachment is obsolete: true
Attachment #342183 - Flags: superreview?(roc)
Attachment #342183 - Flags: review?(peterv)
Attachment #342183 - Flags: superreview?(roc)
Attachment #342183 - Flags: superreview+
Attachment #342183 - Flags: review?(peterv)
Attachment #342183 - Flags: review+
Attachment #317517 - Attachment is obsolete: true
Pushed 34967ab14be8.

Thank you very much!!! Sorry it took so long.
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
Attachment #342183 - Attachment description: Adjusted patch → Adjusted patch [Checkin: Comment 91]
Keywords: checkin-needed
Target Milestone: mozilla1.9beta4 → mozilla1.9.1b2
Attachment #315340 - Attachment description: patch to back out → patch to back out [Checkin: Comment 72]
Depends on: 462188
Depends on: 487524
You need to log in before you can comment on or make changes to this bug.