Bug 24333 - execCommand('underline') can modify DOM outside of the contentEditable area
: execCommand('underline') can modify DOM outside of the contentEditable area
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: All Mac OS X 10.5
: P3 Normal
Assigned To: Ryosuke Niwa
:
Depends on: 20215
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-03 13:48 PST by Robby Walker
Modified: 2009-07-24 14:13 PDT (History)
4 users (show)

See Also:


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-
Details | Formatted Diff | Diff
smaller patch with extra test case for unsplittable element (7.68 KB, patch)
2009-07-24 12:51 PDT, Ryosuke Niwa
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robby Walker 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.
Comment 1 Robby Walker 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.
Comment 2 Ryosuke Niwa 2009-07-14 17:34:56 PDT
Created attachment 32751 [details]
demonstrates the same bug on WebKit & Firefox
Comment 3 Ryosuke Niwa 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.
Comment 4 Ryosuke Niwa 2009-07-14 18:39:14 PDT
It seems that highestAncestorWithTextDecoration called in ApplyStyleCommand::pushDownTextDecorationStyleAroundNode is crossing the editing boundary.
Comment 5 Ryosuke Niwa 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.
Comment 6 Eric Seidel 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.
Comment 7 Ryosuke Niwa 2009-07-21 17:14:41 PDT
Note: also see http://trac.webkit.org/changeset/8466
Comment 8 Ryosuke Niwa 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.
Comment 9 Eric Seidel 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.
Comment 10 Ryosuke Niwa 2009-07-24 12:51:56 PDT
Created attachment 33463 [details]
smaller patch with extra test case for unsplittable element
Comment 11 Justin Garcia 2009-07-24 13:49:23 PDT
Comment on attachment 33463 [details]
smaller patch with extra test case for unsplittable element

r=me
Comment 12 Ryosuke Niwa 2009-07-24 14:13:04 PDT
Landed in http://trac.webkit.org/changeset/46373