Closed Bug 1353695 Opened 3 years ago Closed 3 years ago

execCommand("defaultparagraphseparator", false, "p"); sometimes leads to <enter> being ignored.

Categories

(Core :: DOM: Editor, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: jorgk-bmo, Assigned: ayg)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #1297414 +++

See attached HTML document.

Instructions:
1) Click into the gap between the two blockquotes.
2) Hit <enter>
3) Hit <backspace>
4) Hit <enter> - Nothing happens.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
More minimal test-case:

data:text/html,<!doctype html>
<div contenteditable><div>x</div><br></div>
<script>
getSelection().collapse(document.body.firstChild, 1);
document.body.firstChild.firstChild.focus();
document.execCommand("insertparagraph");
document.body.textContent = document.body.firstChild.innerHTML;
</script>

It should really be <div>x</div><div><br></div><div><br></div>, but if it were <div>x</div><br><br> it would at least work.  But it's <div>x</div><br>, which is no good at all.
Nice, this was actually already tested by wpt.

Thanks a lot for the quick and well-written bug report, Jorg!
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #1)
> More minimal test-case:
You always have simpler test cases, but mine come from real life ;-)

(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #3)
> Thanks a lot for the quick and well-written bug report, Jorg!
You're welcome, thanks for fixing it quickly. I'll try it out in TB when I've fixed the lasted bustage that came our way from M-C.

Quoting from bug 1334771 comment #73:
I know that {some|most} M-C developers are not friends of Thunderbird. However, the TB team, despite being a pack of cats, have an excellent skill set and we are the secondary M-C testing brigade (bug 1328847 comment #38 "more examples of c-c usefully expanding the test coverage of m-c ;)"), because, to use the analogy, we are the kernel module developers and tread where no one else sets their foot. I could give you so many examples of TB people yelling when something wasn't right in FF.

As you know, TB has 25 million users, and all they ever do all day is hammer the M-C editor ;-)
Aryeh, thanks for the fix, that fixes the issue observed in TB when splitting blockquotes, too. Much appreciated.
Duplicate of this bug: 1353913
This breaks Gmail, so landing a fix quickly would be good.  :)  This also proves that Gmail does use our line-breaking behavior!
Severity: normal → critical
Keywords: regression
Priority: P3 → P1
It seems it breaks some tests?
And there is no reviewer requested.
Sorry, I confused changes in insertparagraph.html.ini with test failures ;)
Comment on attachment 8854971 [details]
Bug 1353695 - Sometimes Enter is ignored in editor;

Hmm, I don't know how review board works, but as Aceman pointed out, apparently no review requested. Sorry for the interference.
Attachment #8854971 - Flags: review?(masayuki)
Comment on attachment 8854971 [details]
Bug 1353695 - Sometimes Enter is ignored in editor;

https://reviewboard.mozilla.org/r/126886/#review130738

::: editor/libeditor/HTMLEditRules.cpp:1575
(Diff revision 1)
> +    NS_ENSURE_TRUE(aSelection.GetRangeAt(0) &&
> +                   aSelection.GetRangeAt(0)->GetStartParent(),
> +                   NS_ERROR_FAILURE);

I'd like you to use:

if (NS_WARN_IF(!aSelection.GetRangeAt(0) &&
               !aSelection.GetRangeAt(0)->GetStartParent())) {
  return NS_ERROR_FAILURE;
}

style here.
Attachment #8854971 - Flags: review?(masayuki) → review+
Blocks: 1354931
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/f25386cf25ad
Sometimes Enter is ignored in editor; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/f25386cf25ad
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.