Closed Bug 1279695 Opened 5 years ago Closed 4 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

50 Branch
defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
mozilla51
Iteration:
51.2 - Aug 29
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- disabled
firefox52 --- verified

People

(Reporter: marco, Assigned: mikedeboer)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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.
Duplicate of this bug: 1279708
[Tracking Requested - why for this release]: Regression
Version: Trunk → 50 Branch
Duplicate of this bug: 1280755
Feature has been backed out so not tracking these bugs for 50.
Blocks: 1291278
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).
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Points: --- → 3
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 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).
Attachment #8783562 - Flags: review?(jaws) → review+
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
https://hg.mozilla.org/mozilla-central/rev/4988e9ed22f3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1296822
No longer blocks: 1296822
Depends on: 1298438
The bug is still reproducible.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Small regression, I have a patch ready.
Status: REOPENED → ASSIGNED
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?
Attachment #8791618 - Flags: review?(jaws) → review+
(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.
Bug 1279708 needed the same fix, but landed first.
Depends on: 1279708
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Attached video 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Given comment 18, there isn't a test for this.
Flags: in-testsuite+
(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.
This feature will be disabled in 51.
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.
Attachment #8794177 - Flags: review?(jaws)
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.
Attachment #8794177 - Flags: review?(jaws) → review-
Blocks: 1305057
Attachment #8794215 - Flags: review?(jaws) → review+
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!
Attachment #8794215 - Flags: checkin?(cbook)
Pushed by cbook@mozilla.com:
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 on attachment 8794215 [details] [diff] [review]
Patch 2.1: increase the framerate to allow for quicker position updates

landed!
Attachment #8794215 - Flags: checkin?(cbook) → checkin+
https://hg.mozilla.org/mozilla-central/rev/df63260c1c90
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Verified on the latest Nightly 52.0a1, build ID: 20161004030204, on Windows 10 x64 and the issue is still reproducible.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Based on Comment 34 I will mark this bug as Verified-Fixed.
Status: RESOLVED → VERIFIED
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
Flags: needinfo?(mdeboer)
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?
Flags: needinfo?(mdeboer) → needinfo?(jorgk)
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.
Flags: needinfo?(jorgk)
(In reply to Jorg K (GMT+1) from comment #38)
> Doesn't this error show up in Firefox?

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