644428 - cursor position on wyiswyg editors

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[ebrahim]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0) Gecko/20110322 Firefox/4.0 Iceweasel/4.0
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0) Gecko/20110322 Firefox/4.0 Iceweasel/4.0

i simplified a bug that i seen in wyiswyg editors using html5 contenteditable. it can reproduced with any html4 wyiswyg editors

Reproducible: Always

Steps to Reproduce:
1. put this in a html file <div contenteditable="true" style="border:solid 1px">a<span>b</span>c </div>
2. try remove characters by backspace slowly from end of it.
3. cursor will moved to next line!



i tested both linux and windows version of firefox4. i can see this problem on both of them

Comment 1

[ebrahim]

a fix:
3. after removing "b" from that text (you must start removing from end of it)  keyboards cursors indicator will be moved to nextline.

Comment 2

[Alice0775 White]

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/c0dbdafa583c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101104 Firefox/4.0b8pre Firefox/4.0b8pre ID:20101104134944
Fails:
http://hg.mozilla.org/mozilla-central/rev/52a6a18eeb61
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101104 Firefox/4.0b8pre Firefox/4.0b8pre ID:20101104165457
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c0dbdafa583c&tochange=52a6a18eeb61

In local build,
build from 217e60535a77 : fails
build from ed0befc22bb7 : fails
build from 8389e11781ed : works

Regressed between cset 8389e11781ed and ed0befc22bb7

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Comment 4

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

+  if (mRect.height == 0) {
+    // Empty inline frames will be pushed down in the line, so we need to
+    // account for that here.
+    baseline *= 0;

Why not just baseline = 0?

What if the height is > 0 but less than the font max-ascent? Can that ever happen? If not, we should assert it here.

Comment 5

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #4)
> +  if (mRect.height == 0) {
> +    // Empty inline frames will be pushed down in the line, so we need to
> +    // account for that here.
> +    baseline *= 0;
> 
> Why not just baseline = 0?

Oops, typo!

> What if the height is > 0 but less than the font max-ascent? Can that ever
> happen? If not, we should assert it here.

I don't think that should happen, so I don't think that we need to handle that.  But I agree that it's a very good idea to assert that here.

Comment 6

[(no longer active)]

Attachment: Patch (v3)

Added some test files to the patch.  hg addremove should be automatic...

Comment 7

[(no longer active)]

Attachment: Patch (v4)

The assertion message was correct, but I mistyped the comparison operator...  Sorry for so many iteration on this bug!

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/db8e95fbcd39

Comment 9

[ebrahim]

Thanks ehsan working well in night builds

I found other thing same this bug, if in empty dir="RTL" textarea you insert a latin char, in first inserting, cursor will moved to next line.

This can reproduce in both night builds and stable version.

Comment 10

[ebrahim]

Also I can reproduced that in other cases:

In normal textarea: (a persian char), space, backspace;
or (a persian char), backspace, (a persian char)

In RTL textarea: (a persian char), (latin char), backspace, (latin char)

And other states that I think can discovered.

Comment 11

[(no longer active)]

(In reply to comment #9)
> Thanks ehsan working well in night builds
> 
> I found other thing same this bug, if in empty dir="RTL" textarea you insert a
> latin char, in first inserting, cursor will moved to next line.
> 
> This can reproduce in both night builds and stable version.

That's bug 646382.

Comment 12

[ebrahim]

Thanks!

Comment 13

[(no longer active)]

Reopening to implement bug 646382 comment 10.

Comment 14

[(no longer active)]

Attachment: Alternate approach

Comment 15

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mozilla-central/rev/3c21288157a9