Bug 17733 - execCommand with super/subscript doesn't toggle back to baseline
Summary: execCommand with super/subscript doesn't toggle back to baseline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://tom.me.uk/2008/3/webkit-execco...
Keywords: GoogleBug, HasReduction, InRadar
Depends on: 23366
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-09 18:12 PDT by Tom Gilder
Modified: 2009-01-21 14:19 PST (History)
6 users (show)

See Also:


Attachments
Test case (from URL) (1.14 KB, text/html)
2008-05-22 14:20 PDT, David Kilzer (:ddkilzer)
no flags Details
Naive fix (1.01 KB, patch)
2008-05-22 14:45 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix execCommand() 'super' and 'sub' commands to add <sup> and <sub> in quirks mode, and to toggle when called twice (13.29 KB, patch)
2009-01-16 17:58 PST, Eric Seidel (no email)
justin.garcia: review+
Details | Formatted Diff | Diff
Fix execCommand() 'super' and 'sub' commands to add <sup> and <sub> in quirks mode, and to toggle when called twice (14.37 KB, patch)
2009-01-21 14:12 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Gilder 2008-03-09 18:12:28 PDT
Calling execCommand('Superscript') or execCommand('Subscript') correctly subs or supers the text, but calling it again doesn't un-do this, so the text doesn't return to baseline, unlike IE. Every other execCommand (such as Bold) does toggle. 

See URL for testcase, select text below buttons and click a button twice. Text should go back to normal on second click.
Comment 1 Mark Rowe (bdash) 2008-03-09 18:13:33 PDT
Confirmed in Safari 3.0.4 and TOT WebKit.
Comment 2 Mark Rowe (bdash) 2008-03-17 15:21:01 PDT
<rdar://problem/5803699>
Comment 3 David Kilzer (:ddkilzer) 2008-05-22 14:20:15 PDT
Created attachment 21304 [details]
Test case (from URL)
Comment 4 Eric Seidel (no email) 2008-05-22 14:45:38 PDT
Created attachment 21305 [details]
Naive fix

