Last Comment Bug 291789 - Deletion of list items in a table cell is not working properly
: Deletion of list items in a table cell is not working properly
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Makoto Kato [:m_kato]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-25 06:17 PDT by Muni Sekhar Reddy
Modified: 2012-07-31 06:13 PDT (History)
4 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (613 bytes, text/html)
2005-04-25 06:19 PDT, Muni Sekhar Reddy
no flags Details
Indicating error condition when crossing boundary (1.28 KB, patch)
2005-04-25 07:19 PDT, Muni Sekhar Reddy
daniel: review+
tor: superreview+
Details | Diff | Splinter Review
Rebased against current trunk (1.02 KB, patch)
2012-07-16 05:57 PDT, Phoenix
no flags Details | Diff | Splinter Review
Patch part 1 -- Make various nsHTMLEditUtils methods take nsINode instead of Element (23.91 KB, patch)
2012-07-24 04:25 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2 -- Clean up nsHTMLEditRules::InDifferentTableElements (7.97 KB, patch)
2012-07-24 04:25 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3 -- Try not to ever delete across table boundaries (4.04 KB, patch)
2012-07-24 04:26 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review

Description Muni Sekhar Reddy 2005-04-25 06:17:40 PDT
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 2005-04-25 06:19:22 PDT
Created attachment 181765 [details]
Testcase
Comment 2 Muni Sekhar Reddy 2005-04-25 06:20:36 PDT
Please open the test case in composer and follow the recreation steps. 
Comment 3 Muni Sekhar Reddy 2005-04-25 07:19:48 PDT
Created attachment 181771 [details] [diff] [review]
Indicating error condition when crossing boundary

Proper error condition is retuned in the failing case.
Comment 4 zug_treno 2005-04-26 03:46:07 PDT
Reproducible with Mozilla 1.8b1 and Mozilla 1.8b2/20050424.
Comment 5 Daniel Glazman (:glazou) 2005-05-09 07:04:37 PDT
Nice catch.
Confirming bug; testing patch although it seems ok to me.
Comment 6 Daniel Glazman (:glazou) 2005-05-09 07:11:52 PDT
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!
Comment 7 Phoenix 2012-07-16 05:57:32 PDT
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...
Comment 8 :Ehsan Akhgari 2012-07-16 16:44:19 PDT
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 2012-07-17 00:27:16 PDT
(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 :Ehsan Akhgari 2012-07-17 08:05:04 PDT
That fine, thanks Phoenix for bringing this into my attention.  :-)

Aryeh, would you mind finishing up this patch please?
Comment 11 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-24 04:25:04 PDT
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.
Comment 12 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-24 04:25:36 PDT
Created attachment 645253 [details] [diff] [review]
Patch part 2 -- Clean up nsHTMLEditRules::InDifferentTableElements
Comment 13 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-24 04:26:35 PDT
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
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2012-07-24 04:31:06 PDT
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 :Ehsan Akhgari 2012-07-24 12:15:18 PDT
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) (next working March 28-April 26) 2012-07-25 01:27:07 PDT
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) (next working March 28-April 26) 2012-07-30 07:36:15 PDT
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

Note You need to log in before you can comment on or make changes to this bug.