1779846 - Spell check dialog pops up and disappears instantly when text selection includes the end of the last word before line break or paragraph break

Status: RESOLVED FIXED

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

Description

[bryanwong]

Attachment: Screen Shot 2022-07-15 at 2.06.24 PM.png

Steps to reproduce:

1. On MacOS (12.4) with Thunderbird version 102.02, select "Write" to compose a new message
2. Enter some text in the message area of the email composition window.
3. Highlight and select some text
4. Select "Spelling" at the top of the message composition window

Actual results:

The check spelling window pops up for a brief second, then disappears right away.

Expected results:

The check spelling window should pop up and remain until closed.

Comment 1

[Thomas D. (:thomas8)]

Thanks bryanwong! Confirming (I came here after seeing this independently when smoketesting TB 105.0b1).

STR

• Enter some text in compose window (affected versions below, all except TB 91)
• Select some text which must include the end of the last word before a line break or paragraph break
• Click Spelling button from toolbar

Actual

• Spell check dialog just shortly flashes up and then disappears

Expected

• show spell check dialog for selection (adjusted to full word(s))
• correct behaviour seen in TB 91

This is a regression - Alice, can you help?

• Works in TB 91.13.0 (32-bit), Win10.
• Fails in all other channels:
  • Release 102.2.0 (64-bit)
  • Beta 105.0b1 (64-bit)
  • Daily 106.0a1 (2022-08-24) (64-bit)

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=b87bf19809e3c800457dfc5b342c89b67816f21f&tochange=c894a3a6a723c4a2a18f4cb3c75808f129d475af
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b50ef8e31c4c2dd8993366c68c2f6f87e5a4f68a&tochange=11d3a5026dea4bc941ff67a93465b6b96e72abf7

Comment 3

[Thomas D. (:thomas8)]

Thank you Alice. Hmmm, at first sight nothing stands out from the regression range...

Comment 4

[Thomas D. (:thomas8)]

(In reply to Thomas D. (:thomas8) from comment #3)

Thank you Alice. Hmmm, at first sight nothing stands out from the regression range...

Maybe bug 1730084 regressed this?

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Sorry for the regression. I'll take a look.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #5)

Sorry for the regression. I'll take a look.

Thank you very much Ting-Yu Lin for following up on my needinfo from Bug 1730084 Comment 11, that's much appreciated!

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1779846 - Remove an unneeded if-statement in TextServicesDocument::OffsetEntryArray::FindWordRange().

This patch fixed a regression caused by Bug 1730084 Part 4
https://phabricator.services.mozilla.com/D125434, which is intended to preserve
the behavior of WordBreaker::FindWord().

Before Bug 1730084 Part 4, WordBreaker::FindWord() returns
[aTextLen + 1, aTextLen + 1] only when aOffset > aTextLen.
But WordBreaker::FindWord() had an assertion aOffset <= aTextLen
to guarantee the caller never passes aOffset > aTextLen. Thus, we can never
go into the if (res.mBegin > strLen) in
TextServicesDocument::OffsetEntryArray::FindWordRange().

However, Bug 1730084 Part 4 wrongly changes the if-statement to
if (range.mEnd == range.mBegin), and makes it reachable when
TextServicesDocument::OffsetEntryArray::FindWordRange() passes
aPos == aLen into WordBreaker::FindWord(). This makes InitSpellChecker()
failed when enableSelectionChecking=true.

This patch deletes the originally unreachable error handling if-statement, and
adds an assertion to guarantee strOffset and strLen is valid.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8644e2025dbd
Remove an unneeded if-statement in TextServicesDocument::OffsetEntryArray::FindWordRange(). r=masayuki

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8644e2025dbd

Comment 11

[Wayne Mery (:wsmwk)]

This is needed for Thunderbird 102esr. Can the patch please be uplifted to esr to fix the regression? Thanks.

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 9292698 [details]
Bug 1779846 - Remove an unneeded if-statement in TextServicesDocument::OffsetEntryArray::FindWordRange().

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch fixed the spellchecking when selecting some text. An important usecase when composing email in Thunderbird. See comment 0 for the steps to reproduce the bug.
• User impact if declined: The spellchecking for selected text is broken if declined.
• Fix Landed on Version: 106
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is small, affecting only the initialization of spellchecking in editor.

Comment 13

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/4ecbf3c69ea7

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9292698 [details]
Bug 1779846 - Remove an unneeded if-statement in TextServicesDocument::OffsetEntryArray::FindWordRange().

Approved for 102.5esr.