Bug 17926

Summary: Unformat deletes images
Product: WebKit Reporter: Anders Jenbo <fuck>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: jparent, justin.garcia, ojan, rniwa, tkent, tony
Priority: P2 Keywords: NeedsReduction
Version: 525.x (Safari 3.1)   
Hardware: All   
OS: All   
URL: http://rtef.info/demo.htm
Bug Depends on: 43017    
Bug Blocks:    
Attachments:
Description Flags
Patch ojan: review+

Anders Jenbo
Reported 2008-03-18 16:41:40 PDT
Go to http://rtef.info/demo.htm Switch to HTML mode Insert this code Please don't kill my brother, <img src="http://www.dcs.dk/imgNewDesign/frontBrother.gif" alt="Brother" /> Switch to RichText mode Select all content (button 1 or how ever you do it), hit unformat (button 2)
Attachments
Patch (2.78 KB, patch)
2010-10-25 14:02 PDT, Ryosuke Niwa 		ojan: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Anders Jenbo
Comment 1 2008-03-18 16:51:51 PDT
Probably should mention that unfromat calls "rteCommand('RemoveFormat') rteCommand('JustifyNone') and rteCommand('FormatBlock', '<p>') on the selected contnet.
Jeff
Comment 2 2009-01-09 18:54:43 PST
I have the same issue when executing : document.execCommand('RemoveFormat',false,null); on a mix text / img, img are deleted. I avoid selection on img by adding "-khtml-user-select:none" on them same problem. another thing the text lost class inherit properties <div id='editor' class='textclass'> .... my text ... </div> When apply "RemoveFormat" the text CSS class might be 'textclass' ! Same problem on Chrome Regards Jeff
Ryosuke Niwa
Comment 3 2010-10-25 14:02:24 PDT
Created attachment 71792 [details] Patch
Ryosuke Niwa
Comment 4 2010-10-25 14:03:31 PDT
This bug has been fixed since http://trac.webkit.org/changeset/70283. I'm adding two more tests to make sure we're not removing any images.
Ojan Vafai
Comment 5 2010-10-25 14:25:55 PDT
Comment on attachment 71792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71792&action=review > LayoutTests/editing/execCommand/remove-format-image.html:2 > +<html> Don't need this tag. It just adds clutter. The doctype is useful because it forces standards mode though. > LayoutTests/editing/execCommand/remove-format-image.html:3 > +<body onload="runTests();"> Do you need to run this onload? Can't you just run the script inline? If you do that, you won't need the body tag in the test. > LayoutTests/editing/execCommand/remove-format-image.html:5 > +<div id="test1" contenteditable><storng>hello</strong> <img src="../resources/abe.png"> world</div> typo:storng
Ryosuke Niwa
Comment 6 2010-10-25 14:43:33 PDT
(In reply to comment #5) > > LayoutTests/editing/execCommand/remove-format-image.html:2 > > +<html> > > Don't need this tag. It just adds clutter. The doctype is useful because it forces standards mode though. I don't think having <html> adds that much of cluttering. I think most of tests that do have DOCTYPE also has html, and I'd rather conform to that trend. Are you strongly inclined to remove this before landing? > > LayoutTests/editing/execCommand/remove-format-image.html:3 > > +<body onload="runTests();"> > > Do you need to run this onload? Can't you just run the script inline? If you do that, you won't need the body tag in the test. Yes. Images aren't necessarily loaded by the time the script is ran, and we get a different result when that happens. Or otherwise we'll make this test flaky. > > LayoutTests/editing/execCommand/remove-format-image.html:5 > > +<div id="test1" contenteditable><storng>hello</strong> <img src="../resources/abe.png"> world</div> > > typo:storng Oops, thanks. Will fix.
Ojan Vafai
Comment 7 2010-10-25 15:08:34 PDT
(In reply to comment #6) > (In reply to comment #5) > > > LayoutTests/editing/execCommand/remove-format-image.html:2 > > > +<html> > > > > Don't need this tag. It just adds clutter. The doctype is useful because it forces standards mode though. > > I don't think having <html> adds that much of cluttering. I think most of tests that do have DOCTYPE also has html, and I'd rather conform to that trend. Are you strongly inclined to remove this before landing? No. Either way is fine. While I agree that many tests have the html tag, I don't personally like it. The smaller a test is, the easier it is to make sense of and maintain.
Ryosuke Niwa
Comment 8 2010-10-25 15:18:00 PDT
Committed r70493: <http://trac.webkit.org/changeset/70493>
Ryosuke Niwa
Comment 9 2010-10-25 15:19:08 PDT
Thanks for the review, Ojan.
Note You need to log in before you can comment on or make changes to this bug.