Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0
Build ID: 20190716211651

Steps to reproduce:

1. Log into your outlook account
2. Click on New message and paste a text that contains embolden text
3. Select any of the embolden text and make it regular by using the keyboard shortcut Ctrl+B
4. Select any of the embolden text and make it regular by clicking on the B option from the edit bar that appears when selecting the text

Actual results:
In steps 3 and 4, the embolden text can't be edited to regular- please see the attached video for more details.

Expected results:
In steps 3 and 4, the embolden text is edited to regular.

This issue is reproducible on Windows 10, Ubuntu 18.04 and Mac Os X when using the latest Nightly 70.0a1, Firefox 69 beta 5 and the latest Firefox release.

I could reproduce this on Ubuntu 18.04 I've also compared it to Chrome on Ubuntu 18.04 and for the latter the behavior is as expected.

I could also reproduce this issue on Yahoo mail.

Problem does not occur in gmail.

Works for bold text, but not for h1 headings. Minimal example:
data:text/html,<h1>h1 heading</h1> <b>bold text</b>

Current suspicion is this problem is React specific.

@miket: can you please have a quick glance on this? I'm investigating the issue in parallel, but am not very familiar with debugging issues of this kind.

The problem can also be reproduced by clicking the "B" button in the editor menu.

It calls u.props.disabled||(u.props.menuProps?u._onMenuClick(e):void 0!==u.props.onClick&&u.props.onClick(e) in https://ow2.res.office365.com/owamail/2019070701.15/scripts/owa.mail.js.

With some intermediate steps, it calls function c(){return"MSIE"===Object(r.a)().browser} in the same file. This at least shows that there's potentially browser specific behavior.

A couple of things I would try:

Toggle the following prefs (probably one at a time?)

dom.keyboardevent.keypress.set_keycode_and_charcode_to_same_value
dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content

I can take a closer look in a bit, will leave ni?

@miketaylr: Thanks. FYI, the issue occurs also when clicking the "B" button, that's why I guess the keyboard prefs won't reveal much.

Sorry for the delay here. In my testing, I can't reproduce the issue for the "bold text" case -- the keyboard shortcuts work as well as the B toolbar menu (and the B from the mini floating menu that pops up). I've tested in OSX and Ubuntu on release and Nightly.

However, I can reproduce the bug for the "h1 heading" case. In Chrome, you can make that unbold, and you can't in Firefox.

Simona, can you re-test the "bold text" case again? It's possible Outlook fixed something on their end.

Having trouble debugging this in Fx Devtools, but it's possible this is an execCommand / selection interop bug somehow. Here's where I ended up

4160: function(e, t, n) {
        "use strict";
        Object.defineProperty(t, "__esModule", {
            value: !0
        }),
        t.default = function(e, t) {
            e.focus();
            var n = function() {
                return e.getDocument().execCommand(t, !1, null)
            }
              , o = e.getSelectionRange();
            o && o.collapsed ? (e.addUndoSnapshot(),
            n()) : e.addUndoSnapshot(n, "Format")
        }
    },

This webpack module:

webpack://owa/./packages/common/controls/packages/controls/owa-editor-ribbonplugin/lib/components/buttons/bold.ts

import { bold as bold_1 } from './bold.locstring.json';
import loc from 'owa-localize';
import RibbonButton, { RibbonButtonState } from './RibbonButton';

import { toggleBold } from 'roosterjs-editor-api';

const bold: RibbonButton = {
    key: 'bold',
    title: () => loc(bold_1),
    imageSrc: require('../svg/bold.svg'),
    shortcut: 'ctrl+B',
    buttonState: formatState =>
        formatState.isBold ? RibbonButtonState.Checked : RibbonButtonState.Normal,
    onExecute: plugin => toggleBold(plugin.getEditor()),
};

export default bold;

ends up as https://ow2.res.office365.com/owamail/2019071502.07/scripts/owa.HtmlEditor.js

6606: function(e, t, n) {
        "use strict";
        var r = n(7526)
          , i = n(4)
          , o = n(3538)
          , a = {
            key: "bold",
            title: function() {
                return Object(i.a)(r.a)
            },
            imageSrc: n(7527),
            shortcut: "ctrl+B",
            buttonState: function(e) {
                return e.isBold ? 1 : 0
            },
            onExecute: function(e) {
                return Object(o.toggleBold)(e.getEditor())
            }
        };
        t.a = a
    },

(In reply to Mike Taylor [:miketaylr] from comment #8)

Sorry for the delay here. In my testing, I can't reproduce the issue for the "bold text" case -- the keyboard shortcuts work as well as the B toolbar menu (and the B from the mini floating menu that pops up). I've tested in OSX and Ubuntu on release and Nightly.

However, I can reproduce the bug for the "h1 heading" case. In Chrome, you can make that unbold, and you can't in Firefox.

Simona, can you re-test the "bold text" case again? It's possible Outlook fixed something on their end.

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0
Build ID: 20190724220024

Mike, here are my testing results - on Windows 10 x64 with the latest Nightly (used the minimal example from Comment 3):

• the "bold text" case - is not reproducible - tested using the keyboards shortcut Ctrl+B and the B button from the Editor menu on: Outlook, Yahoo and Gmail.

• the "h1 heading" case - is reproducible, and not only on Outlook and Yahoo, but also on Gmail. On Gmail - I cannot unbold if I select only parts of the heading, for e.g if I select "heading" to unbold it - the issue is reproducible (used the keyboards shortcut Ctrl+B and the B button from the Editor menu), but if I select "h1 heading" - the issue is not reproducible.

Thanks Simona! Given the fact that you can reproduce in other rich text editors, we can rule out a Yahoo! compat bug. This looks like a core interop thing. I've attached a reduced test case that works as expected in Chrome, but not in Firefox.

Makoto-san, is this a known issue (possible duplicate)?

(In reply to Mike Taylor [:miketaylr] from comment #12)

Makoto-san, is this a known issue (possible duplicate)?

No, but editor don't consider this situation. (cmd_bold doesn't remove/split heading element). But there is a test case in web platform test (<h3>foo[bar]baz</h3> etc).

Another case may be that font-weight property is applied.

The WPT mentioned in comment #13 is https://searchfox.org/mozilla-central/source/testing/web-platform/tests/editing/data/bold.js#698-702 and it currently passes. It can be run with ./mach test testing/web-platform/tests/editing/run/bold.html. This indicates that at least some part of the core-functionality works.

Moreover, as mentioned in comment #10, at Gmail it works for certain cases.

@Makoto: I'm not familiar with "cmd_bold", any idea in which area of the code the root-cause is?

When adapting the test case of comment #11 to bold/unbold the manually selected text, this neither works when selecting parts of the text nor the whole.

As Masayuki pointed out per email,

document.execCommand('stylewithcss', false, true);
document.execCommand('bold', false, null);

works.

For the case when stylewithcss is set to false (which is the default), there's even a test which expects to fail.

Unbolding also doesn't work when partially selecting a <span style="font-weight: bold;">some text</span>. However, it does seem to work when the whole span is selected. Unbolding always seems to work when document.execCommand('stylewithcss', false, true); is called. That can be reproduced with the attached example.

Given the functionality was already broken at least since 2015 (see below) and this issue was reported only in 2019, it doesn't seem to affect many users.
Hence lowering the priority to P2.

The corresponding WPT was already expected to fail in 2015.

The spec perhaps defines the desired behavior for the case of using stylewithcss = false. However, the spec is not simple to parse and it contains:

Warning

This spec is incomplete and it is not expected that it will advance beyond draft status.
[...]

Moreover, the corresponding WPT contain the following remark:

Most of this directory tests conformance to the editing spec written long ago
by Aryeh Gregor.  Nobody actually implements the spec, but the tests are still
useful for regression testing. [...]

Comparing the results of running the WPT (http://w3c-test.org/editing/run/bold.html) on Chrome and Firefox confirms that:
FF (Ubuntu 18.04) :

Found 3012 tests
2882 Pass
130 Fail

Chrome (Ubuntu 18.04):

Found 3012 tests
2530 Pass
482 Fail

Safari:

Found 3012 tests
2175 Pass
837 Fail

The test [["stylewithcss","false"],["bold",""]] "<h3>foo[bar]baz</h3>" compare innerHTML passes on Chrome and Safari. Given the above, FF should exhibit the same behavior.

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Given all the work which is necessary for other bugs I'm currently assigned to, I won't be able to work on this one during the coming weeks. Hence unassigning myself from it.

Probably, I got the buggy point which is currently being cleaned up by bug 1574852. I'll fix this after landing the part.

Reproduced on latest Nightly build 71.0a1 (2019-09-12) on Windows 7 x64.

Reproduced on latest Nightly build 71.0a1 (2019-09-24) in MAC OS X 10_14_6.

Both method take a DOM point with nsCOMPtr<nsINode>* and int32_t*.
This makes each caller complicated. Instead, we should use stack only class
to return both EditorDOMPoint and nsresult. I name it EditResult.

Additionally, this fixes a bug of HTMLeditor::SplitStyleAboveRange(). That
is not tracking new selection start point while it splits elements at end
of given range. This is detected by the debug assertion in
ToRawRangeBoundary() (i.e., this fix is required to pass some tests).

Surprisingly, its aChildOnly is never set to true and if it were set to
true, it does unnecessary recursive calls. Therefore, we can make it
simpler.

For compatibility with Chrome, when removing inline style at block parent,
we should reset the style with creating <span> element whose style
attribute removes the style. We do this only in CSS mode, but we should do
it in HTML mode too.

This patch also makes FontFaceStateCommand::SetState() ignore tt value
if its root caller is Document::ExecCommand(). It was implemented for
composer to handle XUL command in bug 115922. Therefore, we should not do
this special handling on the web. If it were possible to separate this
change to another bug, it'd be nicer. But without this change, we'll have
a lot of regressions of Document.execCommand("fontname"). Therefore,
this is also fixed in this patch.

Note that this removes first .ini file selection because
the tests cannot be run without test number range parameter.
So, the sections are not used anymore.

If selection range is not in one text node, RemoveInlinePropertyInternal()
collects target nodes with SubtreeContentIterator. It only collects topmost
nodes which are entirely contained in the range (it's enough because their
descendants will be handled by RemoveStyleInside() recursively).

The reasons why it uses SubtreeContentIterator rather than
PreContentIterator must be:

1. Performance reason.
2. Assuming there are no multiple text nodes.
3. Not expects that user removes text node styles come from parent block.

The reason 2 is wrong because when removing a style, all browsers don't
join text nodes which was in removing element with adjacent text nodes.
(I.e., we cannot change this behavior for compatibility.)

The reason 3 is of course wrong we're struggling with this scenario.

Therefore, RemoveInlinePropertyInternal() needs to collect partially
selected text nodes by itself (if there are). Then, we can merge the
single text node selected case with the for loop.

Finally, Document.execCommand() still does not work fine if selection
starts from very start of block and/or end at very end of block because
PromoteInlineRange() extends selection range to contain the
containers, then, SubtreeContentIterator won't list up text nodes.

In this case, RemoveInlinePropertyInternal() expects that
RemoveStyleInside() removes text node style with creating
<span> elements. However, RemoveStyleInsilde() only handles
Elements and it handles elements from most-descendants.
Therefore, it cannot distinguish whether text node style comes
from removing inline elements or parent block.

This patch makes RemoveInlinePropertyInternal() collect
descendant text nodes in the range after handling all nodes in
the range except descendant text nodes, then, check the
final style of descendant text nodes, finally, remove the style
if coming from parent block.

The patches will fix a lot of WPT failures.

For making easier to fix regressions, I'll land the patches separately.

https://hg.mozilla.org/mozilla-central/rev/e62a594ec4d2

https://hg.mozilla.org/mozilla-central/rev/6d036df5d1f5

Backed out for build bustages on HTMLStyleEditor.cpp.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=270238610&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=90695432b21eb0add256f0f18a5f1775333a38d7

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=270238610&repo=autoland&lineNumber=29290

Backout: https://hg.mozilla.org/integration/autoland/rev/538fc47f8d3bf21b1ca21b0df4c9f302f76c54dd

Ah, it's used only in debug build...

https://hg.mozilla.org/mozilla-central/rev/a3dd266722b0
https://hg.mozilla.org/mozilla-central/rev/22e16ebb107f

https://hg.mozilla.org/mozilla-central/rev/5188f3280dda

https://hg.mozilla.org/mozilla-central/rev/e7a5c041beab