The default bug view has changed. See this FAQ.

Prefer wrapping elements to adding style="" to them

RESOLVED FIXED in mozilla15

Status

()

Core
Editor
--
minor
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Depends on: 3 bugs, Blocks: 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

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.
Created attachment 616115 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsHTMLEditor::SetInlineProperty
Attachment #616115 - Flags: review?(ehsan)
Created attachment 616116 [details] [diff] [review]
Patch part 2, v1 -- Clean up nsHTMLEditor::SetInlinePropertyOnNode
Attachment #616116 - Flags: review?(ehsan)
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).
Attachment #616117 - Flags: review?(ehsan)
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
Attachment #616118 - Flags: review?(ehsan)

Comment 6

5 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
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.  :-)
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.
Attachment #616521 - Flags: review?(ehsan)
(That's the last patch in the series, BTW.)

Comment 10

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

5 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.
Does WebKit work around them in the test case that Aryeh mentioned?  If it does, can you please remove that work-around? :-)

Updated

5 years ago
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+

Updated

5 years ago
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+
(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

5 years ago
I've contacted Roland Steiner internally since he used to maintain the browser scope tests for editing.

Comment 19

5 years ago
Roland says you should upload a patch there if you can (he has transitioned to work on some other project).

Comment 20

5 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

5 years ago
s/files/tests/ ... :P
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 717789
Blocks: 717789
Duplicate of this bug: 781763
The fix for this bug caused regression bug 828931

Updated

4 years ago
Depends on: 828931

Updated

4 years ago
Depends on: 833889

Updated

4 years ago
Depends on: 824926

Updated

4 years ago
Depends on: 838633

Updated

3 months ago
Depends on: 889940
You need to log in before you can comment on or make changes to this bug.