Closed
Bug 746515
Opened 13 years ago
Closed 13 years ago
Prefer wrapping elements to adding style="" to them
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: ayg, Assigned: ayg)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(5 files)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
Test-case:
data:text/html,<!doctype html>
<div contenteditable>foo<i>bar</i></div>
<script>
getSelection().selectAllChildren(document.body.firstChild);
document.execCommand("stylewithcss", false, true);
document.execCommand("bold");
document.body.textContent = document.body.firstChild.innerHTML;
</script>
Gecko: <span style="font-weight: bold;">foo</span><i style="font-weight: bold;">bar</i>
WebKit: <span style="font-weight: bold;">foo<i>bar</i></span>
WebKit is more succinct, and it's what the spec requires.
Assignee | ||
Comment 1•13 years ago
|
||
Another example:
data:text/html,<!doctype html>
<div contenteditable><u>foo</u><i>bar</i></div>
<script>
getSelection().selectAllChildren(document.body.firstChild);
document.execCommand("stylewithcss", false, true);
document.execCommand("bold");
document.body.textContent = document.body.firstChild.innerHTML;
</script>
Gecko: <u style="font-weight: bold;">foo</u><i style="font-weight: bold;">bar</i>
WebKit: <span style="font-weight: bold;"><u>foo</u><i>bar</i></span>
In this case Gecko has fewer elements, but WebKit is still more succinct in terms of bytes, and the spec still mandates what WebKit does.
Now for a more interesting question:
data:text/html,<!doctype html>
<div contenteditable><i>foo</i></div>
<script>
getSelection().selectAllChildren(document.body.firstChild);
document.execCommand("stylewithcss", false, true);
document.execCommand("bold");
document.body.textContent = document.body.firstChild.innerHTML;
</script>
Both Gecko and WebKit say <i style="font-weight: bold;">foo</i>, and this is shortest. But Gecko and the spec both do styling node-by-node, so this case can't be distinguished from the last while we're looking at the first node. Also, making the one-node and two-node cases different means you get different markup in the last case if you first bold "foo" and then "bar", than if you bold both at once.
The spec says to only ever add style="" to empty spans. This also makes CSS and non-CSS mode more closely analogous -- every time non-CSS mode would add an element, so does CSS mode. This is more likely to prevent discrepancies. E.g., consider
data:text/html,<!doctype html>
<div contenteditable><font color=red>foo</font><br><font color=red>bar</font></div>
<script>
getSelection().selectAllChildren(document.body.firstChild.firstChild);
document.execCommand("stylewithcss", false, true);
document.execCommand("underline");
getSelection().selectAllChildren(document.body.firstChild.childNodes[2]);
document.execCommand("stylewithcss", false, false);
document.execCommand("underline");
getSelection().removeAllRanges();
</script>
In Gecko, "foo"'s underline is red, while "bar"'s is black. This is because CSS mode adds text-decoration: underline to the <font>, while non-CSS mode nests it in a <u>, and the color of the underline is the color of the element that generates it.
So my patch here is going to make Gecko only ever add style="" to an empty span. If the current node isn't an empty span (after removing styles), it will get wrapped.
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #616115 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•13 years ago
|
||
Attachment #616116 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•13 years ago
|
||
This patch is needed because if we get a <font> with only a style attribute, previously we'd replace the existing style attribute. With the final patch in this series, we would instead remove the attribute when clearing styles, and then add a span wrapper, leaving an empty <font>. The editing spec requires the behavior of this patch, and richtext tests for it (maybe richtext2 as well).
Attachment #616117 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•13 years ago
|
||
This is the first nontrivial patch in the series. It's probably the last I'll write today, but it doesn't fix this actual bug. It does clean up markup in some cases, however, and fixes some richtext2 failures. For instance, when italicizing
foo[bar<i>baz]</i>qoz
in CSS mode, we'd previously produce
foo[<span style="font-style: italic">bar</span><i style="font-style: italic">baz</i>]</i>qoz
or such. Now we just produce
foo[<span style="font-style: italic">barbaz</span>]</i>qoz
which is simpler.
I honestly don't remember what exactly this had to do with this bug, but it's a change we should have anyway. :)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=feca51d267cc
Attachment #616118 -
Flags: review?(ehsan)
Comment 6•13 years ago
|
||
Try run for feca51d267cc is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=feca51d267cc
Results (out of 222 total builds):
exception: 1
success: 185
warnings: 36
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-feca51d267cc
Comment 7•13 years ago
|
||
In the interest of giving a better review here, I'll hold off until you write all of the patches for this bug, since reviewing this requires keeping everything related to setting inline props in my head, and I don't want to miss something by reviewing this in two separate sessions. Sorry for the inconvenience. :-)
Assignee | ||
Comment 8•13 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=0e0015d5e101
This fixes one richtext2 test, italicizing
foo[bar<b>baz]</b>qoz
in CSS mode, where previously we'd do
foo<span style="font-style: italic;">bar</span><b style="font-style: italic;">baz</b>qoz
but now we do
foo<span style="font-style: italic;">bar<b>baz</b></span>qoz
which is better.
It regresses two tests where the richtext2 authors want special handling for class="Apple-style-span". It's the same test twice, once for backColor and once for hiliteColor: the input is
<span class="Apple-style-span" style="background-color: rgb(255, 0, 0)">[foobarbaz]</span>
and it changes the background to #ace. Previously we'd replace the existing style attribute, which is what it wanted. Now we strip the style attribute, notice that <span class="Apple-style-span"> is not <span>, and wrap it in a new <span> instead of adding a new style attribute to it, so we get something like
<span style="background-color: rgb(170, 204, 238);"><span class="Apple-style-span">[foobarbaz]</span></span>
which it doesn't like. IMO, the test is wrong here. Gecko with this patch is per spec, WebKit is not. If you want us to special-case Apple-style-span here, we can, and I'll change the spec to follow the majority. But I don't see a compat gain from it -- it will just make the source HTML tidier if old WebKit has edited the page, at the cost of adding an ugly special case.
Attachment #616521 -
Flags: review?(ehsan)
Assignee | ||
Comment 9•13 years ago
|
||
(That's the last patch in the series, BTW.)
Comment 10•13 years ago
|
||
Try run for 0e0015d5e101 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=0e0015d5e101
Results (out of 223 total builds):
success: 183
warnings: 40
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-0e0015d5e101
Comment 11•13 years ago
|
||
(In reply to Aryeh Gregor from comment #8)
> which it doesn't like. IMO, the test is wrong here. Gecko with this patch
> is per spec, WebKit is not. If you want us to special-case Apple-style-span
> here, we can, and I'll change the spec to follow the majority. But I don't
> see a compat gain from it -- it will just make the source HTML tidier if old
> WebKit has edited the page, at the cost of adding an ugly special case.
Oh, the test is definitely wrong here. (Please file an issue with browserscope about this.) IIRC WebKit was unwilling to remove the the special handling for Apple-style-span based on concerns about non-browser WebKit embedders (rniwa, please correct me if I'm wrong) but that doesn't mean that we should add hacks around that in other engines, and it definitely doesn't mean that we should spec that brokenness! ;-)
Comment 12•13 years ago
|
||
I don't think we want to special Apple-style-span. It's dead. I'm sorry you have to deal with this problem, and you're free to strip them but WebKit no longer generates class="Apple-style-span" so I don't think we should spec it either.
Comment 13•13 years ago
|
||
Does WebKit work around them in the test case that Aryeh mentioned? If it does, can you please remove that work-around? :-)
Updated•13 years ago
|
Attachment #616115 -
Flags: review?(ehsan) → review+
Comment 14•13 years ago
|
||
Comment on attachment 616116 [details] [diff] [review]
Patch part 2, v1 -- Clean up nsHTMLEditor::SetInlinePropertyOnNode
Review of attachment 616116 [details] [diff] [review]:
-----------------------------------------------------------------
::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +496,1 @@
> {
Nit: please fix this while you're here too! :-)
Attachment #616116 -
Flags: review?(ehsan) → review+
Updated•13 years ago
|
Attachment #616117 -
Flags: review?(ehsan) → review+
Comment 15•13 years ago
|
||
Comment on attachment 616118 [details] [diff] [review]
Patch part 4, v1 -- Remove styles more aggressively in execCommand()
Review of attachment 616118 [details] [diff] [review]:
-----------------------------------------------------------------
::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ -361,2 @@
> {
> - NS_ENSURE_TRUE(aNode && aProperty, NS_ERROR_NULL_POINTER);
Please MOZ_ASSERT this condition here.
Attachment #616118 -
Flags: review?(ehsan) → review+
Comment 16•13 years ago
|
||
Comment on attachment 616521 [details] [diff] [review]
Patch part 5, v1 -- Only add style="" to empty <span>s
Review of attachment 616521 [details] [diff] [review]:
-----------------------------------------------------------------
Looks great!
Attachment #616521 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 17•13 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> Oh, the test is definitely wrong here. (Please file an issue with
> browserscope about this.)
I filed an issue. But I don't seem to be getting any responses:
http://code.google.com/p/browserscope/issues/list?can=1&q=reporter%3ASimetrical%40gmail.com&colspec=ID+Type+Status+Priority+Milestone+Owner+Summary&cells=tiles
http://code.google.com/p/browserscope/issues/list?can=1&q=reporter%3Aayg%40aryeh.name&colspec=ID+Type+Status+Priority+Milestone+Owner+Summary&cells=tiles
:(
http://hg.mozilla.org/projects/birch/rev/2a0c9db01de4
http://hg.mozilla.org/projects/birch/rev/7020b2712513
http://hg.mozilla.org/projects/birch/rev/2909ffd3a1b6
http://hg.mozilla.org/projects/birch/rev/6306ebb388f0
http://hg.mozilla.org/projects/birch/rev/568ff98bad40
Status: NEW → ASSIGNED
Flags: in-testsuite+
Target Milestone: --- → mozilla15
Comment 18•13 years ago
|
||
I've contacted Roland Steiner internally since he used to maintain the browser scope tests for editing.
Comment 19•13 years ago
|
||
Roland says you should upload a patch there if you can (he has transitioned to work on some other project).
Comment 20•13 years ago
|
||
To elaborate: It will take me a few weeks to be able to address Browserscope issues again after my transition (I'm not set up to do so ATM), so if you could file patches directly, that would be greatly helpful.
I set up the test file structure to be easy to understand and readable (I hope), so that amending, adding and removing files should be straightforward. If you have questions, please contact me.
Comment 21•13 years ago
|
||
s/files/tests/ ... :P
Comment 22•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a0c9db01de4
https://hg.mozilla.org/mozilla-central/rev/7020b2712513
https://hg.mozilla.org/mozilla-central/rev/2909ffd3a1b6
https://hg.mozilla.org/mozilla-central/rev/6306ebb388f0
https://hg.mozilla.org/mozilla-central/rev/568ff98bad40
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 25•12 years ago
|
||
The fix for this bug caused regression bug 828931
You need to log in
before you can comment on or make changes to this bug.
Description
•