1022904 - Firefox doen't respect styleWithCSS=false for the fontSize command on contenteditable

Status: NEW

(Core :: DOM: Editor, defect)

Description

[Ionut Colceriu]

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2031.2 Safari/537.36

Steps to reproduce:

With text selected in an `contenteditable` node, run:

document.execCommand('styleWithCSS', false, true);    document.execCommand('fontSize', false, 7);

Here's a quick demo:
http://jsfiddle.net/ghinda/stbNL/9/


Actual results:

Firefox inserts a font node.


Expected results:

It should insert a span node, with a font-size inline style.

Comment 1

[Paul Silaghi, QA [:pauly]]

I'm not so sure this is a bug

Comment 2

[Boris Zbarsky [:bzbarsky]]

We're clearly ignoring this flag for the fontSize command.  Presumably because the actual interaction there is pretty messed up (e.g. even with styleWithCSS=true sometimes fontSize uses a <font>) and was basically created wholesale as part of the so far very draft editing spec.

We'd probaly take a patch to fix this, but until the spec stabilizes this is low priority.

Comment 3

[Tom Geompse]

This is a regression (Bug #763859)
Now all our users are complaining again :(

Using mozregression I found that it occured on nightly version 21 (???)
(BTW C:\mozilla-build\start-l10n.bat is now C:\mozilla-build\start-shell-l10n.bat <- http://mozilla.github.io/mozregression/).
 4:21.17 LOG: MainThread Bisector INFO Last good revision: 20bbf73921f4
 4:21.19 LOG: MainThread Bisector INFO First bad revision: fcf79680a057
 4:21.19 LOG: MainThread Bisector INFO Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=20bbf73921f4&tochange=fcf79680a057

For this test case : https://doc.netsoins.org/firefox-fontsize.php
> "Good" for nightly 2013-02-07
> "Bad" for nightly 2013-02-08

Comment 4

[Alice0775 White]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9793bcb3b7e0&tochange=4b47185f48f8

Regressed by:
41a3c03feff3	Jet Villegas — Bug 812638: revert fix for bug 480647 (part 6) that introduced this regression. r=ehsan

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This is really a dupe of bug 480647, which seems to be closed WONTFIX, although it doesn't say exactly why and I don't remember.  Firefox's historical behavior AFAIK has always been to completely ignore CSS for the fontSize command; I tried to improve it in that bug but it broke stuff so it was reverted.  The major problem here is that queryCommandValue("fontSize") returns a value from 1 to 7, which obviously does not support all the values that CSS supports.  You could still support execCommand("fontSize") somewhat reasonably, but it would require a lot of special-case code to deal with CSS sizes that don't map exactly to <font size=>.  Especially because the actual value of <font size=X> is not any fixed number of points/pixels, IIRC.  Also note that <font size=7> does not correspond to any CSS value (!).

WebKit actually does deal with CSS for this command somewhat sanely, impressively.

(In reply to Boris Zbarsky [:bz] from comment #2)
> We're clearly ignoring this flag for the fontSize command.  Presumably
> because the actual interaction there is pretty messed up (e.g. even with
> styleWithCSS=true sometimes fontSize uses a <font>) and was basically
> created wholesale as part of the so far very draft editing spec.

Actually, this command came pretty much straight from reverse-engineering WebKit's implementation, because it had the sanest way of doing it (although it's still a hopeless mess in practice).  See comments at the side of the spec.

(In reply to Tom Geompse from comment #3)
> This is a regression (Bug #763859)
> Now all our users are complaining again :(

It's a regression caused by backing out a fix that caused a regression.  :)  Basically we never supported this feature correctly.  For a few releases (i.e., less than a year) we had broken support that got backed out.  I wouldn't classify this as a regression, it's been broken for practically Firefox's entire history.

Comment 6

[Tom Geompse]

(In reply to :Aryeh Gregor from comment #5)
> I wouldn't classify this as a regression, it's been broken for practically
> Firefox's entire history.

Meh. Our users reported this as broken mid-december, while no change occured on our side.
I definitely solved this issue by implementing an HTML code cleaner on the client side wich amongts other things replaces <font size> by <span style>, but this is painful Javascript instead of beautiful HTML5 ;)

Comment 7

[Jet Villegas (inactive)]

(In reply to :Aryeh Gregor from comment #5)
> This is really a dupe of bug 480647

Agreed. We won't be backing out the fix for bug 812638 for the reasons Aryeh already explained in comment 5.

Comment 8

[cyril.auburtin]

Hi guys, indeed fontSize doesn't work with styleWithCSS = true (title of the issue should be corrected, false->true) quick example http://jsfiddle.net/crl/aLp9ueyf/ (or more complex http://jsfiddle.net/crl/aLp9ueyf/1)

Comment 9

[cyril.auburtin]

(tested on Firefox 43.0.4)

Comment 10

[cyril.auburtin]

I don't understand why it fails on fontSize like that, while working well on fontName http://jsfiddle.net/crl/aLp9ueyf/2/, foreColor, backColor, justifyRight/Left/Full, ...

that's the only bug I've found on Firefox so far, except that it's by far superior to the chrome and edge

Comment 11

[Tom Geompse]

Hi Cyril, thanks for your comments. There is a lot more bugs in Firefox, especially related to contentEditable=true. Keep digging !

Comment 12

[cyril.auburtin]

Thanks Tom, yea I know this API is old, I hope it's not abandoned though because I plan to make a wysiwyg based a lot on that. document.execCommand does a lot of merging/splitting magic that would take some time to code myself (like some other wysiwyg do) not to mention it allows the native undo/redo to work.

I have made all the code for tables (add rows, merge, etc..) based on insertHTML, I'm also doing some fun hack to replace <b> with <strong> on the fly (and <em>, ...)

The only problem is fontSize which is already bad and inflexible with <span style="font-size:small" and even worse with <font size="2", so I hope really much that there will be some efforts to fix it on Firefox :), I'm ready to help, I guess it's not easy though (probably some c++ code).

I can also try a similar hack for fontSize, by using another cmd and replacing styles, since anyway I will need to convert small, x-small, large, x-large, ... to pixels

Other bad news is Edge doesn't support styleWithCSS https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/11369661-support-document-execcommand-stylewithcss-null I know it's a concurrent, but it's much better than IE, so happy if one of you can upvote that feature request

might put the wysiwyg on github too, if it works

Comment 13

[Tom Geompse]

Well, its support is so random and buggy that all big players simply dropped it and do the wysiwyg magic from scratch.
Start with a DIV tag and watch user events, there you will do something neat.

Comment 14

[cyril.auburtin]

yes, a bit sad because it's really close to work well with execCommand, just need that fontSizing.

Why did Firefox put efforts in those optional things for object resizing and table edits if some more basic feature like fontSize isn't working. Do you know where I have to look into for that part of code? SpiderMonkey I guess?

Comment 15

[cyril.auburtin]

Else, thanks, yea with MutationObserver and some good code for applying tags and styles it may be good, quite harder probably