RESOLVED FIXED 24333
execCommand('underline') can modify DOM outside of the contentEditable area 
https://bugs.webkit.org/show_bug.cgi?id=24333
Summary execCommand('underline') can modify DOM outside of the contentEditable area
Robby Walker
Reported 2009-03-03 13:48:28 PST
1. Start with the following HTML <div style="textDecoration: underline"><div id="editable">Text</div></div> 2. Set document.getElementById('editable').contentEditable = true 3. Select Text 4. Ctrl-U (or execCommand('underline') 5. Note that the style for the outer div has changed.
Attachments
Demonstration of the bug, note the HTML in the second textarea shows style has been removed from the outer DIV. (561 bytes, text/html)
2009-03-03 13:49 PST, Robby Walker 		no flags
Details
demonstrates the same bug on WebKit & Firefox (872 bytes, text/html)
2009-07-14 17:34 PDT, Ryosuke Niwa 		no flags
Details
simplified test case (without designMode="on") (800 bytes, text/html)
2009-07-14 17:40 PDT, Ryosuke Niwa 		no flags
Details
highestAncestorWithTextDecoration stops at an unsplittable element, and improves other text decorations support (10.98 KB, patch)
2009-07-20 16:14 PDT, Ryosuke Niwa 		eric: review-
DetailsFormatted DiffDiff
smaller patch with extra test case for unsplittable element (7.68 KB, patch)
2009-07-24 12:51 PDT, Ryosuke Niwa 		justin.garcia: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Robby Walker
Comment 1 2009-03-03 13:49:48 PST
Created attachment 28238 [details] Demonstration of the bug, note the HTML in the second textarea shows style has been removed from the outer DIV.
Ryosuke Niwa
Comment 2 2009-07-14 17:34:56 PDT
Created attachment 32751 [details] demonstrates the same bug on WebKit & Firefox
Ryosuke Niwa
Comment 3 2009-07-14 17:40:26 PDT
Created attachment 32754 [details] simplified test case (without designMode="on") The previous test case had the designMode="on". When turning this off, Firefox doesn't change the style at all.
Ryosuke Niwa
Comment 4 2009-07-14 18:39:14 PDT
It seems that highestAncestorWithTextDecoration called in ApplyStyleCommand::pushDownTextDecorationStyleAroundNode is crossing the editing boundary.
Ryosuke Niwa
Comment 5 2009-07-20 16:14:07 PDT
Created attachment 33120 [details] highestAncestorWithTextDecoration stops at an unsplittable element, and improves other text decorations support This patch may also resolve https://bugs.webkit.org/show_bug.cgi?id=20215.
Eric Seidel (no email)
Comment 6 2009-07-21 15:59:55 PDT
Comment on attachment 33120 [details] highestAncestorWithTextDecoration stops at an unsplittable element, and improves other text decorations support This makes unrelated, possibly incorrect changes. Please break this into multiple patches with individual test cases. See the test case on https://bugs.webkit.org/show_bug.cgi?id=20215, I think you may have broken some of those cases here.
Ryosuke Niwa
Comment 7 2009-07-21 17:14:41 PDT
Note: also see http://trac.webkit.org/changeset/8466
Ryosuke Niwa
Comment 8 2009-07-24 12:04:07 PDT
Changed the priority because this has potential security vulnerability that part of document that the author does not expect to change may change.
Eric Seidel (no email)
Comment 9 2009-07-24 12:20:43 PDT
I respectfully disagree. http://webkit.org/quality/bugpriorities.html I don't think there is any "security" concern here. If there was it should be under the Security component anyway. P1s are generally regressions or crashes, of which this is neither.
Ryosuke Niwa
Comment 10 2009-07-24 12:51:56 PDT
Created attachment 33463 [details] smaller patch with extra test case for unsplittable element
Justin Garcia
Comment 11 2009-07-24 13:49:23 PDT
Comment on attachment 33463 [details] smaller patch with extra test case for unsplittable element r=me
Ryosuke Niwa
Comment 12 2009-07-24 14:13:04 PDT
Landed in http://trac.webkit.org/changeset/46373
Note You need to log in before you can comment on or make changes to this bug.