Distracting delayed highlighting of previous match when jumping between matches / Keep the current find match highlighted under the outline

VERIFIED FIXED in Firefox 52

Status

()

Toolkit
Find Toolbar
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: mstange, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox52 verified)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a year ago
STR:
 1. Open the findbar on this bug and search for the word "luminous".
 2. Watch how the first match is highlighted in yellow, and
    the second match of the word "luminous" is highlighted in white / transparent.
 3. Press Enter to jump to the next match.

Actual results:
When jumping to the next match, the yellow highlight animates and causes the user's eyes to focus on the right spot. However, at this point the first instance is still dimmed. Then, a fraction of a second later, the first match suddenly becomes undimmed. This drags the user's attention back to it.

Expected results:
The current match shouldn't be dimmed in the first place.
It's being covered by the yellow highlight anyway.
Yeah, this is a dupe I think but nonetheless annoying! The result of the patch in bug 1300824.
Blocks: 1300824
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 52.2 - Oct 17
Points: --- → 1
Flags: qe-verify+
Flags: firefox-backlog+
Comment hidden (mozreview-request)
(Reporter)

Comment 3

a year ago
Does this avoid regressing bug 1300824?

I suppose the other solution would be to synchronously force an update of the highlight map before moving the outline.
No, it basically re-opens it. But that bug requires a more comprehensive fix. In another patch I indeed merged updating the outline and setting the background cutouts into the same 'repaint' method, to simplify things a bit better and that's what it has been doing in practice anyways.
When that lands I can also flip setting the cutouts and updating the outline around to get the correct behavior. I guess I could reuse bug 1300824 for that.

Comment 5

a year ago
mozreview-review
Comment on attachment 8797997 [details]
Bug 1304497 - always draw all rectangles, because the showing and hiding of overlapping rectangles during find occurrence navigation is jarring. This essentially backs out bug 1300824.

https://reviewboard.mozilla.org/r/83598/#review83178

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> No, it basically re-opens it. But that bug requires a more comprehensive
> fix. In another patch I indeed merged updating the outline and setting the
> background cutouts into the same 'repaint' method, to simplify things a bit
> better and that's what it has been doing in practice anyways.
> When that lands I can also flip setting the cutouts and updating the outline
> around to get the correct behavior. I guess I could reuse bug 1300824 for
> that.

Why would we backout the patch in that bug without replacing it with the more comprehensive fix? Landing this patch to re-open that bug with the hopes of doing a more comprehensive fix in the future just seems like wasted work for us and QA. We should just get the better approach implemented and land that to fix this bug and bug 1300824 better.
Attachment #8797997 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Why would we backout the patch in that bug without replacing it with the
> more comprehensive fix? Landing this patch to re-open that bug with the
> hopes of doing a more comprehensive fix in the future just seems like wasted
> work for us and QA. We should just get the better approach implemented and
> land that to fix this bug and bug 1300824 better.

I've spent a while on a more comprehensive fix for bug 1300824, but that's just not possible; there will always be a timing issue between repainting the yellow outline and applying the cutouts.
I still think that this bug has a bigger visual impact, compared to bug 1300824, so I'm in favor of backing that patch out.
Flags: needinfo?(jaws)
Okay, we can back it out. We might have gotten faster at applying the styles since that bug was filed, and we can look for other ways to speed this up too.
Flags: needinfo?(jaws)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=763ed9fbd8b2
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
Comment on attachment 8797997 [details]
Bug 1304497 - always draw all rectangles, because the showing and hiding of overlapping rectangles during find occurrence navigation is jarring. This essentially backs out bug 1300824.

https://reviewboard.mozilla.org/r/83598/#review89790

::: toolkit/modules/FinderHighlighter.jsm:666
(Diff revisions 1 - 2)
>    _getRangeContentArray(range) {
>      let content = range.cloneContents();
>      let t, textContent = [];
>      for (let node of content.childNodes) {
>        t = node.textContent || node.nodeValue;
> -      //if (t && t.trim())
> +      // if (t && t.trim())

Please delete this dead code.
Attachment #8797997 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)

Comment 12

a year ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e98ff6fdd64
always draw all rectangles, because the showing and hiding of overlapping rectangles during find occurrence navigation is jarring. This essentially backs out bug 1300824. r=jaws

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e98ff6fdd64
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Verified fixed using the latest Nightly 52.0a1 (Build ID: 20161109030210) on Windows 10, Ubuntu 16.04 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.