I didn't run the layout tests or write at test case.  But this does fix the bug as described.
Comment 5 Eric Seidel (no email) 2008-05-22 15:06:23 PDT
If you're using this in an application which needs a workaround in shipping versions of Safari, I would encourage you to use document.executeCommand("Unscript"); to remove any subscript/superscripts.  That should work in both IE and Safari.  Sadly, that also means you have to maintain your own "is this subscripted/superscripted" state.  You could use getComputedStyle on some portion of the selected text in order to discover the vertical-align value however.
Comment 6 Mark Grossmann 2008-05-25 05:08:25 PDT
(In reply to comment #5)
> If you're using this in an application which needs a workaround in shipping
> versions of Safari, I would encourage you to use
> document.executeCommand("Unscript"); to remove any subscript/superscripts. 
> That should work in both IE and Safari.  Sadly, that also means you have to
> maintain your own "is this subscripted/superscripted" state.  You could use
> getComputedStyle on some portion of the selected text in order to discover the
> vertical-align value however.
> 
For some reason it did not work for me in Safari 3.0.3 and 3.1.1. I used the same test case and simply added another button with onclick="document.execCommand('Unscript')".
It does not work in IE6/7 either (results in js exception). 

Comment 7 David Bloom 2008-08-18 09:50:33 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > If you're using this in an application which needs a workaround in shipping
> > versions of Safari, I would encourage you to use
> > document.executeCommand("Unscript"); to remove any subscript/superscripts. 
> > That should work in both IE and Safari.  Sadly, that also means you have to
> > maintain your own "is this subscripted/superscripted" state.  You could use
> > getComputedStyle on some portion of the selected text in order to discover the
> > vertical-align value however.
> > 
> For some reason it did not work for me in Safari 3.0.3 and 3.1.1. I used the
> same test case and simply added another button with
> onclick="document.execCommand('Unscript')".
> It does not work in IE6/7 either (results in js exception). 
> 

I have no idea what's up with IE6/7 supporting this...it doesn't appear on MSDN anywhere.

In any case, http://svn.webkit.org/repository/webkit/trunk/WebCore/editing/EditorCommand.cpp seems to limit the "unscript" command identifier to "supportedFromMenuOrKeyBinding". So, this command identifier is useless for web developers...
Comment 8 David Bloom 2008-08-19 11:11:33 PDT
Slightly offtopic, but is there any reason that WebKit doesn't use the <sup> and <sub> elements instead of an Apple-style-span? <sup> and <sub> aren't deprecated, and superscript/subscript do have some semantic meaning, so I don't understand why they aren't used. Other browsers usually can't remove Apple-style-span formatting, so Safari's current behavior is less interoperable in a collaborative and/or multiple-browser environment.
Comment 9 Eric Seidel (no email) 2009-01-16 17:58:56 PST
Created attachment 26819 [details]
Fix execCommand() 'super' and 'sub' commands to add <sup> and <sub> in quirks mode, and to toggle when called twice

 LayoutTests/ChangeLog                              |    9 ++
 .../editing/execCommand/toggle-styles-expected.txt |   12 ++--
 WebCore/ChangeLog                                  |   24 ++++++
 WebCore/editing/ApplyStyleCommand.cpp              |   84 ++++++++++++++------
 WebCore/editing/EditorCommand.cpp                  |    4 +-
 WebCore/editing/htmlediting.cpp                    |    7 ++-
 WebCore/editing/htmlediting.h                      |    1 +
 7 files changed, 107 insertions(+), 34 deletions(-)
Comment 10 Justin Garcia 2009-01-20 14:17:14 PST
Comment on attachment 26819 [details]
Fix execCommand() 'super' and 'sub' commands to add <sup> and <sub> in quirks mode, and to toggle when called twice

-FAIL superscript removing vertical-align: superscript -- <span><span class="Apple-style-span" style="vertical-align: super;">test</span></span>
+FAIL superscript removing vertical-align: superscript -- <span><sup>test</sup></span>

What's up here?

Rest looks good, r=me.
Comment 11 Eric Seidel (no email) 2009-01-20 14:29:44 PST
(In reply to comment #10)
> (From update of attachment 26819 [details] [review])
> -FAIL superscript removing vertical-align: superscript -- <span><span
> class="Apple-style-span" style="vertical-align: super;">test</span></span>
> +FAIL superscript removing vertical-align: superscript --
> <span><sup>test</sup></span>
> 
> What's up here?

Hum... you're right.  That's still wrong.  I don't know how I didn't notice.  The <sup> tags shouldn't be there.
> Rest looks good, r=me.
Comment 12 Eric Seidel (no email) 2009-01-21 14:02:25 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 26819 [details] [review] [review])
> > -FAIL superscript removing vertical-align: superscript -- <span><span
> > class="Apple-style-span" style="vertical-align: super;">test</span></span>
> > +FAIL superscript removing vertical-align: superscript --
> > <span><sup>test</sup></span>
> > 
> > What's up here?
> 
> Hum... you're right.  That's still wrong.  I don't know how I didn't notice. 
> The <sup> tags shouldn't be there.

Ha!  Turns out the behavior was correct.  The test was incorrectly using "vertical-align: superscript;" which is an invalid value (should be "vertical-align: super"), thus the resulting resolved style was still vertical-align: baseline, and the editing command worked as intended.  It removed the bogus vertical-align value, and applied the superscript/subscript using a "legacy" tag.

I'll upload the updated patch which I'm about to land.  Thanks again!

Comment 13 Eric Seidel (no email) 2009-01-21 14:12:00 PST
Created attachment 26904 [details]
Fix execCommand() 'super' and 'sub' commands to add <sup> and <sub> in quirks mode, and to toggle when called twice

 LayoutTests/ChangeLog                              |    9 ++
 .../editing/execCommand/resources/toggle-styles.js |    4 +-
 .../editing/execCommand/toggle-styles-expected.txt |   12 ++--
 WebCore/ChangeLog                                  |   24 ++++++
 WebCore/editing/ApplyStyleCommand.cpp              |   84 ++++++++++++++------
 WebCore/editing/EditorCommand.cpp                  |    4 +-
 WebCore/editing/htmlediting.cpp                    |    7 ++-
 WebCore/editing/htmlediting.h                      |    1 +
 8 files changed, 109 insertions(+), 36 deletions(-)
Comment 14 Eric Seidel (no email) 2009-01-21 14:19:27 PST
Thanks for the cookie Julie!
http://trac.webkit.org/changeset/40097