Closed Bug 1303874 Opened 7 years ago Closed 7 years ago

Sometimes the white highlights disappear

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Type:
defect
Priority:
Not set
Severity:
normal
Points:
2

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla52
Iteration:
52.1 - Oct 3
Tracking Flags:
Tracking Status
firefox52 --- verified

People

(Reporter: jrmuizel, Assigned: mikedeboer)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(4 files)

STR1.mp4
2.54 MB, video/mp4
Details
STR2.mp4
2.65 MB, video/mp4
Details
Bug 1303874 - clear the dictionary for the current find window only upon restart of the iterator, not each reset.
58 bytes, text/x-review-board-request
jaws
: review+
Details
Patch 2: make the active window object part of the iterator params
6.02 KB, patch
jaws
: review+
Details | Diff | Splinter Review 
I'm not sure how to reproduce this, but it seems to happen sometimes when switching tabs.
STR might be something like this:
- Search for X
- Switch to another tab and search for Y there
- Switch to original tab and use the search for Y there. Other Y's on the original page are not highlighted.
Blocks: 1291278
I've also reproduced this issue on the latest Nightly 52.0a1 (Build ID: 20160920030429). Here are the scenarios that I used:

STR1:
1. Launch Firefox
2. Navigate to: https://developer.mozilla.org/en-US/
3. Press CTRL+F to open Find toolbar
4. Search for "to"
5. Click twice on the "Highlight All" button  
6. Click once on the page content to lose the dimmed background
7. In the Find Toolbar, click on the input box and click Enter

Actual results: 
After step 7 - there is only one match for "to". Please see STR1 screen-cast for more details.

STR2:
1. Launch Firefox
2. Navigate to: https://www.reddit.com/
3. Press CTRL+F to open Find toolbar
4. Scroll down the page 
5. In the Find Toolbar, click on the input box and click Enter

Actual results: 
In step 5 - only one match for "ago" is highlighted. Please see STR2 screen-cast for more details.
Attached video STR1.mp4 — 

    
    

    
  

      
      
      
      

        Attached video
          
        STR2.mp4
        — 
      

    


  

    
    

    
      
  

    
  
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 52.1 - Oct 3
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+

    
    

    
      
  

    
  
Blocks: 1301684

    
  
Blocks: 1302033

    
    

    
  
Comment on attachment 8793381 [details]
Bug 1303874 - clear the dictionary for the current find window only upon restart of the iterator, not each reset.

https://reviewboard.mozilla.org/r/80130/#review79392

It doesn't seem like our current automated tests cover this case, and the changes in this patch don't increase test coverage. How can we be sure this won't break in the future?

        Attachment #8793381 -
        Flags: review?(jaws) → review+

    
    

    
  
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/464d8056f9de
clear the dictionary for the current find window only upon restart of the iterator, not each reset. r=jaws

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> It doesn't seem like our current automated tests cover this case, and the
> changes in this patch don't increase test coverage. How can we be sure this
> won't break in the future?

Well, in browser_FinderHighlighter.js you saw the update of the expectedResult.rects number from 2 to 1, right? That's the bug! :/

Very, well, subtle.

    
    

    
  
Backed out for failing mochitest-chrome-3 test_bug263683.xul on Linux:

https://hg.mozilla.org/integration/autoland/rev/a33dcaa0f804

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=464d8056f9de69d7133cc507123a44b8d3d0f6df
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=3946316&repo=autoland

