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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Timothy Nikkel (:tnikkel)]

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.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Only match root scroll frame if we encouter root scroll frame (not root frame).

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: 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.

Comment 3

[Timothy Nikkel (:tnikkel)]

(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.

Comment 4

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8507e88cd18b

Comment 5

[Timothy Nikkel (:tnikkel)]

(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?

Comment 6

[Botond Ballo [: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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60ed9b57b7d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a6134ea200

Comment 8

[Timothy Nikkel (:tnikkel)]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60ed9b57b7d4
https://hg.mozilla.org/mozilla-central/rev/91a6134ea200