1131970 - [Contacts] Highlight is broken after contact node structure changed for bidi

Status: VERIFIED FIXED

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

Description

[Adrián de la Rosa]

Highlighting code is dependent on the structure of html tags in the contact text to support highlighting inside <strong> tags without breaking them.

I realized that this structure has changed and now the whole contact text is enclosed in a <bdi> tag, which breaks highlighting when searching for any character of the tag <strong>.

Steps to reproduce:
 1. Get some contacts with name and surname.
 2. Search for a substring of 'strong'.

What happens:
 The contact text will be changed and highlighting won't work.

What should happen:
 The contact text should stay the same and highlight has to be correctly applied on it.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] ADLR-es:highlight-broken > mozilla-b2g:master

Comment 2

[Adrián de la Rosa]

Comment on attachment 8562726 [details] [review]
[gaia] ADLR-es:highlight-broken > mozilla-b2g:master

Patch submitted. I used it to change <span class="highlight"> by <mark> tags too, which are more semantic and since we don't need different highlight classes, we can do it this way now.

The changes that fix the bug reported here are https://github.com/ADLR-es/gaia/compare/highlight-broken?expand=1#diff-139a10c960db3187930ef89ff828a686R181 and https://github.com/ADLR-es/gaia/compare/highlight-broken?expand=1#diff-32b69f9638ac9fcad175c776a92ecefaR27. If needed, I can split those two lines in a different bug.

I don't know yet what broke this because tests related to highlighting were not working before this patch.

Comment 3

[Jose Manuel Cantera]

Comment on attachment 8562726 [details] [review]
[gaia] ADLR-es:highlight-broken > mozilla-b2g:master

LGTM

thanks Adrian

Comment 4

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/28143

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.

Comment 5

[Maria Angeles Oteo (:oteo)]

Just adding that I am not able to reproduce the issue with the lastest in 2.2 branch, so it seems specific of master branch.

Environmental Variables:
Device: Flame 2.2 
Build ID: 20150212055743
Gaia: 791e537
Gecko: 2a90b4d
Version: 37.0a2

Comment 6

[Jose Manuel Cantera]

(In reply to Maria Angeles Oteo (:oteo) from comment #5)
> Just adding that I am not able to reproduce the issue with the lastest in
> 2.2 branch, so it seems specific of master branch.
> 
> Environmental Variables:
> Device: Flame 2.2 
> Build ID: 20150212055743
> Gaia: 791e537
> Gecko: 2a90b4d
> Version: 37.0a2

probably because the corresponding RTL patch that breaks this has not been uplifted yet to v2.2

Comment 7

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/7c5b3143d699365d9dfdba5600ac17fd14696074

Comment 8

[Jose Manuel Cantera]

Comment on attachment 8562726 [details] [review]
[gaia] ADLR-es:highlight-broken > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: high as search highlighting would not be available breaking the visual aspect. 
[Testing completed]: Yes, unit tests provided. 
[Risk to taking this patch] (and alternatives if risky): Low risk patch
[String changes made]: None

Comment 9

[Lancy(Leave from Mozilla)]

Attachment: Verify1_1131970_video.MP4

This issue has been verified successfully on Flame 3.0.
See attachment: Verify1_1131970_video.MP4
Reproducing rate:0/10

Flame 3.0 build:
Build ID               20150308160204
Gaia Revision          fea83511df9ccba64259346bc02ebf2c417a12c2
Gaia Date              2015-03-08 06:36:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab4a81e4457
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150308.192120
Firmware Date          Sun Mar  8 19:21:31 EDT 2015
Bootloader             L1TC000118D0

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Don't leave the v2.2 status set to unaffected if you're expecting a timely uplift.

v2.2: https://github.com/mozilla-b2g/gaia/commit/15b6790f315e37f8c8638ad825972d60124b79d0

Comment 11

[Ryan VanderMeulen [:RyanVM]]

And reverted for Linter failures.
v2.2: https://github.com/mozilla-b2g/gaia/commit/473cd63f53c855299b719285d9b95e3f2910782f

https://treeherder.mozilla.org/logviewer.html#?job_id=86291&repo=mozilla-b2g37_v2_2

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Jose, maybe you can help with the rebase?

Comment 13

[Jose Manuel Cantera]

Adrian will take care

thanks

Comment 14

[Maria Angeles Oteo (:oteo)]

(In reply to Jose Manuel Cantera from comment #6)

> probably because the corresponding RTL patch that breaks this has not been
> uplifted yet to v2.2

Tested again in latest 2.2 branch and I am not able to reproduce this bug in that branch.

Environmental Variables:
Device: Flame 2.2 
Build ID: 20150407063018
Gaia: 9a7cc87
Gecko: 6935610
Version: 37.0

After talking with Adrian, it seems that initially the issue reported in master was introduced by Bug 1058790 that was uplifted to 2.2 branch more than 2 months ago, so it seems that it was already fixed in other patch in 2.2 so it's not expected that can happen in that release.
For that reason, setting status-b2g-v2.2 as unaffected.

Comment 15

[helen（leave from Mozilla）]

Per comment 9 & comment 14, clear "verifyme" keyword.