Closed Bug 746515 Opened 12 years ago Closed 12 years ago

Prefer wrapping elements to adding style="" to them

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: ayg, Assigned: ayg)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(5 files)

Patch part 1, v1 -- Clean up nsHTMLEditor::SetInlineProperty
11.26 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Clean up nsHTMLEditor::SetInlinePropertyOnNode
8.55 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Remove empty <font> as well as <span>
2.20 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 4, v1 -- Remove styles more aggressively in execCommand()
12.11 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 5, v1 -- Only add style="" to empty <span>s
5.83 KB, 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.
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.
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)
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)
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
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.  :-)
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)
(That's the last patch in the series, BTW.)
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
(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!  ;-)
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.
Does WebKit work around them in the test case that Aryeh mentioned?  If it does, can you please remove that work-around? :-)
Attachment #616115 - Flags: review?(ehsan) → review+
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+
Attachment #616117 - Flags: review?(ehsan) → review+
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 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+
I've contacted Roland Steiner internally since he used to maintain the browser scope tests for editing.
Roland says you should upload a patch there if you can (he has transitioned to work on some other project).
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.
s/files/tests/ ... :P
The fix for this bug caused regression bug 828931
Depends on: 828931
Depends on: 833889
Depends on: 824926
Depends on: 838633
Depends on: 889940
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: