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)
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)
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.
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
Striking QA-Wanted tag (for now) - it seems this bug is By Design according to comment 2
Keywords: qawanted
Comment 4•10 years ago
|
||
After discussing offline with Francisco, the highlighting of the special characters is not implemented. That's a new feature.
blocking-b2g: --- → backlog
status-b2g-v2.2:
--- → affected
Updated•10 years ago
|
Summary: Contact search with character ñ is matched to character n → Accented characters like ñ are not highlighted after doing searching with them
Reporter | ||
Comment 5•10 years ago
|
||
Thanks for the investigation and confirmation.
Updated•10 years ago
|
Summary: Accented characters like ñ are not highlighted after doing searching with them → Accented characters like ñ are not highlighted after doing a search with them
Reporter | ||
Comment 6•10 years ago
|
||
(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?
Updated•10 years ago
|
Assignee: nobody → hola
Target Milestone: --- → 2.1 S8 (7Nov)
Assignee | ||
Comment 7•10 years ago
|
||
Tests for highlight are broken and they are being fixed in bug 1092812.
Depends on: 1092812
Assignee | ||
Comment 8•10 years ago
|
||
Updated•10 years ago
|
Target Milestone: 2.1 S8 (7Nov) → 2.1 S9 (21Nov)
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
Comment on attachment 8516602 [details] [review] Pull request #25802 we do need another round thanks Adrian
Attachment #8516602 -
Flags: review?(jmcf)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8516602 [details] [review] Pull request #25802 Everything pointed out has been fixed. Thanks.
Attachment #8516602 -
Flags: review?(jmcf)
Comment 21•10 years ago
|
||
Comment on attachment 8516602 [details] [review] Pull request #25802 thanks Adrian good work!
Attachment #8516602 -
Flags: review?(jmcf) → review+
Comment 22•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/1719e9f1385e85ab516fa3f6d2726687a5d16ef5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 23•10 years ago
|
||
That patch landed with a perma red Gij 1 test. This needs to be backed out.
Comment 24•10 years ago
|
||
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 → ---
Comment 25•10 years ago
|
||
(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
Comment 26•10 years ago
|
||
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
Comment 27•10 years ago
|
||
(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
Updated•9 years ago
|
Target Milestone: 2.1 S9 (21Nov) → 2.2 S1 (5dec)
Updated•9 years ago
|
Whiteboard: [p=1]
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8516602 -
Attachment is obsolete: true
Assignee | ||
Comment 29•9 years ago
|
||
Link to commit: https://github.com/ADLR-es/gaia/commit/bc28e3858f97ae7927c82156cb12606b2b1abed1
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
Comment 30•9 years ago
|
||
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)
Comment 31•9 years ago
|
||
(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)
Comment 32•9 years ago
|
||
(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.
Comment 33•9 years ago
|
||
(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
Comment 34•9 years ago
|
||
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)
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment 35•9 years ago
|
||
(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)
Comment 36•9 years ago
|
||
(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.
Comment 37•9 years ago
|
||
and this other one https://bugzilla.mozilla.org/show_bug.cgi?id=1100871
Comment 38•9 years ago
|
||
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)
Comment 39•9 years ago
|
||
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
Updated•9 years ago
|
Comment 41•9 years ago
|
||
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).
Comment 42•9 years ago
|
||
Comment 43•9 years ago
|
||
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Comment 44•9 years ago
|
||
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)
Comment 45•9 years ago
|
||
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!
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1S:
--- → unaffected
status-b2g-master:
--- → fixed
Flags: needinfo?(jocheng)
Comment 46•9 years ago
|
||
(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. :)
Comment 47•9 years ago
|
||
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
Keywords: verifyme
Comment 48•9 years ago
|
||
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•