[Email][Search] Inputting then quickly deleting characters in the Email Search field causes text overlap with erroneous search results..

VERIFIED FIXED in 2.2 S5 (6feb)

Status

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Marty, Assigned: jrburke)

Tracking

({regression})

unspecified
2.2 S5 (6feb)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [2.2-Daily-Testing])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Description:
If the user inputs a character into the search field, then quickly deletes that character, the message "No matches in locally cached messages" will overlap with the erroneous search results for the previously inputted character.

The timing of the issue seems to indicate that the character is deleted at the same time, or just before the search results are being returned.

Repro Steps:
1) Update a Flame device to BuildID: 20141211040208
2) Connect to a WiFi or Data network.
3) Launch the Email app and sign in to an email account
4) Sync the inbox for the account, then scroll up and tap the Search Mail field.
5) In the search field, input a character, then quickly delete that character.
  
Actual:
"No matches in locally cached messages" is overlapping erroneous search results.

Expected: 
Only "No matches in locally cached messages" text is displayed.

Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141211040208
Gaia: 1a956437210d2b16988d2ddbf40c9a38d8474435
Gecko: d92db71d4e67
Gonk: 263b5f41f7733c5577fb101eb4dc8ac5c11cfa8d
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
  
Repro frequency: 5/7
See attached: screenshot, logcat
(Reporter)

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Qawanted for branch checks.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: qawanted
The bug repros on Flame 2.2 engineering with shallow flash.
Actual result: Tapping a letter then deleting it quickly can cause the search results to be imposed over the "No matches in locally cached messages" text.

BuildID: 20141211040110
Gaia: 1a956437210d2b16988d2ddbf40c9a38d8474435
Gecko: d92db71d4e67
Platform Version: 37.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

--------------------------------------------------------------------------------------------------------

The bug does not repro on Flame 2.1 engineering with shallow flash.
Actual result: The "No matches in locally cached messages" does not show when there is nothing in the search textbox, so there is no chance of it being imposed over the search results.

BuildID: 20141210190812
Gaia: 97873dca486abf4162a3345e71b375806937bdec
Gecko: 9faa165ac85d
Platform Version: 34.0
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawantedregression
QA Contact: ckreinbring
Nomming for 2.2 - Regression, Broken Functionality, Poor UX
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Regression window
Last working
BuildID: 20141105120748
Gaia: 7918024c737c4570cacd784f267e28737ae05dea
Gecko: b6cd2dd85b26
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First broken
BuildID: 20141106040023
Gaia: 068b9711277b06c7d633517f9e1fcb5624bb39b3
Gecko: a074c0112919
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Working Gaia / Broken Gecko = No repro
Gaia: 7918024c737c4570cacd784f267e28737ae05dea
Gecko: a074c0112919
Broken Gaia / Working Gecko = Repro
Gaia: 068b9711277b06c7d633517f9e1fcb5624bb39b3
Gecko: b6cd2dd85b26
Gaia pushlog: https://github.com/mozilla-b2g/gaia/compare/7918024c737c4570cacd784f267e28737ae05dea...068b9711277b06c7d633517f9e1fcb5624bb39b3


B2G Inbound
Last working
BuildID: 20141105122245
Gaia: 774e560f8079016199baeef861575492f2af75f7
Gecko: 938deea820e9
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First broken
BuildID: 20141105140350
Gaia: 81b1a5c13696b77df31bccae36493879d9be331e
Gecko: a27a7143352c
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Working Gaia / Broken Gecko = No repro
Gaia: 774e560f8079016199baeef861575492f2af75f7
Gecko: a27a7143352c
Broken Gaia / Working Gecko = Repro
Gaia: 81b1a5c13696b77df31bccae36493879d9be331e
Gecko: 938deea820e9
Gecko pushlog: https://github.com/mozilla-b2g/gaia/compare/774e560f8079016199baeef861575492f2af75f7...81b1a5c13696b77df31bccae36493879d9be331e
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Issue is caused by the patch to Bug 1091736

James - I'm not able to find the patch author - rsajdok  - so I'm NI'ing you (patch reviewer) - can you push this issue in the correct direction? Thanks!
Blocks: 1091736
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(jrburke)
QA Contact: ckreinbring
(Assignee)

Comment 7

4 years ago
Posted file GitHub pull request (obsolete) —
This is a tricky one because of the async nature of the slice and vscroll updates and when message_list's showSearch is called. Also probably collusion by the search slice implementation:

showSearch is called on user typing events. This means the `if (phrase.length < 1) this.showEmptyLayout();` logic will execute before the previous search results have finished their slice event notifications and for the vScroll's setData work as async updated the display.

Since showSearch does not trigger a new search slice result for the an empty this.curPhrase, the observed behavior is a result of that tension.

Ideally, I think it might be best if we could construct a search query that says "no phrase and no results expected". Right now passing a blank this.curPhrase results in all messages being returned. I can see a logic behind that, but I think given the async nature of these APIs, allowing a search result to be empty and to mean "no results" would help the logic stay consistently the same in consumers.

What I have done in this patch though is to keep the search slice the same and the early return in showSearch as-is but place extra guards. If a search is involved with no this.curPhrase, report the listFunc.size as zero and to make sure the splice notification does not remove the "empty results" message in that case. This fixes the issue and keeps the fix impact very small and limited to search modes.
Flags: needinfo?(jrburke)
Attachment #8536833 - Flags: review?(bugmail)
Comment on attachment 8536833 [details] [review]
GitHub pull request

Let's fix this in the back-end, like you propose.  The front-end logic can avoid the complexity/need for the (good) comment, and the diff should still be minimal.

The MessageFilterer in searchslice.js will already return false as long as filters is empty.  I suggest we alter SearchSlice, wrapping the whatToSearch and phrase conversion to RegExp inside an if (phrase) sort of thing.  That way an empty phrase will not create any filters.

test_search_slice.js does exist now, and it would want coverage.  Basically I think we create a folder with like 3 messages in it, call searchFolderMessages with an empty string, and put a lazy logger on the slice's oncomplete notification that logs when it's complete and the number of items.  And expect it to be zero.
Attachment #8536833 - Flags: review?(bugmail) → review-

Updated

4 years ago
blocking-b2g: 2.2? → 2.2+
(Assignee)

Updated

4 years ago
Assignee: nobody → jrburke
(Assignee)

Comment 9

4 years ago
Posted file GELAM pull request
Empty phrases should result in an zero length search result. Takes the approach described in comment 8.
Attachment #8554718 - Flags: review?(bugmail)
(Assignee)

Comment 10

4 years ago
Posted file Gaia pull request
Gaia patch that includes the GELAM change and the removal of the phrase.length 0 early return from message_list.js. The pull request fixes the problem when using flame master.
Attachment #8536833 - Attachment is obsolete: true
Attachment #8554719 - Flags: review?(bugmail)
Comment on attachment 8554718 [details] [review]
GELAM pull request

looks good!  one nit in PR that's not important enough to fix, but if you're jonesing for a rebase, I won't fight you on it ;)
Attachment #8554718 - Flags: review?(bugmail) → review+
Attachment #8554719 - Flags: review?(bugmail) → review+
(Assignee)

Comment 12

4 years ago
Fixed the whitespace issue and rebased. Automation passed, so merged.

Merged in GELAM master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/9d2d837fcb7f0a9ec29dc271b2ec78fc9f7b5b7a

from pull request:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/364

---

Merged in Gaia master:
https://github.com/mozilla-b2g/gaia/commit/58e5a4ce068958b4af8ce9bc5722c407b921abd2

from pull request:
https://github.com/mozilla-b2g/gaia/pull/27688
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

4 years ago
Comment on attachment 8554719 [details] [review]
Gaia pull request

[Approval Request Comment]
Asking for 2.2 since this was marked as blocking-b2g 2.2+:

[Bug caused by] (feature/regressing bug #):
The changes in bug 1091736 highlighted a deeper issue around empty search phrases.

[User impact] if declined:
If the user types fast enough and deletes some text, could get the overlapping "no messages found" used for empty search results and actual search results from a previous search.

[Testing completed]:
On Flame device, and back-end change has a test in the GELAM repo.

[Risk to taking this patch] (and alternatives if risky):
Very low risk. Only affects empty search phrases, does not affect other message displays. The conceptual change is very small, most of the change is whitespace indentation.

[String changes made]:
none
Attachment #8554719 - Flags: approval-gaia-v2.2?
Attachment #8554719 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
This issue is verified fixed on Flame 2.2 and Master.

Result: The overlapping issue does not exist anymore. 
 
Device: Flame 2.2 (319mb, full flash)
Build ID: 20150202002507
Gaia: d6141fa3208f224393269e17c39d1fe53b7e6a05
Gecko: be206fa2fb60
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame Master (319mb, full flash)
Build ID: 20150202010229
Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a
Gecko: 940118b1adcd
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.