Closed Bug 894716 Opened 6 years ago Closed 6 years ago

Parameterize nsLayoutUtils::GetNearestScrollableFrame

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: coyotebush, Assigned: coyotebush)

Details

Attachments

(1 file, 3 obsolete files)

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: nobody → cford
Status: NEW → ASSIGNED
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-
(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).
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)
Attachment #776844 - Attachment is obsolete: true
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+
(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()))
Done. And realized I had removed an if statement from the logic of the original.
Attachment #782183 - Attachment is obsolete: true
Attachment #782677 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #783523 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/710c7f7c8e4c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.