Summary: | execCommand with super/subscript doesn't toggle back to baseline | ||
---|---|---|---|
Product: | WebKit | Reporter: | Tom Gilder <tom> |
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | eric, futurama, grossmann.mark, jparent, justin.garcia, mrowe |
Priority: | P2 | Keywords: | GoogleBug, HasReduction, InRadar |
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | OS X 10.5 | ||
URL: | http://tom.me.uk/2008/3/webkit-execcommand.html | ||
Bug Depends on: | 23366 | ||
Bug Blocks: | |||
Attachments: |
Description
Tom Gilder
2008-03-09 18:12:28 PDT
Confirmed in Safari 3.0.4 and TOT WebKit. Created attachment 21304 [details]
Test case (from URL)
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.
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. (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). (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... 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. 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 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.
(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. (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! 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(-)
Thanks for the cookie Julie! http://trac.webkit.org/changeset/40097 |