[New Find Toolbar] Highlighting is broken on XML pages

VERIFIED FIXED in Firefox 52

Status

()

Toolkit
Find Toolbar
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: Liviu Cirdei, Assigned: mikedeboer)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

50 Branch
mozilla51
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox50 affected, firefox51 disabled, firefox52 verified)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

a year ago
Created attachment 8766230 [details]
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.
(Assignee)

Updated

a year ago
Blocks: 1291278
(Assignee)

Comment 1

a year ago
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.
Depends on: 1054845
(Assignee)

Updated

a year ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 51.3 - Sep 12
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
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?
Attachment #8788994 - Flags: review?(jaws) → review+
I forgot to add, it would be nice to include a test for this since we obviously didn't have one before.
(Assignee)

Comment 5

a year ago
(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.
(Assignee)

Updated

a year ago
Keywords: leave-open
Comment hidden (mozreview-request)

Comment 7

a year ago
Pushed by mdeboer@mozilla.com:
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
(Assignee)

Comment 8

a year ago
Created attachment 8789851 [details] [diff] [review]
Patch 2: add a test for highlighting XML documents; needed to fix up highlightFinished notifications
Attachment #8789851 - Flags: review?(jaws)
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?
(Assignee)

Comment 10

a year ago
(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.
Created attachment 8789912 [details]
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.
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/287ff570b1a1
(Assignee)

Comment 14

a year ago
(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 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 :)
Attachment #8789851 - Flags: review?(jaws) → review+
(Assignee)

Comment 16

a year ago
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
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b36aa2b45076
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Comment 18

a year ago
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
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?
Flags: needinfo?(mdeboer)
(Assignee)

Comment 20

a year ago
(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!
Flags: needinfo?(mdeboer)
QA Contact: brindusa.tot
Based on Comment 20, marking this as Verified on Firefox 51.
status-firefox51: fixed → verified
Verified fixed also on Windows 10, Ubuntu 16.04 and Mac OS X 10.11 using the latest Nightly 52.0a1 (Build ID: 20160926030203).
Status: RESOLVED → VERIFIED
status-firefox51: verified → disabled
status-firefox52: --- → verified
(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.
You need to log in before you can comment on or make changes to this bug.