482481 - Commands doesn't work on contained contentEditable elements

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[spam]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre

execCommands like bold doesn't work correctly if a contentEditable element is contained within a wrapper element.

Reproducible: Always

Steps to Reproduce:
1. Open attached file or URL.
2. Select all contents using Ctrl+A.
3. Click the Bold link.
4. Notice that the bold doesn't apply.
Actual Results:  
The bold command doesn't do anything.

Expected Results:  
Should have wrapped the text in a strong or b element.

Comment 1

[spam]

Attachment: Testcase displaying the bug

Comment 2

[arno renevier]

If I understand correctly, problem happens because editor select  "any ancestors who's children are entirely selected" before applying a command. Then, outer div is selected, and as it's not editable, command does not apply.

Comment 3

[arno renevier]

Attachment: patch v1 [Checkin: Comment 11]

Comment 4

[spam]

I'm not sure that the container selection is the problem. I'm attaching a second test case could you verify that this scenario is resolved by the patch as well. It selects the text node contents of the paragraph.

Comment 5

[spam]

Attachment: Additional test case.

Additional test case selecting the text node contents and not the entire paragraph.

Comment 6

[arno renevier]

second testcase fails without patch and is successful with patch applied.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 366796 [details] [diff] [review]
patch v1
[Checkin: Comment 11]

Please add the testcases to the testsuite.

Comment 8

[Markus Stange [:mstange]]

Should this patch be checked in without the tests, or do you want to make a new patch with tests?

Comment 9

[arno renevier]

I should probably add some tests.

Comment 10

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

I'd rather have the patch without tests than no patch at all.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 366796 [details] [diff] [review]
patch v1
[Checkin: Comment 11]


http://hg.mozilla.org/mozilla-central/rev/0fe2dc64d480

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 366796 [details] [diff] [review]
patch v1
[Checkin: Comment 11]

Can we get one of these testcases incorporated into our automated test-runs as part of the patch?

Comment 13

[Samuel Sidler (old account; do not CC)]

Comment on attachment 366796 [details] [diff] [review]
patch v1
[Checkin: Comment 11]

Please re-request approval after answering comment 12.