Closed Bug 1302170 Opened 8 years ago Closed 8 years ago

Performance of dimming find animation is bad

Categories

(Toolkit :: Find Toolbar, defect)

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

Tracking

()

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

People

(Reporter: jrmuizel, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1302170 - use the newly added setCutoutRectsForElement API for AnonymousContent to optimize rectangle cutouts rendering speed when using findbar dimmed, modal highlighting mode.
58 bytes, text/x-review-board-request
jaws
: review+
mstange
: review+
Details 
The animation runs at a low enough frame rate that animating feels worse than having no animation at all. i.e. every time I see it screams firefox is slow at me.
Thanks for filing! The dimming of the page itself is not animated. What part do you perceive as slow? The only thing we do animate is the yellow highlight box when you navigate from occurrence to occurrence.
Flags: needinfo?(jmuizelaar)
Blocks: 1291278
(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Thanks for filing! The dimming of the page itself is not animated. What part
> do you perceive as slow? The only thing we do animate is the yellow
> highlight box when you navigate from occurrence to occurrence.

The animation of the highlight box is what is slow.
Flags: needinfo?(jmuizelaar)
There also seems to be substantial lag between typing and the highlight changing. i.e. As I type characters show up in the find bar but the highlight takes longer to update.
Here's the page that I was searching on:
https://www.reddit.com/r/rust/

It seems like some pages might be worse than others.
I see this too, searching for 'SimpleInputStream' on https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1288997&attachment=8790647 in the "All Files" view.  Typing into the input bar appears to update the first match found somewhat faster than other matches, and the letter-by-letter highlighting feels like a typewriter, rather than something smooth.

Navigating between occurrences is also slow.
Comment 5 is on x86-64 Linux, fwiw.
Perhaps the feature should be disabled until there's a solution to this?
Flags: needinfo?(mdeboer)
No.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> No.

Why not?  (I mean, users could disable the feature themselves, but that's not a very good solution--assuming the toggles are still there from the last time this code tried to land.)
(In reply to Nathan Froyd [:froydnj] from comment #9)
> Why not?  (I mean, users could disable the feature themselves, but that's
> not a very good solution--assuming the toggles are still there from the last
> time this code tried to land.)

Because I just re-enabled this past Friday and don't see an immediate reason to jank it again. Everything I do here costs cycles and I'd rather focus on letting users file bug reports while they're using the latest and greatest and I on getting as many fixed in the shortest amount of time possible.
Let's look at where we stand at the beginning of next week, right before the uplift.
*yank
Depends on: 1302551
Depends on: 1302522
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 52.1 - Oct 3
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
My bet is that this'll also fix bug 1302244.
Blocks: 1302244
Comment on attachment 8793321 [details]
Bug 1302170 - use the newly added setCutoutRectsForElement API for AnonymousContent to optimize rectangle cutouts rendering speed when using findbar dimmed, modal highlighting mode.

https://reviewboard.mozilla.org/r/80086/#review78820

This still removes and re-inserts the element when the cutout region changes. Please keep the element in there and just setCutoutRectsForElement repeatedly.
You don't need to do the geometryChanged check because Gecko knows that it does not need to repaint if the region hasn't changed.
Please use rgba(0, 0, 0, 0.35) instead of black+opacity.
Attachment #8793321 - Flags: review?(mstange) → review-
That said, this is already much better than what we have at the moment! Thanks!

I used this test page to check for invalidations: data:text/html,Test<div style=padding:400px;>Test
Set nglayout.debug.paint_flashing to true, and Cmd+G after searching for "test".
Comment on attachment 8793321 [details]
Bug 1302170 - use the newly added setCutoutRectsForElement API for AnonymousContent to optimize rectangle cutouts rendering speed when using findbar dimmed, modal highlighting mode.

https://reviewboard.mozilla.org/r/80086/#review78864

Perfect! Thanks.

::: toolkit/modules/FinderHighlighter.jsm:1034
(Diff revision 2)
>          if (dict.updateAllRanges)
>            rects = this._updateRangeRects(range);
>          if (this._checkOverlap(dict.currentFoundRange, range))
>            continue;
> -        for (let rect of rects) {
> -          maskContent.push(`<div xmlns="${kNSHTML}" style="${rectStyle}; top: ${rect.y}px;
> +        for (let rect of rects)
> +          allRects.push(new window.DOMRect(rect.x, rect.y, rect.width, rect.height));

Personally I'd put a "let DOMRect = window.DOMRect;" somewhere outside the loops, mostly because "new DOMRect" looks better. But that's up to you.
Attachment #8793321 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #20)
> Personally I'd put a "let DOMRect = window.DOMRect;" somewhere outside the
> loops, mostly because "new DOMRect" looks better. But that's up to you.

Thanks, will do. But moreover, thanks for the awesome work on the cutouts API!! I'm quite pleased with the result so far... now onward to zarro boogs!
Comment on attachment 8793321 [details]
Bug 1302170 - use the newly added setCutoutRectsForElement API for AnonymousContent to optimize rectangle cutouts rendering speed when using findbar dimmed, modal highlighting mode.

https://reviewboard.mozilla.org/r/80086/#review79096

::: toolkit/modules/tests/browser/browser_FinderHighlighter.js:322
(Diff revision 4)
>          dwu.loadSheetUsingURIString(uri, dwu.USER_SHEET);
>        } catch (e) {}
>      });
>  
>      let promise = promiseTestHighlighterOutput(browser, word, expectedResult, node => {
> +      dump("NODE..." + node.outerHTML + "\n");

looks like this should be removed before checking in
Attachment #8793321 - Flags: review?(jaws) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa0690208220
use the newly added setCutoutRectsForElement API for AnonymousContent to optimize rectangle cutouts rendering speed when using findbar dimmed, modal highlighting mode. r=jaws,mstange
https://hg.mozilla.org/mozilla-central/rev/aa0690208220
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
QA Contact: brindusa.tot
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161005030211

I can confirm the fix for all the duplicate bugs (I reproduced the initial issues on a Nightly build with the ID: 20160911030419). Verified on Windows 10, Ubuntu 16.04 and Mac OS X 10.11 using the latest Nightly 52 (Build ID: 20161005030211)

A slight delay from the moment a letter is added in the search input box and the moment the match is highlighted can still be seen. Filed Bug 1308214 to cover and track that issue. 

Setting the status of this bug to Verified fixed.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
Depends on: 1311663
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: