1279695 - Search results in the MDN sidebar are wrongly positioned and move as you scroll

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Marco Castelluccio [:marco]]

For example, load https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error and search for "throw".

The result in the sidebar isn't positioned where it should be and it moves as you scroll.

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 4

[Marcia Knous [:marcia]]

Feature has been backed out so not tracking these bugs for 50.

Comment 5

[Andrew Overholt [:overholt]]

Bulk update to find bar bugs that won't be in 50 (according to https://bugzilla.mozilla.org/show_bug.cgi?id=1279695#c4 and local testing).

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1279695 - update the position of ranges that are in fixed or sticky positioned container nodes at each repaint or scroll.

Review commit: https://reviewboard.mozilla.org/r/73340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73340/

Comment 7

[Mike de Boer [:mikedeboer]]

This patch also changes the `_scheduleRepaintOfMask` semantics a bit to make `kModalHighlightRepaintFreqMs` _really_ the frequency of repaints of the mask and its contents.
Before, it reset the timer each time `scheduleRepaintOfMask` was called thus postponing the redraw until the timer could finish.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8783562 [details]
Bug 1279695 - update the position of ranges that are in fixed or sticky positioned container nodes at each repaint or scroll.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73340/diff/1-2/

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8783562 [details]
Bug 1279695 - update the position of ranges that are in fixed or sticky positioned container nodes at each repaint or scroll.

https://reviewboard.mozilla.org/r/73340/#review71144

::: toolkit/modules/FinderHighlighter.jsm:159
(Diff revision 2)
> -  this._modal = Services.prefs.getBoolPref(kModalHighlightPref);
>    this._highlightAll = Services.prefs.getBoolPref(kHighlightAllPref);
>    this._lastIteratorParams = null;
> +  this._modal = Services.prefs.getBoolPref(kModalHighlightPref);
> +  this._modalHighlightRectsMap = new Map();
> +  this._fixedRangesSet = new Set(); 

eslint will flag this as trailing whitespace

::: toolkit/modules/FinderHighlighter.jsm:664
(Diff revision 2)
> +      if (kFixed.has(window.getComputedStyle(node, null).position))
> +        return true;

This is a net improvement but I imagine we will have to do more with pages that use position:absolute and adjust the position of the element during scroll events (in other words, implementing their own position:sticky).

Comment 10

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fa1449a5a9c

Comment 11

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15cdb1e0834a

Comment 12

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/4988e9ed22f34ffa92723a16b0be9f82f48fe3e9
Bug 1279695 - update the position of ranges that are inside fixed, sticky positioned container nodes or frameset or iframe at each repaint or scroll. r=jaws

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4988e9ed22f3

Comment 14

[Marco Castelluccio [:marco]]

The bug is still reproducible.

Comment 15

[Mike de Boer [:mikedeboer]]

Small regression, I have a patch ready.

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Follow-up patch: always make sure to update the geometry of dynamic ranges

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8791618 [details] [diff] [review]
Follow-up patch: always make sure to update the geometry of dynamic ranges

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

Should this have been caught by one of the tests?

Comment 18

[Mike de Boer [:mikedeboer]]

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

No, that's quite hard to do. browser_FinderHighlighter.js would be the please, but I'd have to add a lot more instrumentation to analyze the flow between each find action. IOW, the AnonymousContent API is making our life a bit hard on this one.

Comment 19

[Mike de Boer [:mikedeboer]]

Bug 1279708 needed the same fix, but landed first.

Comment 20

[Mike de Boer [:mikedeboer]]

Fixed by https://hg.mozilla.org/mozilla-central/rev/945d91bffbdd

Comment 21

[Simona Badau, Desktop QA]

Attachment: scrolling.mp4

The search match in the MDN sidebar is still moving on the latest Nightly 52 (Build ID: 20160919065232) Please see the attached screen-cast.

Comment 22

[Marco Castelluccio [:marco]]

Given comment 18, there isn't a test for this.

Comment 23

[Mike de Boer [:mikedeboer]]

(In reply to Simona B [:simonab ] from comment #21)
> Created attachment 8792865 [details]
> scrolling.mp4
> 
> The search match in the MDN sidebar is still moving on the latest Nightly 52
> (Build ID: 20160919065232) Please see the attached screen-cast.

Well, this actually shows the fix! The content is still scrolling along, with a less than optimal framerate, but it doesn't scroll out of view anymore.
Strictly speaking, they still move, but merely to be repositioned correctly when the scrolling ends.

Comment 24

[Gerry Chang [:gchang]]

This feature will be disabled in 51.

Comment 25

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: increase the framerate to allow for quicker position updates

Well, this is all I can really do about this: update the frequency to update. I set it to 16, to approximate 60fps. But in reality we won't be able to get it that fast without a visual trail of the rectangles' previous positions.
That's the way it is and will always be, unless we change a different way to implement all this.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8794177 [details] [diff] [review]
Patch 2: increase the framerate to allow for quicker position updates

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

It looks like you uploaded the wrong patch here.

Also, we should only be increasing the framerate when we notice that position:sticky etc. are used.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: increase the framerate to allow for quicker position updates

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8794215 [details] [diff] [review]
Patch 2.1: increase the framerate to allow for quicker position updates

Carsten, can you land this on autoland for me? Thanks!

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/df63260c1c90
increase the framerate to allow for quicker position updates of the rectangles in the dimmed highlighting region of the findbar. r=jaws

Comment 30

[Carsten Book [:Tomcat]]

Comment on attachment 8794215 [details] [diff] [review]
Patch 2.1: increase the framerate to allow for quicker position updates

landed!

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df63260c1c90

Comment 32

[roxleitan]

Verified on the latest Nightly 52.0a1, build ID: 20161004030204, on Windows 10 x64 and the issue is still reproducible.

Comment 33

[Mike de Boer [:mikedeboer]]

I know that it's reproducible, but it should be much smoother now compared to before the last patch landed.
I can not eliminate the moving behavior completely, so if that's the desired end result of this bug, it'll be a WONTFIX.
Or another bug that we can keep to track this for the coming months/ years.

Comment 34

[roxleitan]

Verified on the latest Nightly 52.0a1 (Build ID: 20161010030204) on Windows 10, Ubuntu 14.04 and Mac OS X 10.10.
Indeed I can see an improvement.
Please mark this as resolved fixed.
Here is the new bug 1308187 that tracks this issue.

Comment 35

[roxleitan]

Based on Comment 34 I will mark this bug as Verified-Fixed.

Comment 36

[Jorg K (CEST = GMT+2)]

Sorry, I'm a little late with my complaint, but better late than never ;-)
I'm a Thunderbird developer and run a debug compile of TB all the time. As of late, I see a lot of this:
INFO -  JavaScript error: resource://gre/modules/FinderHighlighter.jsm, line 1165: TypeError: window is null

As you might know, TB runs Mozmill tests and there this error is the top-ranking error in our logs.

Do you think we can do something about it? You introduced that line here:
https://hg.mozilla.org/mozilla-central/rev/4988e9ed22f3#l2.792

Comment 37

[Mike de Boer [:mikedeboer]]

Perhaps it's about time I get a build of Thunderbird running as well... I'm taking a look for ya.
Regardless, I think it'd be good if you file a bug for TB and CC me in it?

Comment 38

[Jorg K (CEST = GMT+2)]

Thanks for getting back to me quickly. I filed bug 1319095. Doesn't this error show up in Firefox? I haven't done a FF compile for a while.

Comment 39

[Mike de Boer [:mikedeboer]]

(In reply to Jorg K (GMT+1) from comment #38)
> Doesn't this error show up in Firefox?

Nope.