The default bug view has changed. See this FAQ.

Parameterize nsLayoutUtils::GetNearestScrollableFrame

RESOLVED FIXED in mozilla25

Status

()

Core
Layout: Misc Code
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: coyotebush, Assigned: coyotebush)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → cford
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
Created attachment 776844 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame.

Something like this.

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

Comment 2

4 years ago
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.
Attachment #776844 - Flags: review?(dbaron)
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-.
Attachment #776844 - Flags: review?(dbaron) → review-
(Assignee)

Comment 4

4 years ago
(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).
(Assignee)

Comment 5

4 years ago
Created attachment 782183 [details] [diff] [review]
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).
Attachment #782183 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #776844 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Created attachment 782677 [details] [diff] [review]
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...
Attachment #782677 - Flags: feedback?(dbaron)
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.)
Attachment #782677 - Flags: feedback?(dbaron) → feedback-
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
Attachment #782183 - Flags: review?(dbaron) → review+
(Assignee)

Comment 9

4 years ago
(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()))
(Assignee)

Comment 10

4 years ago
Created attachment 783523 [details] [diff] [review]
Parameterize nsLayoutUtils::GetNearestScrollableFrame.

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

Updated

4 years ago
Attachment #782183 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #782677 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #783523 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/710c7f7c8e4c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/710c7f7c8e4c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.