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)
Tracking
()
RESOLVED
FIXED
mozilla54
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/
Updated•11 years ago
|
Component: Untriaged → Editor
Product: Firefox → Core
Comment 1•11 years ago
|
||
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.
Comment 3•11 years ago
|
||
(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?
Comment 5•11 years ago
|
||
Thanks a lot, Nathan! Alice, any chance you could please look into getting a regression range? Thanks in advance!
Keywords: regression,
regressionwindow-wanted
Comment 6•11 years ago
|
||
#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
Updated•11 years ago
|
(In reply to Alice0775 White from comment #6) > Suspected: Bug 746515 Again ?!? Wow.
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).
Comment 10•11 years ago
|
||
(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.
Comment 12•11 years ago
|
||
(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.
Comment 13•11 years ago
|
||
(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).
Comment 15•11 years ago
|
||
(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="".
Reporter | ||
Comment 18•11 years ago
|
||
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.
Comment 19•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Blocks: editor-blink-compat
Assignee | ||
Updated•7 years ago
|
Assignee: bugs → m_kato
Assignee | ||
Updated•7 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
mozreview-review |
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 23•7 years ago
|
||
mozreview-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+
Comment 24•7 years ago
|
||
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
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8b7597b2fffa https://hg.mozilla.org/mozilla-central/rev/e05399182c38
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 26•7 years ago
|
||
Did you want to uplift this to Aurora for Fx53 or let it ride the trains?
status-firefox51:
--- → wontfix
status-firefox52:
--- → wontfix
status-firefox53:
--- → affected
Flags: needinfo?(m_kato)
Flags: in-testsuite+
Comment 27•7 years ago
|
||
(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.
Comment 28•7 years ago
|
||
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 ;)
Assignee | ||
Comment 29•7 years ago
|
||
(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)
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•