291789 - Deletion of list items in a table cell is not working properly

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Muni Sekhar Reddy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041217
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041217

Deletion of list iems in a table cell is not working properly. Delete  operation
is not stopping in the current cell if we start deletion not form the end of the
list item.  Instead it is going to the next cell also.

Reproducible: Always

Steps to Reproduce:
1.Open the test case
2. Put the cursor "second element22" any where in this item, but not at the end.  
3. Keep pressing the backspace.
4. Deletion continues to the next previous table cell also.

Actual Results:  
Text in the previous cell is also getting deleted.

Expected Results:  
Deletion should stop in the table cell.

I will upload the testcase shortly.

Comment 1

[Muni Sekhar Reddy]

Attachment: Testcase

Comment 2

[Muni Sekhar Reddy]

Please open the test case in composer and follow the recreation steps. 

Comment 3

[Muni Sekhar Reddy]

Attachment: Indicating error condition when crossing boundary

Proper error condition is retuned in the failing case.

Comment 4

[zug_treno]

Reproducible with Mozilla 1.8b1 and Mozilla 1.8b2/20050424.

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Nice catch.
Confirming bug; testing patch although it seems ok to me.

Comment 6

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 181771 [details] [diff] [review]
Indicating error condition when crossing boundary

r=[e-mail]
Get an sr and check it in! It will also be in Nvu 1.0.
Thanks!

Comment 7

[Phoenix]

Attachment: Rebased against current trunk

Rebased patch against current trunk. I also wondering, if same occurrence on line 2116 must be replaced...

Comment 8

[(no longer active)]

Comment on attachment 642550 [details] [diff] [review]
Rebased against current trunk

Can you please add an automated test case with the code change?  Also, I think that you only want to cancel if bInDifTblElems is true, the NS_FAILED check shouldn't do this.

Comment 9

[Phoenix]

(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> Comment on attachment 642550 [details] [diff] [review]
> Rebased against current trunk
> 
> Can you please add an automated test case with the code change?
Unfortunately, no, as I'm not a developer, just found patch which was reviewed and easy to rebase without programming knowledge.

Comment 10

[(no longer active)]

That fine, thanks Phoenix for bringing this into my attention.  :-)

Aryeh, would you mind finishing up this patch please?

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Make various nsHTMLEditUtils methods take nsINode instead of Element

Something like nsHTMLEditUtils::IsTableElement(node) on something that's not an element should just return false.  There shouldn't be any need to use AsElement() -- it complicates things unnecessarily.  If I want to special-case non-elements, I can do that separately.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 -- Clean up nsHTMLEditRules::InDifferentTableElements

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3 -- Try not to ever delete across table boundaries

The failure is not specific to list items.  The spec's tests test for it, happily.  Try: https://tbpl.mozilla.org/?tree=Try&rev=384f42ad0b95

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 645252 [details] [diff] [review]
Patch part 1 -- Make various nsHTMLEditUtils methods take nsINode instead of Element

Review of attachment 645252 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, though I'd prefer if you also updated the callers that do the IsElement() check before calling :)

Comment 15

[(no longer active)]

Comment on attachment 645252 [details] [diff] [review]
Patch part 1 -- Make various nsHTMLEditUtils methods take nsINode instead of Element

Review of attachment 645252 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with what Ms2ger said.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

New try run to make sure the new version of part 1 didn't blow stuff up: https://tbpl.mozilla.org/?tree=Try&rev=db205f19d23e

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Oops, I forgot about this!  The try run vanished into the ether, but I'm pretty sure I remember it was green, so I went ahead and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/eda4a70cc946
https://hg.mozilla.org/integration/mozilla-inbound/rev/680da0011a04
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c56a9dc330

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eda4a70cc946
https://hg.mozilla.org/mozilla-central/rev/680da0011a04
https://hg.mozilla.org/mozilla-central/rev/72c56a9dc330