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
: WebKit
HTML Editing
: 528+ (Nightly build)
: All Mac OS X 10.5
: P3 Normal
Assigned To:
:
:
: 20215
:
  Show dependency treegraph
 
Reported: 2009-03-03 13:48 PST by
Modified: 2009-07-24 14:13 PST (History)


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 PST, Ryosuke Niwa
no flags Details
simplified test case (without designMode="on") (800 bytes, text/html)
2009-07-14 17:40 PST, 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 PST, Ryosuke Niwa
eric: review-
Review Patch | Details | Formatted Diff | Diff
smaller patch with extra test case for unsplittable element (7.68 KB, patch)
2009-07-24 12:51 PST, Ryosuke Niwa
justin.garcia: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 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 From 2009-03-03 13:49:48 PST -------
Created an attachment (id=28238) [details]
Demonstration of the bug, note the HTML in the second textarea shows style has been removed from the outer DIV.
------- Comment #2 From 2009-07-14 17:34:56 PST -------
Created an attachment (id=32751) [details]
demonstrates the same bug on WebKit & Firefox
------- Comment #3 From 2009-07-14 17:40:26 PST -------
Created an attachment (id=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 From 2009-07-14 18:39:14 PST -------
It seems that highestAncestorWithTextDecoration called in ApplyStyleCommand::pushDownTextDecorationStyleAroundNode is crossing the editing boundary.
------- Comment #5 From 2009-07-20 16:14:07 PST -------
Created an attachment (id=33120) [details]
fixes the bug

This patch may also resolve https://bugs.webkit.org/show_bug.cgi?id=20215.
------- Comment #6 From 2009-07-21 15:59:55 PST -------
(From update of attachment 33120 [details])
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 From 2009-07-21 17:14:41 PST -------
Note: also see http://trac.webkit.org/changeset/8466
------- Comment #8 From 2009-07-24 12:04:07 PST -------
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 From 2009-07-24 12:20:43 PST -------
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 From 2009-07-24 12:51:56 PST -------
Created an attachment (id=33463) [details]
smaller patch with extra test case for unsplittable element
------- Comment #11 From 2009-07-24 13:49:23 PST -------
(From update of attachment 33463 [details])
r=me
------- Comment #12 From 2009-07-24 14:13:04 PST -------
Landed in http://trac.webkit.org/changeset/46373