1562677 - When searching for text when reading an email, the background of the text input does not turn red if not found

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Patrick Cloke [:clokep]]

This is using Thunderbird 68 beta 3 (build 3) on macOS.

When reading an email and searching for text that is not in the email I expect the background of the textbox to go "red", but it stays white. This is a regression, but not sure when it regressed.

STR:

• Open an email for reading.
• Search for text that doesn't exist.

Expected:

• The text input of the find bar goes red.

Actual:

• The text input of the find bar stays white.

Comment 1

[Benjamin Flanagin]

Confirmed, on beta 3 on Linux.

Comment 2

[Richard Marti (:Paenglab)]

I see this too on Windows. Jörg, maybe you remember that I chatted with you about the "undefined" in the search box when I open the search and the wrong search doesn't change to red? This is still the case. Unfortunately I forgot to file a bug then because I almost never use this search. :-(

Comment 3

[Jorg K (CEST = GMT+2)]

I suspect this comes from the de-XBL of the status bar, so bug 1491660.

Khushil, could you please take a look. Alice, can you find or confirm the regression for us.
STR: View an e-mail in the preview, Ctrl+F to open the Find box in the status bar, type something that is NOT found. The background of the search box should go red.

Comment 4

[Alice0775 White]

Umm, intermittently?, strange...
It needs to restart(step5 and 6) to reproduce the issue in the bad c-c build.
Without restart, the problem does not occur.

Steps to reproduce for finding regression window:

1. Start TB with new profile
2. When "Set Up an Existing Email Account" pops up, create email account
3. When "System Integration" dialog pops up, click on "Skip integration"
4. When "Lightning Added" door hanger pops up, click on "Enable"
5. Exit TB (do not try Ctrl+F now)
6. Start TB again , When "System Integration" dialog pops up, click on "Skip integration"
7. Select Inbox folder
8. Select a email in thread pane
9. Ctrl+F
10. Type jhxjhsajkfghjfgjhgj

Actual results:
At step 9: "undefined" in the find textbox, and "undefined" is selected
At step 10: beeped, but textbox background color did not change

Expected results:
At step 9: "Find in page" in the find textbox
At step 10: beeped, and red

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=b7fe10c2833b71275de4a8583b193f1a30f3e199&tochange=cb009c395002e0a6bfbfa429448f6036c3ea6f2e
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d40c98b9efbf728f7fa542150ca8023dfbee022&tochange=74bb778f78793e82cfcae11446387795cb4d4180

Comment 5

[Alice0775 White]

And no status message("n of m matches", "Reached end of page, continued from top", "Reached top of page, continued from bottom", "Phrase not found")

Comment 6

[Jorg K (CEST = GMT+2)]

Thank you so much, Alice. Richard Marti mentioned the "undefined" to me, but I never saw it myself.
So this is from bug 1441935 and our port in bug 1519078.
Who do we ask while Magnus is away?

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1562677-findbar-hack-1.diff

This is a hack. Here's the real culprit – I'm not sure why this synchronous XMLHttpRequest kills the find bar, but it does.

Patrick, I'm asking you for review since there's a distinct lack of other suitable people available at the moment. (Also, you clearly want to see this bug fixed.)

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 9075616 [details] [diff] [review]
1562677-findbar-hack-1.diff

Why is a Thunderbird peer not suitable for review here?

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 9075616 [details] [diff] [review]
1562677-findbar-hack-1.diff

Thanks for fixing this! I'm assuming my review is not necessary anymore. :)

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9075616 [details] [diff] [review]
1562677-findbar-hack-1.diff

Review of attachment 9075616 [details] [diff] [review]:
-----------------------------------------------------------------

I'm looking at this again and I have some question. Should you land it while I sleep, either the original version or addressing my comment, please change the commit message to:
Re-initialise find toolbar's browser to work around broken find bar. r=jorgk

::: mail/base/content/mailTabs.js
@@ +505,5 @@
>      if (aIsFirstTab) {
>        aTab.folderDisplay.messenger = messenger;
> +
> +      // By reassigning this here, we fix the find bar (bug 1562677).
> +      document.getElementById("FindToolbar").browser = document.getElementById("messagepane");

Why is this not needed in the else branch? OK, I tested with debug to see which branch is run and it doesn't seem necessary. But why?

Next question: We don't be move this hunk into openFirstTab() which is the only caller which passes true.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c3b1828cbcbe
Re-initialise find toolbar's browser to work around broken find bar. r=jorgk

Comment 12

[Jorg K (CEST = GMT+2)]

TB 68 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/0a9fb6aaba41849490869e849bd935bb3be4d50c