[task 2016-09-23T14:46:52.725344Z] 14:46:52     INFO -  776 INFO TEST-START | toolkit/content/tests/chrome/test_bug263683.xul
[task 2016-09-23T14:46:55.158695Z] 14:46:55     INFO -  TEST-INFO | started process screentopng
[task 2016-09-23T14:46:55.815557Z] 14:46:55     INFO -  TEST-INFO | screentopng: exit 0
[task 2016-09-23T14:46:55.815826Z] 14:46:55     INFO -  777 INFO Starting test with browser 'content'
[task 2016-09-23T14:46:55.816047Z] 14:46:55     INFO -  778 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly detects new selection type - true == true
[task 2016-09-23T14:46:55.816545Z] 14:46:55     INFO -  779 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 1 == 1
[task 2016-09-23T14:46:55.816923Z] 14:46:55     INFO -  780 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla"
[task 2016-09-23T14:46:55.817018Z] 14:46:55     INFO -  781 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly removed the range - 0 == 0
[task 2016-09-23T14:46:55.817816Z] 14:46:55     INFO -  782 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match from input to the selection type - 1 == 1
[task 2016-09-23T14:46:55.818446Z] 14:46:55     INFO -  783 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla"
[task 2016-09-23T14:46:55.818583Z] 14:46:55     INFO -  784 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly removed the range - 0 == 0
[task 2016-09-23T14:46:55.819298Z] 14:46:55     INFO -  785 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 1 == 1
[task 2016-09-23T14:46:55.819390Z] 14:46:55     INFO -  786 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla"
[task 2016-09-23T14:46:55.820096Z] 14:46:55     INFO -  787 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly added a match to the selection type - 1 == 1
[task 2016-09-23T14:46:55.820716Z] 14:46:55     INFO -  788 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Added the correct match - "mozilla" == "mozilla"
[task 2016-09-23T14:46:55.820845Z] 14:46:55     INFO -  789 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Found correct amount of matches
[task 2016-09-23T14:46:55.821448Z] 14:46:55     INFO -  790 INFO TEST-PASS | toolkit/content/tests/chrome/test_bug263683.xul | Correctly removed the range - 0 == 0
[task 2016-09-23T14:46:55.822525Z] 14:46:55     INFO -  791 INFO TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_bug263683.xul | Correctly removed the range - 1 == 0
[task 2016-09-23T14:46:55.823548Z] 14:46:55     INFO -  receiveMessage@resource://testing-common/ContentTask.jsm:117:7
[task 2016-09-23T14:46:55.824426Z] 14:46:55     INFO -  792 INFO Starting test with browser 'content-remote'
Flags: needinfo?(mdeboer)

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> > It doesn't seem like our current automated tests cover this case, and the
> > changes in this patch don't increase test coverage. How can we be sure this
> > won't break in the future?
> 
> Well, in browser_FinderHighlighter.js you saw the update of the
> expectedResult.rects number from 2 to 1, right? That's the bug! :/
> 
> Very, well, subtle.

A bit too subtle I think actually. These numbers change with many of the patches that are landing, so I think it might be easy to accidentally overlook introducing a regression.

    
    

    
      
  

    
    

    
  
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee0663840951
clear the dictionary for the current find window only upon restart of the iterator, not each reset. r=jaws

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ee0663840951
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox52:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

    
  
Flags: needinfo?(mdeboer)
QA Contact: brindusa.tot

    
    

    
  
The issue is still reproducible on the latest Nightly 52.0a1, build ID: 20160929030426 , on Mac OS X 10.11 using the steps from the Description (Comment 1).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Comment on attachment 8797992 [details] [diff] [review]
Patch 2: make the active window object part of the iterator params

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

::: toolkit/modules/FinderIterator.jsm
@@ +117,5 @@
>  
>      let window = finder._getWindow();
>      let resolver;
>      let promise = new Promise(resolve => resolver = resolve);
> +    let iterParams = { caseSensitive, entireWord, linksOnly, useCache, window, word };

The docs above this function need updating to include `window`.

        Attachment #8797992 -
        Flags: review?(jaws) → review+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e3ee9b41edc9d70863704fad44a50d283bb886
Bug 1303874 - make the active window object part of the iterator params to make sure that similar iterator runs for different runs are not treated as the same, thus potentially yielding incorrect results. r=jaws

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/67e3ee9b41ed
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED

    
    

    
  
Verified fixed using the latest Nightly 52.0a1 (Build ID:20161016030205) on Ubuntu 16.04, Windows 10 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED

    
  
Regressions: 1605575

          You need to log in
          before you can comment on or make changes to this bug.