make nsLayoutUtils::GetNearestScrollableFrame never return a proper descendant of the passed in frame

RESOLVED FIXED in Firefox 45

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
If you are using nsLayoutUtils::GetNearestScrollableFrame to iterate up the frame tree, then returning a proper descendant would put you in an infinite loop.

This happens if you pass "always match root" and pass the root frame. It will return the root scroll frame, which is a child of the root frame.

This specific behaviour was added in http://hg.mozilla.org/mozilla-central/rev/cd8ecc2e1a2e (bug 1105823) so that fixed pos frames (which are children of the root frame, but not descendants of the root scroll frame) would find the root scroll frame APZC for their document. Otherwise they would find nothing, because there is no other scroll frame containing them in the document tree.

I propose to change this as follows: add a new flag that specifies this special behaviour for fixed pos frames, and use it. And then remove the quirk of GetNearestScrollableFrame with root frames.
(Assignee)

Comment 1

3 years ago
Created attachment 8675452 [details] [diff] [review]
Part 1. Only match root scroll frame if we encouter root scroll frame (not root frame).
Attachment #8675452 - Flags: review?(botond)
(Assignee)

Comment 2

3 years ago
Created attachment 8675454 [details] [diff] [review]
Part 2. Add a flag to make fixed pos return the root scroll frame.

As I was implementing this I realized we may want to do this for all documents, not just the root document. We can do that in a followup bug if you agree it is something we want.
Attachment #8675454 - Flags: review?(botond)
(Assignee)

Comment 3

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #0)
> I propose to change this as follows: add a new flag that specifies this
> special behaviour for fixed pos frames, and use it. And then remove the
> quirk of GetNearestScrollableFrame with root frames.

The patches do this in the reverse order.

Updated

3 years ago
Attachment #8675452 - Flags: review?(botond) → review+

Updated

3 years ago
Attachment #8675454 - Flags: review?(botond) → review+
(Assignee)

Comment 5

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #2)
> As I was implementing this I realized we may want to do this for all
> documents, not just the root document. We can do that in a followup bug if
> you agree it is something we want.

Did you have an opinion on whether we should do this for all documents? Not just the root document?
Flags: needinfo?(botond)
(In reply to Timothy Nikkel (:tn) from comment #5)
> (In reply to Timothy Nikkel (:tn) from comment #2)
> > As I was implementing this I realized we may want to do this for all
> > documents, not just the root document. We can do that in a followup bug if
> > you agree it is something we want.
> 
> Did you have an opinion on whether we should do this for all documents? Not
> just the root document?

I think that's a sensible change.
Flags: needinfo?(botond)
(Assignee)

Comment 8

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #2)
> As I was implementing this I realized we may want to do this for all
> documents, not just the root document. We can do that in a followup bug if
> you agree it is something we want.

This is bug 1221870.

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60ed9b57b7d4
https://hg.mozilla.org/mozilla-central/rev/91a6134ea200
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.