Closed Bug 1088706 Opened 10 years ago Closed 9 years ago

Accented characters like ñ are not highlighted after doing a search with them

Categories

(Firefox OS Graveyard :: Gaia::Contacts, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S1 (5dec)
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: brg, Assigned: hola)

References

Details

(Whiteboard: [p=1])

Attachments

(5 files, 1 obsolete file)

ñ character belongs to Spanish alphabet and it is used in some names and surnames.

When you try to search a contact with this character in your contact list,
1) it is matched to character n as well.
2) The names containing letter ñ are listed but not highlighted. See attachment.

Tested with Flame, master, Gecko-0e0825b.Gaia-dedd8c6

My Android device make the search to character ñ and does not offer other options with n.
QA wanted for a branch check.
Keywords: qawanted
Not a bug but a feature. We were asked by product to increase the matches trying to avoid special keyboard combinations.

So a user doesn't have to long press on 'n' and then select 'ñ', it's a common practice to match those for fast typing search.
Striking QA-Wanted tag (for now) - it seems this bug is By Design according to comment 2
Keywords: qawanted
After discussing offline with Francisco, the highlighting of the special characters is not implemented. That's a new feature.
blocking-b2g: --- → backlog
Summary: Contact search with character ñ is matched to character n → Accented characters like ñ are not highlighted after doing searching with them
Thanks for the investigation and confirmation.
Summary: Accented characters like ñ are not highlighted after doing searching with them → Accented characters like ñ are not highlighted after doing a search with them
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #2) 
> So a user doesn't have to long press on 'n' and then select 'ñ', it's a
> common practice to match those for fast typing search.

long press on 'n'? not needed if you are Spanish and configure your device in Spanish. IMHO this explanation could fit accented characters but not for ñ letter which had a dedicated key in Spanish keyboard. Could you please review this design decision?
Assignee: nobody → hola
Target Milestone: --- → 2.1 S8 (7Nov)
Tests for highlight are broken and they are being fixed in bug 1092812.
Depends on: 1092812
Attached file Pull request #25802 (obsolete) —
Target Milestone: 2.1 S8 (7Nov) → 2.1 S9 (21Nov)
Comment on attachment 8516602 [details] [review]
Pull request #25802

Pull request ready for review.

Now search works from and to accented characters.
New test added to check against strange duplications.
New suite to group tests related to search to avoid code duplication, tests that were depending on other tests and fix issues with assertHighlight.
Better regexp for higlighting that avoids breaking html tags.
Attachment #8516602 - Flags: review?(jmcf)
Comment on attachment 8516602 [details] [review]
Pull request #25802

cancelling review as we are going to refactor the suite corresponding to search in a new test file search_test.js.
Attachment #8516602 - Flags: review?(jmcf)
No longer depends on: 1092812
Comment on attachment 8516602 [details] [review]
Pull request #25802

In my PR I fixed the bug and improved the highlighting to be safe from HTML tags, but I also made a new test suite for contacts search, and I wrote there the tests about search and highlighting from list test, adding one more test for this bug. I've also fixed one integration test that tested the opening of search mode and skipped another integration test that was testing indirectly for what is to be fixed in bug 1093523. All references to search dependencies in list tests has been removed as well.

I left two tests commented out to ask for feedback about them, because I would say we can delete them, but I'm not sure.
Attachment #8516602 - Flags: review?(jmcf)
Comment on attachment 8516602 [details] [review]
Pull request #25802

Hi Adrian,

We are on the right way. Please check the comments on GH. 

Wrt integration tests, I would keep them as they are (no change the existing code base) and we should open a follow-up for integration tests. As a result we can progress faster with this bug. 

Thanks!
Attachment #8516602 - Flags: review?(jmcf)
Comment on attachment 8516602 [details] [review]
Pull request #25802

PR updated with the things you pointed in comments. As you can see, I removed the changes to the integration tests and saved them for a follow up.
Attachment #8516602 - Flags: review?(jmcf)
Comment on attachment 8516602 [details] [review]
Pull request #25802

we do need another round

thanks Adrian
Attachment #8516602 - Flags: review?(jmcf)
Comment on attachment 8516602 [details] [review]
Pull request #25802

Everything pointed out has been fixed.

Thanks.
Attachment #8516602 - Flags: review?(jmcf)
Comment on attachment 8516602 [details] [review]
Pull request #25802

thanks Adrian

good work!
Attachment #8516602 - Flags: review?(jmcf) → review+
https://github.com/mozilla-b2g/gaia/commit/1719e9f1385e85ab516fa3f6d2726687a5d16ef5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
That patch landed with a perma red Gij 1 test. This needs to be backed out.
Strange that a unit test patch would break Gij. Must have been the shared/js/contacts/search.js change? In any case, backed out:

master: https://github.com/mozilla-b2g/gaia/commit/8e09627d75acd4abced0ab81983b5b5de6d15881
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Michael Henretty [:mhenretty] from comment #24)
> Strange that a unit test patch would break Gij. Must have been the
> shared/js/contacts/search.js change?

