1283042 - [New Find Toolbar] Highlighting is broken on XML pages

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Liviu Cirdei]

Attachment: xml_find_issue.mp4

[Affected platforms]: Windows 10, Mac OS X 10.10, Ubuntu 14.10

[Affected versions]: Nightly 50, Build ID  20160628030238

[Steps to reproduce]:

1. Open Nightly (be sure you have "findbar.modalHighlight" and "findbar.highlightAll" set to true).
2. Go to this XML page: http://www.w3schools.com/xml/cd_catalog.xml
3. Press CTRL+F to open Find toolbar.
4. Search for any word that appears on the page (e.g "empire").

 
[Expected Results]: The matches are highlighted, first one with a yellow background and the other ones with a high contrast background.

[Actual Result]: Highlighting is broken, the page is not dimmed. The match appears as you type in the top-left corner of the screen, and disappears after you finished typing. In addition, on Windows 10 and Ubuntu 14.10 the match from the page becomes white as you type. Also the Find Toolbar shown that no matching words are found on the page, even if they exist.

See the attached video with the issue.

Comment 1

[Mike de Boer [:mikedeboer]]

Part of the story here is that it's already broken without modal highlighting turned on (bug 1054845). But we'll make it look better at least here.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1283042 - make sure we are able to render the DOM elements of the modal highlighter when using the find toolbar on XML documents.

Review commit: https://reviewboard.mozilla.org/r/77290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77290/

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8788994 [details]
Bug 1283042 - make sure we are able to render the DOM elements of the modal highlighter when using the find toolbar on XML documents.

https://reviewboard.mozilla.org/r/77290/#review75540

Yep! Makes sense to me. Can you test that this also fixes SVG documents?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

I forgot to add, it would be nice to include a test for this since we obviously didn't have one before.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Yep! Makes sense to me. Can you test that this also fixes SVG documents?

I'm not sure what the expected behavior is for SVG documents? Can you provide an example?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> I forgot to add, it would be nice to include a test for this since we
> obviously didn't have one before.

Yup, one test coming up.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8788994 [details]
Bug 1283042 - make sure we are able to render the DOM elements of the modal highlighter when using the find toolbar on XML documents.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77290/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/287ff570b1a1
make sure we are able to render the DOM elements of the modal highlighter when using the find toolbar on XML documents. r=jaws

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: add a test for highlighting XML documents; needed to fix up highlightFinished notifications

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8789851 [details] [diff] [review]
Patch 2: add a test for highlighting XML documents; needed to fix up highlightFinished notifications

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

::: toolkit/modules/tests/browser/browser_FinderHighlighter.js
@@ +328,5 @@
> +    yield promiseOpenFindbar(findbar);
> +
> +    let word = "Example";
> +    let expectedResult = {
> +      rectCount: 0,

Shouldn't rectCount equal 1 here because it found 1 result?

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Shouldn't rectCount equal 1 here because it found 1 result?

No, because of bug 1054845 :( I should add a comment there to clarify.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: test.svg

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> > Yep! Makes sense to me. Can you test that this also fixes SVG documents?
> 
> I'm not sure what the expected behavior is for SVG documents? Can you
> provide an example?
> 
See attached.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

To add to comment #11, right now if you search in that attachment for "o" the findbar says "1 of 1 match" but you can hit next and previous to cycle between the two "o" characters in the document, so this breaks the match count also.

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> > Shouldn't rectCount equal 1 here because it found 1 result?
> 
> No, because of bug 1054845 :( I should add a comment there to clarify.

Bug 1054845 is about Highlight All not working, but shouldn't we still have 1 rect for the currently selected match?

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/287ff570b1a1

Comment 14

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Bug 1054845 is about Highlight All not working, but shouldn't we still have
> 1 rect for the currently selected match?

No. Since the iterator doesn't yield any matches due to bug 1054845, we also don't paint the selected match in the mask. We do paint the yellow box.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8789851 [details] [diff] [review]
Patch 2: add a test for highlighting XML documents; needed to fix up highlightFinished notifications

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

Thanks for the test and entertaining all of my questions :)

Comment 16

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/b36aa2b45076542947324127f604a1058cc66ecf
Bug 1283042 - add a test for highlighting XML documents; needed to fix up highlightFinished notifications. r=jaws

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b36aa2b45076

Comment 18

[Carmen]

I was able to reproduce the issue using Nightly 50.0a1 (2016-06-28).

I verified the fix on latest Nightly (2016-09-13) and here are my results:
- page is dimmed, the found word is highlighted with a yellow background; 
- even though Highlight All is enabled by default, it's not highlighting all the found words on the page, but only the first one (maybe related to this bug 1054845)
- searching special characters and writing a word that exists in the XML doc right after the special character (e.g. "<title"), only the special character is highlighted with an yellow backgroung, the word written on the Findbar seems to be displayed one row below compared to where it is located on the page; this way, it overlaps the row bellow -> http://imgur.com/a/6qtTK; I guess this has to do with this bug 1302887

Platforms used to verify the fix: Win 10 Pro x64, Ubuntu 16.04 x64, Mac OS 10.11 El Capitan Beta

Comment 19

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 (20160914030200)

I can also confirm the fixes for the following (when following the STR from the Description):
- the page is now dimmed 
- as you type in the Find bar, the matches appear in the proper position and remain visible (except for the special characters searching mentioned in Comment 18)
- the first match is yellow highlighted (including Windows 10 and Ubuntu 16.04).

But, the "Highlight all" is still not working (Bug 1054845) and the find toolbar still shows that no matching words are found on the page (even when they exists).

Mike, should I file a new bug for the last mentioned issue, or is this something already covered by an existing bug?

Comment 20

[Mike de Boer [:mikedeboer]]

(In reply to Simona B [:simonab ] from comment #19)
> Mike, should I file a new bug for the last mentioned issue, or is this
> something already covered by an existing bug?

Already covered by the existing bug you mentioned. Thanks!

Comment 21

[Simona Badau, Desktop QA]

Based on Comment 20, marking this as Verified on Firefox 51.

Comment 22

[Simona Badau, Desktop QA]

Verified fixed also on Windows 10, Ubuntu 16.04 and Mac OS X 10.11 using the latest Nightly 52.0a1 (Build ID: 20160926030203).

Comment 23

[Simona Badau, Desktop QA]

(In reply to Simona B [:simonab ] from comment #22)
> Verified fixed also on Windows 10, Ubuntu 16.04 and Mac OS X 10.11 using the
> latest Nightly 52.0a1 (Build ID: 20160926030203).

Setting the status-firefox51 to disabled because the modal highlight feature is disabled by default in Firefox 51.