Last Comment Bug 746515 - Prefer wrapping elements to adding style="" to them
: Prefer wrapping elements to adding style="" to them
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla15
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
: 781763 (view as bug list)
Depends on: 828931 833889 838633 824926
Blocks: 717789
  Show dependency treegraph
 
Reported: 2012-04-18 02:47 PDT by :Aryeh Gregor (away until August 15)
Modified: 2013-02-06 18:52 PST (History)
5 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1, v1 -- Clean up nsHTMLEditor::SetInlineProperty (11.26 KB, patch)
2012-04-18 07:11 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Clean up nsHTMLEditor::SetInlinePropertyOnNode (8.55 KB, patch)
2012-04-18 07:12 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Remove empty <font> as well as <span> (2.20 KB, patch)
2012-04-18 07:14 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 4, v1 -- Remove styles more aggressively in execCommand() (12.11 KB, patch)
2012-04-18 07:17 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch part 5, v1 -- Only add style="" to empty <span>s (5.83 KB, patch)
2012-04-19 05:04 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-04-18 02:47:58 PDT
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.
Comment 1 :Aryeh Gregor (away until August 15) 2012-04-18 04:02:50 PDT
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.
Comment 2 :Aryeh Gregor (away until August 15) 2012-04-18 07:11:45 PDT
Created attachment 616115 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsHTMLEditor::SetInlineProperty
Comment 3 :Aryeh Gregor (away until August 15) 2012-04-18 07:12:20 PDT
Created attachment 616116 [details] [diff] [review]
Patch part 2, v1 -- Clean up nsHTMLEditor::SetInlinePropertyOnNode
Comment 4 :Aryeh Gregor (away until August 15) 2012-04-18 07:14:16 PDT
Created attachment 616117 [details] [diff] [review]
Patch part 3, v1 -- Remove empty <font> as well as <span>

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).
Comment 5 :Aryeh Gregor (away until August 15) 2012-04-18 07:17:06 PDT
Created attachment 616118 [details] [diff] [review]
Patch part 4, v1 -- Remove styles more aggressively in execCommand()

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
Comment 6 Mozilla RelEng Bot 2012-04-18 14:02:10 PDT
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 :Ehsan Akhgari 2012-04-18 16:01:40 PDT
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.  :-)
Comment 8 :Aryeh Gregor (away until August 15) 2012-04-19 05:04:38 PDT
Created attachment 616521 [details] [diff] [review]
Patch part 5, v1 -- Only add style="" to empty <span>s

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.
Comment 9 :Aryeh Gregor (away until August 15) 2012-04-19 05:05:28 PDT
(That's the last patch in the series, BTW.)
Comment 10 Mozilla RelEng Bot 2012-04-19 10:30:46 PDT
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 :Ehsan Akhgari 2012-04-19 11:10:20 PDT
(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 Ryosuke Niwa 2012-04-19 11:33:59 PDT
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 :Ehsan Akhgari 2012-04-19 12:27:20 PDT
Does WebKit work around them in the test case that Aryeh mentioned?  If it does, can you please remove that work-around? :-)
Comment 14 :Ehsan Akhgari 2012-04-19 13:03:20 PDT
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! :-)
Comment 15 :Ehsan Akhgari 2012-04-19 13:27:28 PDT
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.
Comment 16 :Ehsan Akhgari 2012-04-19 13:31:03 PDT
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!
Comment 18 Ryosuke Niwa 2012-04-22 23:29:59 PDT
I've contacted Roland Steiner internally since he used to maintain the browser scope tests for editing.
Comment 19 Ryosuke Niwa 2012-04-23 00:38:25 PDT
Roland says you should upload a patch there if you can (he has transitioned to work on some other project).
Comment 20 Roland Steiner 2012-04-23 00:53:05 PDT
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 Roland Steiner 2012-04-23 00:53:37 PDT
s/files/tests/ ... :P
Comment 23 :Aryeh Gregor (away until August 15) 2012-05-02 03:13:09 PDT
*** Bug 717789 has been marked as a duplicate of this bug. ***
Comment 24 :Aryeh Gregor (away until August 15) 2012-08-14 02:27:09 PDT
*** Bug 781763 has been marked as a duplicate of this bug. ***
Comment 25 Daniel Glazman (:glazou) 2013-01-10 04:17:16 PST
The fix for this bug caused regression bug 828931

Note You need to log in before you can comment on or make changes to this bug.