Closed Bug 1331693 Opened 3 years ago Closed 3 years ago

APZ scrollbar dragging breaks devtools property list scrolling

Categories

(Core :: Panning and Zooming, defect, P3)

53 Branch
Unspecified
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 + disabled
firefox54 --- verified

People

(Reporter: cpeterson, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

[Tracking Requested - why for this release]:

Kats, this devtools bug is a regression in Nightly 53 from APZ scrollbar bug 1324117.

STR:
1. Open devtools console pane.
2. Type "navigator.".
3. When the navigator object's property name list opens up, try to click and drag the list's scrollbar.

RESULT:
Nothing happens. The scrollbar can't be dragged. Clicking in the scrollbar area above or below the scrollbar thumb will correctly scroll the list.
Flags: needinfo?(bugmail)
I am running Windows 10.
OS: Unspecified → Windows
Component: Developer Tools → Panning and Zooming
Product: Firefox → Core
Flags: needinfo?(bugmail) → needinfo?(botond)
Version: unspecified → 53 Branch
Priority: -- → P3
Whiteboard: [gfx-noted]
Tracking 53+ for this devtools regression.
I can reproduce this. Will look into it.
Assignee: nobody → botond
Flags: needinfo?(botond)
The scroll frame in question generates a scroll info layer because it's inside SVG effects.

Async scrollbar dragging doesn't work for scroll info layers, because it relies on getting metrics about the scrollbar from the layer tree, which requires the scrollbar to be layerized. The scrollbar is only layerized is the scroll frame itself is layerized, not if it just generates a scroll info layer.

However, the conditions for the main thread handing off the scrollbar drag to APZ don't check for the scroll info layer case, so we also don't take the main-thread dragging path, resulting in no dragging at all.

Since scroll info layers aren't visually async-scrolled anyways, there's not much point trying to get async scrollbar dragging to work for them, so we should just enter the main-thread dragging path for them.
There's one thing I don't love about this patch: if we ever change the conditions under which a scroll info layer is created, we have to remember to make a corresponding change here as well. However, I don't have any good ideas about how to put the condition into a single place, given that the actual decision is made during display-list building, while here we explicitly want to know whether a scroll frame will build a scroll info layer before display-list building takes place.
Comment on attachment 8828161 [details]
Bug 1331693 - Do not attempt async scrollbar dragging for scroll info layers.

https://reviewboard.mozilla.org/r/105646/#review106592

::: layout/xul/nsSliderFrame.cpp:986
(Diff revision 1)
> +  nsIFrame* current = aScrollFrame;
> +  while (current) {
> +    if (UsesSVGEffects(current)) {
> +      return true;
> +    }
> +    current = nsLayoutUtils::GetParentOrPlaceholderFor(current);

We'll need to look in all parent documents, so use GetParentOrPlaceholderForCrossDoc.
Attachment #8828161 - Flags: review?(tnikkel) → review+
(In reply to Botond Ballo [:botond] from comment #6)
> There's one thing I don't love about this patch: if we ever change the
> conditions under which a scroll info layer is created, we have to remember
> to make a corresponding change here as well. However, I don't have any good
> ideas about how to put the condition into a single place, given that the
> actual decision is made during display-list building, while here we
> explicitly want to know whether a scroll frame will build a scroll info
> layer before display-list building takes place.

What we've done for other cases like this is put a comment in the code in all the locations that need to be kept in sync saying "this needs to be kept in sync with ...".
Addressed review comments.
Duplicate of this bug: 1331481
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aec005f46692
Do not attempt async scrollbar dragging for scroll info layers. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/aec005f46692
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Async scrollbar dragging is currently Nightly-only, so marking as firefox-53:disabled.
I verified this issue on FF Beta 54.0b5 with Windows 10 x32, Mac OS X 10.10 and Ubuntu 16.04 and I can't reproduce it anymore. I will mark this as a verified fix.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.