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

RESOLVED FIXED in mozilla17

Status

()

Core
Editor
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: Muni Sekhar Reddy, Assigned: ayg)

Tracking

({testcase})

Trunk
mozilla17
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 181765 [details]
Testcase
(Reporter)

Comment 2

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

Updated

12 years ago
Keywords: testcase
(Reporter)

Comment 3

12 years ago
Created attachment 181771 [details] [diff] [review]
Indicating error condition when crossing boundary

Proper error condition is retuned in the failing case.

Comment 4

12 years ago
Reproducible with Mozilla 1.8b1 and Mozilla 1.8b2/20050424.

Updated

12 years ago
Summary: Deletion of list iems in a table cell is not working properly → Deletion of list items in a table cell is not working properly
Version: unspecified → Trunk
Nice catch.
Confirming bug; testing patch although it seems ok to me.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 181771 [details] [diff] [review]
Indicating error condition when crossing boundary

r=daniel@glazman.org
Get an sr and check it in! It will also be in Nvu 1.0.
Thanks!
Attachment #181771 - Flags: review+

Updated

12 years ago
Attachment #181771 - Flags: superreview?(tor)

Updated

12 years ago
Attachment #181771 - Flags: superreview?(tor) → superreview+
Assignee: composer → nobody
QA Contact: composer

Comment 7

5 years ago
Created attachment 642550 [details] [diff] [review]
Rebased against current trunk

Rebased patch against current trunk. I also wondering, if same occurrence on line 2116 must be replaced...
Attachment #642550 - Flags: review?(ehsan)
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.
Attachment #642550 - Flags: review?(ehsan)

Comment 9

5 years ago
(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.
That fine, thanks Phoenix for bringing this into my attention.  :-)

Aryeh, would you mind finishing up this patch please?
Created attachment 645252 [details] [diff] [review]
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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #645252 - Flags: review?(ehsan)
Attachment #645252 - Flags: feedback?(Ms2ger)
Created attachment 645253 [details] [diff] [review]
Patch part 2 -- Clean up nsHTMLEditRules::InDifferentTableElements
Attachment #645253 - Flags: review?(ehsan)
Created attachment 645254 [details] [diff] [review]
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
Attachment #645254 - Flags: review?(ehsan)
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 :)
Attachment #645252 - Flags: feedback?(Ms2ger)
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.
Attachment #645252 - Flags: review?(ehsan) → review+
Attachment #645253 - Flags: review?(ehsan) → review+
Attachment #645254 - Flags: review?(ehsan) → review+
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
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
Component: Composer → Editor
Flags: in-testsuite+
OS: Windows XP → All
Product: SeaMonkey → Core
Hardware: x86 → All
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/eda4a70cc946
https://hg.mozilla.org/mozilla-central/rev/680da0011a04
https://hg.mozilla.org/mozilla-central/rev/72c56a9dc330
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.