948411 - execCommand('bold') does not toggle bold state correctly if css styling for font-weight uses numeric values

Status: RESOLVED WONTFIX

(Core :: DOM: Editor, defect)

Description

[C. Martin]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

I have a document that uses a Google font (Roboto) with 300 and 400 weight. The 300 is used as "normal" text, the 400 as bold. So the styling is something like
body {font-weight: 300;}
b, strong {font-weight: 400;}
Now I make that document editable and apply execCommand('bold') to a selection.


Actual results:

Using execCommand('bold') to a selection makes the weirdest things happen.
1) selection gets bold - good
2) apply command again to same selection - nothing happens
3) apply command to a part of the bold text - that part gets bolder!
4) it's impossible to toggle the bold state off
So Firefox sort of looks at the weight styling and not at the applied tag. That's very confusing!


Expected results:

Firefox shouldn't care about the styling of the b or strong tag at all. If I create a b tag from a selection and apply execCommand('bold') to the same selection again the b tag should be removed no matter what. 

If I change the css code to
body {font-weight: normal;}
b, strong {font-weight: bold;}
everything works fine!

Chrome and Internet Explorer 11 do not have the same problem. They simply add or remove the b tag as they should.

Comment 1

[C. Martin]

Here's a text case: http://kunden.cmcm.info/moz-bug/

Comment 2

[C. Martin]

I should add to actual results 3:

If I have:
<b>This is my bold text</b>
then select bold and apply execCommand('bold') again,

I get:
<b>This is my <b>bold</b> text</b>

instead of:
<b>This is my</b> bold <b>text</b>

Again, this only happens when the font weight is set to a numeric number.

Comment 3

[Hallvord R. M. Steen [:hallvors]]

I think this is caused by calls like 
mHTMLCSSUtils->IsCSSEquivalentToHTMLInlineStyleSet

Code here-ish:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLCSSUtils.cpp#1075

Comment 4

[Hallvord R. M. Steen [:hallvors]]

Specifically, this code:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLCSSUtils.cpp#1112

         if (400 < weight) {
           aIsSet = true;
           valueString.AssignLiteral("bold");
         } else {
           aIsSet = false;
           valueString.AssignLiteral("normal");
         }

Means the B/STRONG tag will only be "undone" if the computed weight is more than 400. (If you try to select a snippet of text that is already bolded twice, you'll see the style removed again - the cumulative weight is now above 400). 

I'm not sure who owns this code - recently touched by Ehsan and Aryeh but neither made substantial changes it seems. Aryeh is probably the right person to ask about what the code *should* do.

Comment 5

[(no longer active)]

Our logic here is definitely broken in more than just one way here.  But the real question is what the right thing to do here is.  Perhaps Aryeh has some ideas.  Another question would be whether we can change our behavior here, and the conservative answer would probably be no.

Comment 6

[Hallvord R. M. Steen [:hallvors]]

The right thing to do is probably something along the lines of https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#set-the-selection%27s-value which I believe will fix this bug because it says "This step also removes simple modifiable elements entirely" and B is one such element.

(And I don't see why we couldn't change our behaviour here - other browsers do it right)

Now, naturally there will be more complicated cases - what about 

<b><span>foo|foo|foo</span></b>

with the pipes indicating start/end of selection? Or even 

<b><i><a href="#" style="font-weight:normal">foo|foo|foo</a></i></b>

Maybe the spec already handles it ;)

Comment 7

[(no longer active)]

(In reply to comment #6)
> The right thing to do is probably something along the lines of
> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#set-the-selection%27s-value
> which I believe will fix this bug because it says "This step also removes
> simple modifiable elements entirely" and B is one such element.
> 
> (And I don't see why we couldn't change our behaviour here - other browsers do
> it right)
> 
> Now, naturally there will be more complicated cases - what about 
> 
> <b><span>foo|foo|foo</span></b>
> 
> with the pipes indicating start/end of selection? Or even 
> 
> <b><i><a href="#" style="font-weight:normal">foo|foo|foo</a></i></b>

I think you answered your question.  :-)  All I was saying is that deciding what to do here requires a lot of thought, especially about the edge cases, and about backwards compat concerns, which would be one reason to not change our behavior.

(Just a bit of context here, HTML editing is one area which *all* browser engines have sucked at pretty badly over the years, and all of the current implementations have been done by reverse engineering what early IE versions used to do.  The editing spec you're quoting is one attempt at reconciling the different and sometimes buggy behaviors across browsers but is not actually something that any browser engine implements today.)

Comment 8

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

(In reply to C. Martin from comment #0)
> Steps to reproduce:
> 
> I have a document that uses a Google font (Roboto) with 300 and 400 weight.
> The 300 is used as "normal" text, the 400 as bold. So the styling is
> something like
> body {font-weight: 300;}
> b, strong {font-weight: 400;}
> Now I make that document editable and apply execCommand('bold') to a
> selection.

Sorry, this is just going to break.  Editing is not designed to work with non-default styles.  The web convention is 400 is normal, 700 is bold; if you don't stick to that, be prepared to write your own editing code.  Editing is much too complicated as it stands without attempting to figure stuff like this out.

I don't claim Firefox's behavior here is correct or makes sense, but I don't think this is a valid use-case.

> Firefox shouldn't care about the styling of the b or strong tag at all. If I
> create a b tag from a selection and apply execCommand('bold') to the same
> selection again the b tag should be removed no matter what. 

Editing is WYSIWYG -- the goal of the "bold" command is to get the text to appear bold.  This means we have to look at actual styles, not just the tags in the DOM.  Otherwise the user will highlight some text that looks bold due to <span class="foo"> plus .foo { font-weight: bold }, click "bold" to unbold it, and it doesn't get unbolded.  As far as the user is concerned, the browser is just broken.  The a WYSIWYG editor's job is to deal with the actual styling the user sees, not the DOM that the user doesn't even know exists.

> Chrome and Internet Explorer 11 do not have the same problem. They simply
> add or remove the b tag as they should.

IIRC, IE ignores CSS entirely, which IMO is a large bug.  I didn't look at how Chrome behaves.  IIRC, my spec would force the weight to 700 or 400 with <span style=""> if you've altered the default style of <b>.  Regardless, there's so little interop in editing even in simple cases that what other browsers do doesn't carry a lot of weight.


Sorry, but I recommend WONTFIX.  Both because I don't think the use-case should work as you want it to, and because our editing code is a hornet's nest and changes like this are liable to break all kinds of stuff.

Comment 9

[(no longer active)]

Agreed.