1661283 - Overflow badge not shown for Document node

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Brad Werth [:bradwerth]]

Attachment: overflow-document.html

Steps to Reproduce:

1. Ensure the pref "devtools.overflow.debugging.enabled" is true.
2. Load the attached overflow-document.html file. Note that the page has a vertical scrollbar.
3. Open the devtools Inspector and select the body tag.

Expected Results:
The body tag should have a "scroll" badge.

Comment 1

[Brad Werth [:bradwerth]]

The fix will likely involve a change to InspectorUtils::GetOverflowingChildrenOfElement.

Comment 2

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #1)

The fix will likely involve a change to InspectorUtils::GetOverflowingChildrenOfElement.

Nope, the problem is that NodeActor.initialize for a HTMLBodyElement checks isScrollable and decides that the body is not scrollable, even if a scrollbar is present. I'm now checking DocumentWalker to see why it fails in this case.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1661283 Part 1: Add a ChromeOnly method to Element for getting scrollbar visibility.

This will be used to support the display of "scroll" badges in devtools.

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1661283 Part 2: Make InspectorUtils::GetOverflowingChildrenOfElement correctly handle the body element.

This requires using the same Element::GetScrollFrame logic used in the new
Element::HasVisibleScrollbars method. This also makes GetScrollFrame public,
so that InspectorUtils can use it.

Depends on D88504

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1661283 Part 3: Make NodeActor.isScrollable use the new Element.hasVisibleScrollbars method.

This removes the need for the document/dom walk.

Depends on D88505

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1661283 Part 4: Make NodeActor.initialize respect overflow debugging pref.

This also reuses a cached result of isScrollable, to avoid a second call.

Depends on D88506

Comment 7

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #2)

(In reply to Brad Werth [:bradwerth] from comment #1)

The fix will likely involve a change to InspectorUtils::GetOverflowingChildrenOfElement.

Nope, the problem is that NodeActor.initialize for a HTMLBodyElement checks isScrollable and decides that the body is not scrollable, even if a scrollbar is present. I'm now checking DocumentWalker to see why it fails in this case.

Both the isScrollable logic and the GetOverflowingChildrenOfElement needed an update to handle the body element case. This has the pleasant effect of eliminating the possibly-slow DocumentWalker call in isScrollable.

Comment 8

[Brad Werth [:bradwerth]]

Attachment: Bug 1661283 Part 5: Add a test of GetOverflowingChildrenOfElement on the body element.

Depends on D88507

Comment 9

[Brad Werth [:bradwerth]]

Attachment: Bug 1661283 Part 6: Add a test of NodeActor.isScrollable on various elements.

Depends on D88682

Comment 10

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d8f75cf9c9052da28b70cae9e3f270b04c9ce27

Comment 11

[Brad Werth [:bradwerth]]

Attachment: Bug 1661283 Part 7: Update TestActor reflow function, and await reflow in an intermittent test.

Depends on D88683

Comment 12

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4ab43f1464ab0980b456b84abe3d30bd2b14aaa

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/986ef17f0fe4
Part 1: Add a ChromeOnly method to Element for getting scrollbar visibility. r=emilio
https://hg.mozilla.org/integration/autoland/rev/87c6bbff43c5
Part 2: Make InspectorUtils::GetOverflowingChildrenOfElement correctly handle the body element. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8b32a0860985
Part 3: Make NodeActor.isScrollable use the new Element.hasVisibleScrollbars method. r=gl
https://hg.mozilla.org/integration/autoland/rev/236068f7e724
Part 4: Make NodeActor.initialize respect overflow debugging pref. r=gl
https://hg.mozilla.org/integration/autoland/rev/62d68911ba94
Part 5: Add a test of GetOverflowingChildrenOfElement on the body element. r=gl
https://hg.mozilla.org/integration/autoland/rev/2d040d38476f
Part 6: Add a test of NodeActor.isScrollable on various elements. r=gl

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/986ef17f0fe4
https://hg.mozilla.org/mozilla-central/rev/87c6bbff43c5
https://hg.mozilla.org/mozilla-central/rev/8b32a0860985
https://hg.mozilla.org/mozilla-central/rev/236068f7e724
https://hg.mozilla.org/mozilla-central/rev/62d68911ba94
https://hg.mozilla.org/mozilla-central/rev/2d040d38476f