Bug 24333

Summary:

execCommand('underline') can modify DOM outside of the contentEditable area

Product:

WebKit

Reporter:

Robby Walker <robbyw>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, jparent, justin.garcia, rniwa

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

OS X 10.5

Bug Depends on:

20215

Bug Blocks:

Attachments:

Description	Flags
Demonstration of the bug, note the HTML in the second textarea shows style has been removed from the outer DIV.	none
demonstrates the same bug on WebKit & Firefox	none
simplified test case (without designMode="on")	none
highestAncestorWithTextDecoration stops at an unsplittable element, and improves other text decorations support	eric: review-
smaller patch with extra test case for unsplittable element	justin.garcia: review+

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-	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
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.