Bug 17733 – execCommand with super/subscript doesn't toggle back to baseline

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Tom Gilder]

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.

[Mark Rowe (bdash)]

Confirmed in Safari 3.0.4 and TOT WebKit.

[Mark Rowe (bdash)]

<rdar://problem/5803699>

[David Kilzer (:ddkilzer)]

Created attachment 21304 [details]
Test case (from URL)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Mark Grossmann]

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

[David Bloom]

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

[David Bloom]

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.

[Eric Seidel (no email)]

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(-)

[Justin Garcia]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

(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!

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

Thanks for the cookie Julie!
http://trac.webkit.org/changeset/40097