in nsLayoutUtils::GetNearestScrollableFrame don't skip the root scroll frame if we are asked to always match the root scroll frame even if it doesn't WantAsyncScroll() (and we are also asked to match only async scrollable frames)

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

(1 attachment)

(Assignee)

Description

3 years ago
APZ needs a scrollable frame otherwise events might be lost. So if we are asked to always match root and only match async scrollable we need to return the root scroll frame even if it isn't "WantAsyncScroll()" because otherwise we will return null since we have nothing else to return.

APZ is the only user of these two flags.
(Assignee)

Comment 1

3 years ago
Seems like this doesn't cause problems because PrepareForSetTargetAPZCNotification in APZCCallbackHelper (the only user) will fall back to the root element, and should almost always already have a displayport before PrepareForSetTargetAPZCNotification is called.
(Assignee)

Comment 2

3 years ago
Created attachment 8675447 [details] [diff] [review]
patch
Attachment #8675447 - Flags: review?(botond)
(Assignee)

Updated

3 years ago
Blocks: 1208780
Comment on attachment 8675447 [details] [diff] [review]
patch

Review of attachment 8675447 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +1909,5 @@
> +        if (!(aFlags & SCROLLABLE_ONLY_ASYNC_SCROLLABLE) ||
> +            scrollableFrame->WantAsyncScroll()) {
> +          return scrollableFrame;
> +        }
> +      }

I would find the following more readable:

if (aFlags & SCROLLABLE_ONLY_ASYNC_SCROLLABLE) {
  if (scrollableFrame->WantAsyncScroll()) {
    return scrollableFrame;
  }
} else {
  ScrollbarStyles ss = scrollableFrame->GetScrollbarStyles();
  if ((aFlags & SCROLLABLE_INCLUDE_HIDDEN) ||
      ss.mVertical != NS_STYLE_OVERFLOW_HIDDEN ||
      ss.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN) {
    return scrollableFrame;
  }
}
Attachment #8675447 - Flags: review?(botond) → review+
(Assignee)

Comment 5

3 years ago
(In reply to Botond Ballo [:botond] from comment #3)
> I would find the following more readable:

I agree, I changed it.

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/394230165a48
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.