Closed Bug 833889 Opened 11 years ago Closed 7 years ago

execCommand('bold') does not toggle bold state correctly when text is already bold

Categories

(Core :: DOM: Editor, defect, P3)

15 Branch
x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: nspady, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 Safari/537.17

Steps to reproduce:

I was attempting to bold and unbold a user editable title using the execCommand('bold', false, null) command. With the "styleWithCSS" enabled.


Actual results:

The text became bolded, then unbolded. But when attempting to bold the text again, it failed to re-bold the text. I had to select the title in two parts to rebold it again.


Expected results:

The text should be togglable in the bold state and it should not create a nested hierarchy when bolding pieces of the title and then bolding/unbolding the whole line.

This appears to only be an issue when the font-weight is already set to bold in the CSS. If it is set to normal, then it toggles the bold state as expected. Please see the included JSFiddle for an example.

http://jsfiddle.net/umwXE/7/
Component: Untriaged → Editor
Product: Firefox → Core
Is this a regression?
It is not a regression per se since it came up as an issue for Firefox users of a title editing system.
(In reply to comment #2)
> It is not a regression per se since it came up as an issue for Firefox users of
> a title editing system.

To make myself clearer, I meant does this bug also occur in older versions of Firefox?
Yes, the first occurrence was in version 14 I believe.
Thanks a lot, Nathan!

Alice, any chance you could please look into getting a regression range?  Thanks in advance!
#1 It works as expected in Firefox14 and earlier  if I selected whole sentence.
#2 However, it does not work in all version if I selected word partially.

Regression window(m-c) for #1
Good:
http://hg.mozilla.org/mozilla-central/rev/424cb3a6141b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120424 Firefox/15.0a1 ID:20120424095512
Bad:
http://hg.mozilla.org/mozilla-central/rev/f661fb83699b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120424 Firefox/15.0a1 ID:20120424181127
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=424cb3a6141b&tochange=f661fb83699b


Regression window(birch)
Good:
http://hg.mozilla.org/projects/birch/rev/98beb4e935d7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120419 Firefox/14.0a1 ID:20120419040227
Bad:
http://hg.mozilla.org/projects/birch/rev/568ff98bad40
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120420 Firefox/14.0a1 ID:20120420040227

Suspected: Bug 746515
Blocks: 746515
OS: Mac OS X → All
Version: 18 Branch → 15 Branch
(In reply to Alice0775 White from comment #6)

> Suspected: Bug 746515

Again ?!? Wow.
Assigning to Jet in order to find an owner.
Assignee: nobody → bugs
Ok, this bug is the _perfect_ example why Aryeh's changes in bug 746515
should never have happened. That change does not aggregate style changes
into one single style attribute on same element but nests changes. In other
words, if you bolden a word and make italic afterwards, you end up with TWO
nested spans around the word, one having 'font-weight: bold' and the other
having 'font-style: italic'.

That explained, our current case is even more painful: we try to unbolden
a h2 that is made bold by a CSS rule. The only way to do that is to add
a 'font-style: normal' inline style (we could also tweak the CSS rules but
the editor is not that smart). Once this is done, if we try to bolden back
the h2, we just put encapsulate the span we just created into a bold span.
Result is the following one

  <h2>
    <span style="font-weight: bold">
      <span style="font-weight: normal">
      </span>
    </span>
  </h2>

Do you see the crazyness of this, only because of bug 746515?
This is an insane situation. At this point, I recommend backing out
the fix for 746515 and all the subsequent regression fixes or we'll
discover regressions of that depth for months. And I am super-serious
here.

If I have a conclusion to draw from this, here it is: the HTML editing
rules of the editor are extremely complex. They were _extremely_ precisely
implemented carefully listening to the requirements of Composer and
Thunderbird. The fact we want to harmonize editing rules between Gecko
and the other rendering engines is NOT a good reason enough in my opinion
to massacre years of work. We can improve the code, sure. But carefully.
Pinging me, the only survivor of the original editor team still working
on Gecko, could have helped but I was not even cc:ed on the bug (that
won't happen again, I'm observing all editor bugs now).
(In reply to Daniel Glazman (:glazou) from comment #9)
> Ok, this bug is the _perfect_ example why Aryeh's changes in bug 746515
> should never have happened. That change does not aggregate style changes
> into one single style attribute on same element but nests changes. In other
> words, if you bolden a word and make italic afterwards, you end up with TWO
> nested spans around the word, one having 'font-weight: bold' and the other
> having 'font-style: italic'.
> 
> That explained, our current case is even more painful: we try to unbolden
> a h2 that is made bold by a CSS rule. The only way to do that is to add
> a 'font-style: normal' inline style (we could also tweak the CSS rules but
> the editor is not that smart). Once this is done, if we try to bolden back
> the h2, we just put encapsulate the span we just created into a bold span.
> Result is the following one
> 
>   <h2>
>     <span style="font-weight: bold">
>       <span style="font-weight: normal">
>       </span>
>     </span>
>   </h2>
> 
> Do you see the crazyness of this, only because of bug 746515?
> This is an insane situation. At this point, I recommend backing out
> the fix for 746515 and all the subsequent regression fixes or we'll
> discover regressions of that depth for months. And I am super-serious
> here.

I would take a patch which reverts parts 4 and 5 of that bug and also makes sure tests pass etc., until a better patch gets written.  I'm not sure what subsequent regression fixes you're talking about though.

> If I have a conclusion to draw from this, here it is: the HTML editing
> rules of the editor are extremely complex.

Absolutely!

> They were _extremely_ precisely
> implemented carefully listening to the requirements of Composer and
> Thunderbird. The fact we want to harmonize editing rules between Gecko
> and the other rendering engines is NOT a good reason enough in my opinion
> to massacre years of work.

Daniel, nobody is massacring years of work.  This bug affects Firefox and the downstream editor users in precisely the same way.  Harmonizing HTML editing between different engines is in fact quite valuable, I'm not sure why you're saying this.

> We can improve the code, sure. But carefully.
> Pinging me, the only survivor of the original editor team still working
> on Gecko, could have helped but I was not even cc:ed on the bug (that
> won't happen again, I'm observing all editor bugs now).

I would have definitely CCed you if I knew that you're actively working on the editor, but I haven't seen many patches from you since I started to work on the editor at least.  But it's great that you're watching the component now.  I definitely appreciate you keeping an eye on things.  :-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to :Ehsan Akhgari from comment #10)

> I would take a patch which reverts parts 4 and 5 of that bug and also makes
> sure tests pass etc., until a better patch gets written.

Fine, I think it's really the best solution at this point.

> I'm not sure what
> subsequent regression fixes you're talking about though.

832025 for instance. Probably 824926. Certainly more.

> Daniel, nobody is massacring years of work.  This bug affects Firefox and

I disagree. The change in 746515 was implemented only on the basis of
harmonization between WebKit and Gecko. This was seen as a more important
gaol than our resulting markup quality. Since I discovered bug 832025,
I compared quite extensively WebKit and Gecko. It _clearly_ appears our
HTML editing rules are superior - and sometimes VERY superior - to WebKit's.
So sorry, but seen from here, I can only call downgrading the quality of the
markup our editor outputs only "to make like WebKit" a massacre.

> the downstream editor users in precisely the same way.  Harmonizing HTML
> editing between different engines is in fact quite valuable, I'm not sure
> why you're saying this.

It's valuable if the harmonization is not harmonization for the only sake of
harmonization. Harmonization does not mean "let's all make the worst markup
we can find". Nesting spans carrying each a style attribute _is_ the worst
markup we can find; Aryeh had no valid justification for that change when
we discussed it on IRC, beyond "that's harmonization".
If 2 browsers' common practice is to make ugly code for
a given editing and Gecko does clean code, I don't see why we should
accept a change; we should instead push to make the others accept the
cleaner solution. That's called Standardization and its ultimate only
goal is quality for the user.

> I would have definitely CCed you if I knew that you're actively working on
> the editor, but I haven't seen many patches from you since I started to work
> on the editor at least.  But it's great that you're watching the component
> now.  I definitely appreciate you keeping an eye on things.  :-)

Ehsan, you seem to take my comment personnally. Don't. You have dived into
editor's code super-quickly, and alone. Wow, seriously. Your review could
probably not have detected this. Mine could, that's all I'm saying.

The changes implied by the current Editing specification
need to be super-carefully reviewed before accepting them into our code. Some are
terribly counter-productive and should be rejected by Mozilla as such.

A change like 746515 can break many inline wysiwyg editors, drastically
impact my own products, now successful parts of the Mozilla ecosystem,
and decrease the quality of the richtext emails output by Thunderbird
at a time Outlook moves to rendering engines of better quality producing
a better markup.
(In reply to Daniel Glazman (:glazou) from comment #11)
> (In reply to :Ehsan Akhgari from comment #10)
> 
> > I would take a patch which reverts parts 4 and 5 of that bug and also makes
> > sure tests pass etc., until a better patch gets written.
> 
> Fine, I think it's really the best solution at this point.
> 
> > I'm not sure what
> > subsequent regression fixes you're talking about though.
> 
> 832025 for instance. Probably 824926. Certainly more.

Oh, right, bad dependency on bug 832025.  Bug 824926 is a regression from bug 590640 AFAIK.

> > Daniel, nobody is massacring years of work.  This bug affects Firefox and
> 
> I disagree. The change in 746515 was implemented only on the basis of
> harmonization between WebKit and Gecko. This was seen as a more important
> gaol than our resulting markup quality. Since I discovered bug 832025,
> I compared quite extensively WebKit and Gecko. It _clearly_ appears our
> HTML editing rules are superior - and sometimes VERY superior - to WebKit's.
> So sorry, but seen from here, I can only call downgrading the quality of the
> markup our editor outputs only "to make like WebKit" a massacre.

Nobody has the goal of making Gecko editing rules like WebKit.

> > the downstream editor users in precisely the same way.  Harmonizing HTML
> > editing between different engines is in fact quite valuable, I'm not sure
> > why you're saying this.
> 
> It's valuable if the harmonization is not harmonization for the only sake of
> harmonization. Harmonization does not mean "let's all make the worst markup
> we can find". Nesting spans carrying each a style attribute _is_ the worst
> markup we can find; Aryeh had no valid justification for that change when
> we discussed it on IRC, beyond "that's harmonization".
> If 2 browsers' common practice is to make ugly code for
> a given editing and Gecko does clean code, I don't see why we should
> accept a change; we should instead push to make the others accept the
> cleaner solution. That's called Standardization and its ultimate only
> goal is quality for the user.

Daniel, you're reading motives that do not exist.  Please, let's keep the conversation not about pointing fingers.

> > I would have definitely CCed you if I knew that you're actively working on
> > the editor, but I haven't seen many patches from you since I started to work
> > on the editor at least.  But it's great that you're watching the component
> > now.  I definitely appreciate you keeping an eye on things.  :-)
> 
> Ehsan, you seem to take my comment personnally. Don't.

Believe it or not, no.  :-)  I just don't want you to read incorrect intentions into the work that has been done in the editor component, that's all.

> You have dived into
> editor's code super-quickly, and alone. Wow, seriously. Your review could
> probably not have detected this. Mine could, that's all I'm saying.

Sure, I'm not debating that!  I just didn't know that you were available for code reviews.

> The changes implied by the current Editing specification
> need to be super-carefully reviewed before accepting them into our code.
> Some are
> terribly counter-productive and should be rejected by Mozilla as such.

Nothing that the spec says is set into stone, and we can definitely change it where it makes sense.  Please bring up the issues that you see.  (Unfortunately as I understand things, Aryeh is busy with his education at this time, so I'm not sure if he'll be available to address the spec issues, but it's best to have them documented on the list rather than nowhere.

> A change like 746515 can break many inline wysiwyg editors, drastically
> impact my own products, now successful parts of the Mozilla ecosystem,
> and decrease the quality of the richtext emails output by Thunderbird
> at a time Outlook moves to rendering engines of better quality producing
> a better markup.

I understand all of that, but nobody had the intention of doing the above!  I actually think that the markup badness is mostly caused by the fact that our implementation has been buggy.  It could also be the fact that there is a spec bug that needs to get fixed before this can be adequately addressed in an implementation.
(In reply to Daniel Glazman (:glazou) from comment #9)
> That explained, our current case is even more painful: we try to unbolden
> a h2 that is made bold by a CSS rule. The only way to do that is to add
> a 'font-style: normal' inline style (we could also tweak the CSS rules but
> the editor is not that smart). Once this is done, if we try to bolden back
> the h2, we just put encapsulate the span we just created into a bold span.
> Result is the following one
> 
>   <h2>
>     <span style="font-weight: bold">
>       <span style="font-weight: normal">
>       </span>
>     </span>
>   </h2>

The problem here is that adding bold should remove any font-weight property that's set on descendants.  If we don't, it probably causes other bugs elsewhere, and that should be fixed in itself.  It looks like bug 746515 made a number of other underlying bugs arise in cases where they previously didn't -- if we had more editor resources, fixing those bugs themselves would be a better solution than returning to the status quo ante.

> Do you see the crazyness of this, only because of bug 746515?
> This is an insane situation. At this point, I recommend backing out
> the fix for 746515 and all the subsequent regression fixes or we'll
> discover regressions of that depth for months. And I am super-serious
> here.

Given that we don't have anyone putting much time into maintaining the editor, I agree that backing out the relevant changes may well be the best option at this point.  I still think the change is in principle a good one, but given the state of editing code (= disastrous), any nontrivial change is likely to introduce regressions, so we have to avoid them.

> The fact we want to harmonize editing rules between Gecko
> and the other rendering engines is NOT a good reason enough in my opinion
> to massacre years of work.

FWIW, the motivation for the change was not to harmonize behavior with any other engine -- in fact, as I explain in bug 746515 comment 1, the spec's behavior does not match any browser.  The motivation was to harmonize behavior between CSS and non-CSS modes.  (Which unfortunately is putting a band-aid on a severed limb in Gecko's case, because they follow totally different codepaths in many cases.)
(In reply to :Aryeh Gregor from comment #13)

> The problem here is that adding bold should remove any font-weight property
> that's set on descendants.  If we don't, it probably causes other bugs

The editor does it, I coded it myself 12 years ago.

> Given that we don't have anyone putting much time into maintaining the
> editor, I agree that backing out the relevant changes may well be the best
> option at this point.  I still think the change is in principle a good one,
> but given the state of editing code (= disastrous), any nontrivial change is
> likely to introduce regressions, so we have to avoid them.

The editing code is in bad shape because Mozilla has let it fall w/o
paying someone to keep it afloat. It's not "disastrous" and nontrivial
changes are perfectly doable.

> FWIW, the motivation for the change was not to harmonize behavior with any
> other engine -- in fact, as I explain in bug 746515 comment 1, the spec's
> behavior does not match any browser.  The motivation was to harmonize
> behavior between CSS and non-CSS modes.  (Which unfortunately is putting a
> band-aid on a severed limb in Gecko's case, because they follow totally
> different codepaths in many cases.)

Of course they do, because the CSS-based editing and the HTML-based editing
are by nature very different. You *can* aggregate styles into one single @style
attribute with CSS and you *can't* collapse several html elements into one.
Making different codes for CSS-editing and HTML-editing was then a mature
decision, taken after long discussions between myself, Beth Epperson (manager
of the editor team at that time), Kin Blas (editor team, author of the
Transaction Manager) and Joe Francis (main author of the editing rules).
(In reply to Daniel Glazman (:glazou) from comment #14)
> The editor does it, I coded it myself 12 years ago.

That doesn't mean it doesn't have bugs that prevent it from working in situations where it was intended to.

> The editing code is in bad shape because Mozilla has let it fall w/o
> paying someone to keep it afloat.

Mozilla paid me for around a year to work primarily on editing, so it seems they're willing to provide funding.

> It's not "disastrous" and nontrivial
> changes are perfectly doable.

That does not match my experience.

> Of course they do, because the CSS-based editing and the HTML-based editing
> are by nature very different.

Not from the end-user's point of view.  Clicking the "B" button should make the selected text bold.  If it makes different text bold depending on whether the boldness is implemented in HTML, CSS, ODF, or whatever, that's a bug.  HTML vs. CSS should be an implementation detail.
(In reply to :Aryeh Gregor from comment #15)

> > The editor does it, I coded it myself 12 years ago.
> 
> That doesn't mean it doesn't have bugs that prevent it from working in
> situations where it was intended to.

"intended to"? When? By who? Your HTML Editing spec is recent, still an
WIP and there were absolutely no spec for what IE or other browsers were
doing in that field. Gecko was the first browser to deal with a CSS mode.

> > The editing code is in bad shape because Mozilla has let it fall w/o
> > paying someone to keep it afloat.
> 
> Mozilla paid me for around a year to work primarily on editing, so it seems
> they're willing to provide funding.

One year between june 2003 and now, if you except the work peterv did on
contenteditable.

> > It's not "disastrous" and nontrivial
> > changes are perfectly doable.
> 
> That does not match my experience.

Ok, let's discuss the word "disastrous" then.
My current experience is that the editor currently has *major* regressions that
Mozilla users do suffer from (Cf. Dom Foulsham's email) because changes to the
editor to make it match an unstabilized spec were implemented.
That spec is even mentioned in its header as WIP...

  https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html

These changes have drastic impacts on all mozilla-based editors, inline or
standalone. These changes break webapps, these changes plague Thunderbird and
BlueGriffon.

> > Of course they do, because the CSS-based editing and the HTML-based editing
> > are by nature very different.
> 
> Not from the end-user's point of view.  Clicking the "B" button should make
> the selected text bold.  If it makes different text bold depending on
> whether the boldness is implemented in HTML, CSS, ODF, or whatever, that's a
> bug.  HTML vs. CSS should be an implementation detail.

The *visual* behaviour of the the CSS mode and the HTML mode were
exactly the same modulo two main exceptions (background colors and font sizes
that have no strict equivalences) and one or two very minor ones.

It's perfectly normal to replace a <b><i>foo</b></i> by a 
<span style="font-weight: bold; font-style: italic">foo</span>
and not two nested spans. I'm sorry if you can't see the correctness of that
but if other browsers do implement nested spans, _they_ are failing, not
Gecko. In a stylesheet, an equivalent issue would be to generate
.foo{font-weight:bold} .foo{font-style:italic} instead of
.foo{ font-weight: bold; font-style: italic}. Both will give the same
visual result but one is what users expect in terms of quality, the other
is not. That's that simple.


All that said, the editor needs *urgent* fixes. I am unfortunately chairing
a CSS WG meeting right now and later traveling to NYC to speak at a W3C
Workshop. Who's going to work on these urgent fixes?
(In reply to Daniel Glazman (:glazou) from comment #16)
> It's perfectly normal to replace a <b><i>foo</b></i> by a 
> <span style="font-weight: bold; font-style: italic">foo</span>
> and not two nested spans.

What contenteditable use cases benefit from <span style="font-weight: bold;"> compared to <b>? If I was writing a CMS, I'd sure prefer contenteditable execCommand("bold") to generate <b> instead of style="".
One use case is if the text is already set to be bold, then the only way to toggle the bold state is to add <span style="font-weight: normal;"> to unbold the text.
Any solution for this? this is still happening. To me, this is kind of a buggy behavior. If for instance, you have an heading element with a font-weight of bold ( > than 400 ) and you try to execCommand('bold'..) on it, it won't do anything.. now if font-weight is below 400 it will. It just doesn't recognize that the font is already bold and you can't un-bold it. Even if the heading element has no font-weight, because by default, the h1 and h2 is already bold. So it doesn't work.
Assignee: bugs → m_kato
Priority: -- → P3
Comment on attachment 8839811 [details]
Bug 833889 - Part 1. Remove CSS properties even if it is default value.

https://reviewboard.mozilla.org/r/114380/#review115834

::: editor/libeditor/CSSEditUtils.h:216
(Diff revision 1)
>     *                            if irrelevant.
>     * @param aValueString   [IN/OUT] The attribute value (in) the list of CSS
>     *                                values (out).
>     * @param aStyleType     [IN] eSpecified or eComputed.
>     * @return               A boolean being true if the css properties are
> -   *                       set.
> +   *                       not *default*.

Perhaps, you meant "same as initial value".

::: editor/libeditor/CSSEditUtils.h:245
(Diff revision 1)
>                                             StyleType aStyleType);
>  
>    /**
> +   * This is a kind of IsCSSEquivalentToHTMLInlineStyleSet.
> +   * IsCSSEquivalentToHTMLInlineStyleSet returns whether the properties
> +   * aren't default.  But this method returns whether the properties aren't

So, please update this "default" too.

::: editor/libeditor/CSSEditUtils.h:253
(Diff revision 1)
> +   *  - IsCSSEquivalentToHTMLInlineStyleSet returns false.
> +   *  - HaveCSSEquivalentStyles returns true.
> +   *
> +   * @param aNode          [IN] A DOM node.
> +   * @param aHTMLProperty  [IN] An atom containing an HTML property.
> +   * @param aAttribute     [IN] A atom to an attribute name or nullptr

nit: An atom
Attachment #8839811 - Flags: review?(masayuki) → review+
Comment on attachment 8839812 [details]
Bug 833889 - Part 2. Update test result of web platform tests.

https://reviewboard.mozilla.org/r/114382/#review115840

Nice!
Attachment #8839812 - Flags: review?(masayuki) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7597b2fffa
Part 1. Remove CSS properties even if it is default value. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05399182c38
Part 2. Update test result of web platform tests. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/8b7597b2fffa
https://hg.mozilla.org/mozilla-central/rev/e05399182c38
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Did you want to uplift this to Aurora for Fx53 or let it ride the trains?
Flags: needinfo?(m_kato)
Flags: in-testsuite+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> Did you want to uplift this to Aurora for Fx53 or let it ride the trains?

So this won't be solved until version 53 is released? right now we have the 51. Do you know how much time it would take to make it up to 53? Thanks.
Right now, the fix won't ship until Firefox 54, which is due to ship in mid-June. I'm asking about whether we want to backport the fix to a release shipping sooner than that. But 52 is pushing it because it's already late in the development cycle (we're building the release candidate next Monday), so there's going to be very little time to find any possible regressions caused by this patch before it goes out to the full release audience. Given that this was broken for nearly 5 years, I'm not sure it's worth risking unknown regressions just to get it out a couple months earlier. But that's why I'm asking ;)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> Did you want to uplift this to Aurora for Fx53 or let it ride the trains?

Humm, risk isn't low and this is long aged bug.  So I don't think it is good to uplift this.
Flags: needinfo?(m_kato)
You need to log in before you can comment on or make changes to this bug.