894716 - Parameterize nsLayoutUtils::GetNearestScrollableFrame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Corey Ford [:coyotebush]]

nsLayoutUtils::GetNearestScrollableFrame is used by nsPresShell.cpp and AnimationCommon.cpp. It "locates the first ancestor of aFrame (or aFrame itself) that is scrollable with overflow:scroll or overflow:auto in some direction. The search extends across document boundaries."

nsFrame::Handle{Press,Drag} each have a similar loop to find the nearest scrollable ancestor, except that they don't cross document boundaries and don't exclude overflow:hidden elements.

The latter behavior is probably what we want for bug 886646, and I think a good approach might be to add flag parameters to GetNearestScrollableFrame to toggle those two constraints.

Comment 1

[Corey Ford [:coyotebush]]

Attachment: Parameterize nsLayoutUtils::GetNearestScrollableFrame.

Something like this.

https://tbpl.mozilla.org/?tree=Try&rev=2522d6aa9275

Comment 2

[Corey Ford [:coyotebush]]

Comment on attachment 776844 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame.

I might as well actually think about landing this. I'd consider inverting the sense of either of the booleans; it's a little weird that the two current use cases happen to be (true, false) and (false, true), and while they're both positively worded, one lets the search continue longer while the other lets it stop sooner.

And yes, it turned a bit messy to get the nsIFrame* version back out in HandlePress.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 776844 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame.

So rather than using sequences of boolean parameters (which made the calling code unreadable), we tend to use bitfields.  For an example, see nsLayoutUtils::PaintFrame.

I think I'd be inclined to invert the sense of the hidden bit -- make it a bit for skipping hidden.

I'd also prefer keeping the for() structure rather than switching to while() -- just use ?: inside the last part of the for statement.

And avoid default parameters unless there's a good reason for them.

Otherwise, I think this sounds good, though, but I'd like to see a patch with those changes, so marking as review-.

Comment 4

[Corey Ford [:coyotebush]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3)
> I think I'd be inclined to invert the sense of the hidden bit -- make it a
> bit for skipping hidden.

Sounds fine, although I realize that if we instead go with SAME_DOC and INCLUDE_HIDDEN flags, the default behavior is then identical to the original (and to GetNearestScrollableFrameForDirection).

Comment 5

[Corey Ford [:coyotebush]]

Attachment: Parameterize nsLayoutUtils::GetNearestScrollableFrame (v2, flags)

So if we were to use CROSS_DOC and SKIP_HIDDEN flags, I dislike the resulting asymmetry of the calls in nsIPresShell::GetFrameToScrollAsScrollable.

Another approach might be to unify this method with GetNearestScrollableFrameForDirection (whose only caller is nsIPresShell::GetFrameToScrollAsScrollable). The resulting logic would then resemble their ancestor GetNearestScrollingView (see bug 526394, part 5).

Comment 6

[Corey Ford [:coyotebush]]

Attachment: Unify nsLayoutUtils::GetNearestScrollableFrame{,ForDirection} with extra parameters

That approach would look like this. (still would need to figure out where best to declare/include the enum, and that enormous if condition could maybe be formulated better).

Feel free to just r+ the other patch, though, and we'll go with that...

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 782677 [details] [diff] [review]
Unify nsLayoutUtils::GetNearestScrollableFrame{,ForDirection} with extra parameters

I agree with you about the asymmetry in nsIPresShell::GetFrameToScrollAsScrollable, but I think a cleaner way to fix it, instead of adding an additional parameter to nsLayoutUtils::GetNearestScrollableFrame, would be to add two new flag bits to represent the two direction requirements in GetNearestScrollableFrameForDirection.  Except I also think the check it's doing is actually substantially different -- it's checking scrollbar visibility, which is something that's seems to me odd to check.  I'm inclined to suggest leaving the functions separate, and just going with cross-doc by default, then.  (Though I'm curious how much of the difference in terms of checking GetPerceivedScrollingDirections was really intentional.)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 782183 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame (v2, flags)

>+    nsIPresShell::SetCapturingContent(((nsIFrame*)do_QueryFrame(scrollFrame))->
>+                                      GetContent(), CAPTURE_IGNOREALLOWED);

Put the nsIFrame* in a variable (no cast) rather than doing this cast (and a C-style cast, ugh) inline.

r=dbaron with that

Comment 9

[Corey Ford [:coyotebush]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7)
> (Though I'm curious how much of the difference
> in terms of checking GetPerceivedScrollingDirections was really intentional.)

Incidentally, GetNearestScrollingView had logic like
 (aDirection == eEither || totalHeight > visibleSize.height || margin.LeftRight()))

Comment 10

[Corey Ford [:coyotebush]]

Attachment: Parameterize nsLayoutUtils::GetNearestScrollableFrame.

Done. And realized I had removed an if statement from the logic of the original.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/710c7f7c8e4c

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/710c7f7c8e4c