Emphasized highlight does not follow browser resize

VERIFIED FIXED in Firefox 51

Status

()

Toolkit
Find Toolbar
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: Alice0775 White, Assigned: mikedeboer)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(firefox51 verified)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8785382 [details]
screenshot

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/a65b35c8e5b17c2585968974aef1da67a8c56642
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 ID:20160826063819

Steps To reproduce:
1. Enable new findbar
  user_pref("findbar.highlightAll", true);
  user_pref("findbar.modalHighlight", true);
2. Open any web page
3. Open Find Bar (Ctrl+F)
4. Perform find with any term
5. Resize browser

Actual Results:
Emphasized highlight  is wrongly positioned

Expected Results:
Properly positioned
(Assignee)

Updated

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

Comment 2

a year ago
mozreview-review
Comment on attachment 8785933 [details]
Bug 1298435 - upon browser window resize, update all the rects we track for each range we found and correctly reposition all oulines atop the mask.

https://reviewboard.mozilla.org/r/74950/#review72978

::: toolkit/modules/FinderHighlighter.jsm:1018
(Diff revision 1)
>     *   {Boolean} scrollOnly     TRUE when the page has scrolled in the meantime,
>     *                            which means that the fixed positioned elements
>     *                            need to be repainted.
>     */
> -  _scheduleRepaintOfMask(window, { contentChanged, scrollOnly } = { contentChanged: false, scrollOnly: false }) {
> +  _scheduleRepaintOfMask(window, { contentChanged, scrollOnly, updateAllRanges } =
> +                                 { contentChanged: false, scrollOnly: false, updateAllRanges: false }) {

Please add documentation here for updateAllRanges.

::: toolkit/modules/FinderHighlighter.jsm:1035
(Diff revision 1)
>  
>      // When we request to repaint unconditionally, we mean to call
>      // `_repaintHighlightAllMask()` right after the timeout.
>      if (!dict.unconditionalRepaintRequested)
>        dict.unconditionalRepaintRequested = !contentChanged || repaintFixedNodes;
> +    // Some events, like a resize, call for recalculation all the rects of all ranges.

recalculation of all the rects of all ranges.
Attachment #8785933 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)

Comment 4

a year ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf5b8a794ab0
upon browser window resize, update all the rects we track for each range we found and correctly reposition all oulines atop the mask. r=jaws

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf5b8a794ab0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Verified on the latest Nightly Build ID: 20160913030425 on Windows 7 x64 and Windows 10 x64 with https://www.reddit.com/ page , searching for "ago" term and managed to reproduce it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

a year ago
Regression, I have a patch ready.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 8

a year ago
Created attachment 8791608 [details] [diff] [review]
Follow-up patch: make sure the modal highlight outline is always inserted in the top-most window
Attachment #8791608 - Flags: review?(jaws)
Comment on attachment 8791608 [details] [diff] [review]
Follow-up patch: make sure the modal highlight outline is always inserted in the top-most window

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

Should this have been caught by one of our tests?
Attachment #8791608 - Flags: review?(jaws) → review+
(Assignee)

Comment 10

a year ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Should this have been caught by one of our tests?

Well, I think it'd be quite simple to add window-object checking to browser_FinderHighlighter.jsm, even though I think that it's highly unlikely that the same bug will be introduced again.
I will work on this asap in another bug.
(Assignee)

Comment 11

a year ago
https://hg.mozilla.org/integration/fx-team/rev/d8aad5dec43e6e962cc39d2a78c1002c1872d1a7
Bug 1298435 - make sure the modal highlight outline is always inserted in the top-most window. r=jaws
(Assignee)

Updated

a year ago
Blocks: 1303287

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8aad5dec43e
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
This bug is still reproducible on build Nightly 52.0a1, build ID: 20160920030429. 
Bug is reproducible on Windows 10 x64 on https://www.washingtonpost.com/ page when the browser is resized multiple times.
Reopening the bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

a year ago
(In reply to roxana.leitan@softvision.ro from comment #13)
> Bug is reproducible on Windows 10 x64 on https://www.washingtonpost.com/
> page when the browser is resized multiple times.
> Reopening the bug.

This page is causing so many reflows when resizing, it's insane. However, that doesn't mean we shouldn't support it, just that this is simply the best I can do (using the Nightly-to-be for Sept 28th) for now. You notice, for example, that after resizing some more the rectangles do correct themselves. But sometimes ranges simply disappear, because you resize to a low-width/ mobile size and the layout switches. Detection for that is happening in bug 1302470, so that'll make the behavior a _lot_ better.
Any other optimizations to be more correct would be a P4 bug IMO.
I think it'd better to resolve this particular bug, file a new one that depends on bug 1302470 and once that's resolved, we'll see what's left to work on.
Flags: needinfo?(roxana.leitan)
Hi Mike,

I agree that the initial issue reported in this bug is resolved, now the issue is reproducible only after multiple browser resize.
I filed bug 1305725 to cover the multiple resize issue from Comment 13.
Flags: needinfo?(roxana.leitan)
(Assignee)

Comment 16

a year ago
Other bug covers the remaining bug here, calling this fixed.
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Based on comment 15 and comment 16 I will mark this as Verified-Fixed
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.