Yup, that must be it. See bug 1101912 for details.
Depends on: 1101912
Jose, The try run for this was not green, that is an absolute must for landing new code

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=74424231c998
(In reply to Dale Harvey (:daleharvey) from comment #26)
> Jose, The try run for this was not green, that is an absolute must for
> landing new code
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=74424231c998

At first glance the failures were not related to this PR, so that's why I landed it. Nonetheless, sorry about that and I will follow the rule in the future
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
Whiteboard: [p=1]
Attached file New pull request
Attachment #8516602 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whoa guys, someone brought this commit to my attention specifically for removing a lot of marionette coverage in favor of unit tests. In addition it looks like we've added test.skip() instead of fixing the test files when landing.

I don't really think it's acceptable to lose out on integration coverage like this, and land with disabled tests. Can you guys explain some of the reasoning behind why we landed like this? I'd like to avoid this type of coverage loss in the future, it makes our code way more brittle.
Flags: needinfo?(jmcf)
Flags: needinfo?(hola)
Flags: needinfo?(francisco)
(In reply to Kevin Grandon :kgrandon from comment #30)
> Whoa guys, someone brought this commit to my attention specifically for
> removing a lot of marionette coverage in favor of unit tests. In addition it
> looks like we've added test.skip() instead of fixing the test files when
> landing.
> 
> I don't really think it's acceptable to lose out on integration coverage
> like this, and land with disabled tests. Can you guys explain some of the
> reasoning behind why we landed like this? I'd like to avoid this type of
> coverage loss in the future, it makes our code way more brittle.

We are going to increase integration test coverage in a follow-up bug
Flags: needinfo?(jmcf)
(In reply to Jose Manuel Cantera from comment #31)
> We are going to increase integration test coverage in a follow-up bug

Sure, we can always increase in follow-ups, but landing features without tests is not something that we should be doing. Especially not disabling tests which were testing the functionality you were changing.

My recommendation is to back this out, and land it properly with tests. I am not a peer of the contacts module though, so I'll leave it up to you. We should absolutely not work like this though.
(In reply to Kevin Grandon :kgrandon from comment #32)
> (In reply to Jose Manuel Cantera from comment #31)
> > We are going to increase integration test coverage in a follow-up bug
> 
> Sure, we can always increase in follow-ups, but landing features without
> tests is not something that we should be doing. Especially not disabling
> tests which were testing the functionality you were changing.
> 
> My recommendation is to back this out, and land it properly with tests. I am
> not a peer of the contacts module though, so I'll leave it up to you. We
> should absolutely not work like this though.

You ARE TOTALLY WRONG. The original tests on contacts search WERE NOT WORKING and we created this patch to solve that issue. In the meantime we had to disable an integration test because we are going to change the highlighting algorithm and we plan to increase the integration test coverage under that specific bug. 

In addition, some of the integration tests that were originally created for contacts were absolutely wrong and caused intermittent failures. We have been landing several patches with integration tests that increase the coverage significantly such as bug 1098218, or bug 1094730 to name a few. So please DO NOT make uninformed considerations. 

In the Contacts App normally no patch passes review until tests (unit or integration are added). 

thanks
This issue is verified as fixed on Flame 2.2.

Actual Result: Contact search will now highlight contacts with characters such as ñ.

Device: Flame 2.2  (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141125040209
Gaia: 824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko: acde07cb4e4d
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0"
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(In reply to Jose Manuel Cantera from comment #33)
> 
> You ARE TOTALLY WRONG. The original tests on contacts search WERE NOT
> WORKING and we created this patch to solve that issue. In the meantime we
> had to disable an integration test because we are going to change the
> highlighting algorithm and we plan to increase the integration test coverage
> under that specific bug. 
> 
> In addition, some of the integration tests that were originally created for
> contacts were absolutely wrong and caused intermittent failures. We have
> been landing several patches with integration tests that increase the
> coverage significantly such as bug 1098218, or bug 1094730 to name a few. So
> please DO NOT make uninformed considerations. 
> 
> In the Contacts App normally no patch passes review until tests (unit or
> integration are added). 
> 
> thanks

This way of communicating is totally unacceptable, I understand Kevin's comments and we can give answers in the same respectful way.

@Kevin, it's true that past week we have been working a lot around integration tests for contacts, adding new libraries to avoid intermittent errors and increasing the coverage.

We have been working lately around contact search highlight since we realized that we had a lot of problems, our unit test suite was totally wrong and the mechanism to perform this (regular expressions) wasn't valid to take into consideration all possible cases. 

Cause of that we have rewritten this part of the code. Don't know the reasons yet for disabling this test, but if the reasons are cause it will take long time to come with a huge suite and we don't want the patch to get rotten, I'm ok with that, with the new set of unit tests and doing a proper follow up.

@Jose, we need to open the follow up immediately (perhaps it's already created), reference it here and schedule it for next sprint, so our colleagues can see that we are actively working on the integration tests and it is not just a vague promise.

Again, we are working in a open source project and it's part of it's nature having people asking us about the work we do, if we are not able to accept this, we are doing it wrong.
Flags: needinfo?(francisco)
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #35)
> (In reply to Jose Manuel Cantera from comment #33)
> > 
> > You ARE TOTALLY WRONG. The original tests on contacts search WERE NOT
> > WORKING and we created this patch to solve that issue. In the meantime we
> > had to disable an integration test because we are going to change the
> > highlighting algorithm and we plan to increase the integration test coverage
> > under that specific bug. 
> > 
> > In addition, some of the integration tests that were originally created for
> > contacts were absolutely wrong and caused intermittent failures. We have
> > been landing several patches with integration tests that increase the
> > coverage significantly such as bug 1098218, or bug 1094730 to name a few. So
> > please DO NOT make uninformed considerations. 
> > 
> > In the Contacts App normally no patch passes review until tests (unit or
> > integration are added). 
> > 
> > thanks
> 
> This way of communicating is totally unacceptable, I understand Kevin's
> comments and we can give answers in the same respectful way.

Just one thing to note, I don't think there was anything unrespectful in my answer. I have my own view on this but If you think the response was unrespectful my apologies, it was not my intention at all. 

> 
> @Kevin, it's true that past week we have been working a lot around
> integration tests for contacts, adding new libraries to avoid intermittent
> errors and increasing the coverage.
> 
> We have been working lately around contact search highlight since we
> realized that we had a lot of problems, our unit test suite was totally
> wrong and the mechanism to perform this (regular expressions) wasn't valid
> to take into consideration all possible cases. 
> 
> Cause of that we have rewritten this part of the code. Don't know the
> reasons yet for disabling this test, but if the reasons are cause it will
> take long time to come with a huge suite and we don't want the patch to get
> rotten, I'm ok with that, with the new set of unit tests and doing a proper
> follow up.
> 
> @Jose, we need to open the follow up immediately (perhaps it's already
> created), reference it here and schedule it for next sprint, so our
> colleagues can see that we are actively working on the integration tests and
> it is not just a vague promise.

https://bugzilla.mozilla.org/show_bug.cgi?id=1104139

> 
> Again, we are working in a open source project and it's part of it's nature
> having people asking us about the work we do, if we are not able to accept
> this, we are doing it wrong.

I have my own view on this, you have your own which I respect.
Ok, thanks for the clarification guys. Again, I was not even originally aware of this bug. Another party brought this to my attention, so yes - there are always people looking at your code in open source projects.

Regardless of that though we are really trying to stress the importance of integration test coverage. From a first glance at the pull request, disabling the tests without any comments in the files, or in the bug, and with no visible follow-up filed would definitely be a cause of concern. I think we can avoid all of this in the future by linking to a follow-up, commenting in the bug, or even adding a // TODO in the code.

I think it's been generally accepted that we want to land feature work with integration tests though. If there's something that makes this difficult in the future, we should talk about that to see how we can make that easier.
Flags: needinfo?(hola)
sure Kevin, totally in favor of integration tests and in the future we will make it clearer the dependencies, follow-ups and plans, just ot avoid misunderstandings and that other people can follow our work properly. 

thanks
See Also: → 1115628
Hi William,

    I can repro this bug on latest Flame v2.0,it will be "wontfix" ? Could you help with this bug?

Thank you.

See attachments: verify_v2.0.png and logcat_v2.0_1043.txt
Reproduce rate: 5/5

Repro STR:
1.Create a new contact with special characters in Name field (ex: "Carlos Morón"). 
2.Tap Search box. 
3.Input the string 'moron' or 'morón'. 
**The contact is found but the matching string is not highlighted properly.  --KO

Flame 2.0 build:
Gaia-Rev        2989f2b2bd12fcc0e9c017d2db766e76a55873b8
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5bf8087572c3
Build-ID        20150128000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150128.033208
FW-Date         Wed Jan 28 03:32:20 EST 2015
Bootloader      L1TC000118D0
--------------------------------------------------------------------
Note:I can't repro this bug on latest Flame v2.1 (20150128001258).
Flags: needinfo?(whsu)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Hi, Josh,

Do we need to uplift this patch to v2.0&v2.0m?
If partners don't need this patch, I think we can change tag-"status-b2g-v2.0" to "WONTFIX".
Thanks.
Flags: needinfo?(whsu) → needinfo?(jocheng)
Hi William,
I see this bug depends on bug 1101912 and only fix on master. Since partner does not report until now, I think we can mark "status-b2g-v2.0" and "status-b2g-v2.0M" to "WONTFIX".
Thanks!
(In reply to Josh Cheng [:josh] from comment #45)
> Hi William,
> I see this bug depends on bug 1101912 and only fix on master. Since partner
> does not report until now, I think we can mark "status-b2g-v2.0" and
> "status-b2g-v2.0M" to "WONTFIX".
> Thanks!

Thanks for your confirmation. :)
This bug has been successfully verified on latest Flame v3.0.
See attachment: verified_v3.0.png.
Reproduce rate: 0/5.

Flame 3.0 build:
Gaia-Rev        ab69ae06a7f2b54e60ab63b1b44c8d19d5d20d94
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/c2359a6a6958
Build-ID        20150201010217
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150201.044915
FW-Date         Sun Feb  1 04:49:25 EST 2015
Bootloader      L1TC000118D0
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.