1724088 - Don't notify ScrollbarActivity in scroll frames without scrollbars

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, task, P2)

Description

[Markus Stange [:mstange]]

This seems to be one of the cases of bug 1722583: We call ActivityOccurred even on scroll frames without scrollbars.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1724088 - Don't notify ScrollbarActivity in scroll frames without scrollbars. r=tnikkel

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5e9bf4c7e67
Don't notify ScrollbarActivity in scroll frames without scrollbars. r=tnikkel

Comment 3

[Iulian Moraru]

Backed out for causing reftest failures on iframe-modify-scrolling-attr-to-yes.html.

Push with failures

Failure log for wr3
Failure log for R2

Backout link

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mstange, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 5

[Markus Stange [:mstange]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #4)

There's a r+ patch which didn't land

It did land, but it was backed out.

Comment 6

[Markus Stange [:mstange]]

There are two Android-only test failures. That makes sense because Android is the only platform where we test the overlay scrollbar configuration.
The test failures are in layout/reftests/scrolling/iframe-scrolling-attr-2.html and in iframe-modify-scrolling-attr-to-yes.html. In both tests, the difference is that the reference has visible scrollbars but the test does not.

My guess is that in the past, we were showing scrollbars in both the test and the reference. And somehow the patch makes us not show the scrollbars in the test anymore. I don't have an idea yet for why that would happen.

I tried to reproduce one of the test failures on macOS with overlay scrollbars enabled, but the test was passing. I don't remember which of the two tests I tried.

The next steps would be to: Try both tests again on macOS; maybe debug the good state to get an idea for which NotifyActivity call is needed and why it might be missing in the bad state; set up an Android testing and debugging environment if all else fails.

Comment 7

[Markus Stange [:mstange]]

Hi Botond, I think this bug is somewhat important to fix but I don't think I'll have time for a deeper investigation. Do you think someone from the APZ team could take it?

Comment 8

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #7)

I think this bug is somewhat important to fix

Could you say a bit more about what the user-noticeable effects of the bug are? (Based on bug 1722583, it looks like performance? Is it noticeable though?)

Comment 9

[Markus Stange [:mstange]]

It directly affects power usage to some degree because we're ticking the refresh driver unnecessarily (which comes with CPU wakeups and CSS restyle costs, example profile). Fixing this also has an indirect benefit of making it easier to find other stray refresh observers, because the profiler marker chart won't be cluttered by the scrollbar ones. This would help the power usage effort.

Comment 10

[Botond Ballo [:botond]]

Discussed this in today's APZ planning meeting. We're going to try and look at it as part of our current "stabilization sprint".

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Note that I just tried to run the reftests locally with D121784 on local Android emulator, the test passed properly. I pushed a new try run to see if it still fails on try. https://treeherder.mozilla.org/jobs?repo=try&revision=3b1eedb8637fe7d807f97c1e7d62dc98b242b60c

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I am idiot. Now I can see the failure locally. An excuse is that I have a local cloned repo for Android build but last night moz-phab broke the repo for some reasons so I had to re-clone the repo locally and forgot restoring .mozconfig there. :/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I did track down what the difference between desktops with ui.useOverlayScrollbars=1 and Android is. The difference is this if (gfxPlatform::UseDesktopZoomingScrollbars()) {} block in ScrollFrameHelper::ReflowFinished.

On Android apz.allow_zoomiing pref is disabled both on wpt reftest and our in-tree reftest, thus the block doesn't run through on Android, so the scroll range for iframes in question is wrongly computed in the first place in the case of scrolling=no iframes. Thus when the scrolling attribute is changed to "yes", we unfortunately bail out from this aElement->AttrValueIs() check in ScrollFrameHelper::SetCoordAttribute, thus ScrollbarActivity->ActivityOccurred() wont't get called.

A couple of comments I can tell;

1. D121784 revealed a pre-existing issue in ScrollFrameHelper::ReflowFinished() with disabling desktop zooming, you can easily see same failures on desktop with ui.useOverlayScrollbars=1 and apz.allow_zooming=false
2. Given that desktop zooming has been enabled for a while by default, the test failures are not a big problem
3. We should take some amount of time to make our in-tree reftests and wpt reftests pass with apz.allow_zooming=true since it's by default

I think we can land D121784 with explicit apz.allow_zooming=true for the reftests, but I'd defer the final decision to Markus or Botond.

Comment 14

[Botond Ballo [:botond]]

Thanks for investigating!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)

2. Given that desktop zooming has been enabled for a while by default, the test failures are not a big problem

Agreed.

3. We should take some amount of time to make our in-tree reftests and wpt reftests pass with apz.allow_zooming=true since it's by default

Yep, I think that's a good idea. We have bug 1650080 on file for our reftests.

I think we can land D121784 with explicit apz.allow_zooming=true for the reftests, but I'd defer the final decision to Markus or Botond.

+1

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I did upload the new changeset with the pref setting.

I've also pushed a try run to see if it's okay; https://treeherder.mozilla.org/jobs?repo=try&revision=0f6d26c02c233f8858e5e4cb65b74679cfb875a4, there are two unknown failures, but they are not related to the changeset, I did double-check in another try without the change; https://treeherder.mozilla.org/jobs?repo=try&revision=3976f5310bb1e315a5ddab8c2ee8613278579b03, there are also two failures.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9682c9f452e6
Don't notify ScrollbarActivity in scroll frames without scrollbars. r=tnikkel

Comment 17

[Sandor Molnar[:smolnar]]

Backed out changeset 9682c9f452e6 (bug 1724088) for causing android wr failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/82563b380afb5b8952e044a0f0f5d29a1585aa89

Push with failures

Failure log WR2

Failure log WR6

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Looks like I did push the try run based on a wrong revision. :/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Sandor Molnar from comment #17)

Failure log WR2

This one, grid-container-auto-margins-scrollbars-001.html is actually another test case we need to specify apz.allow_zooming pref.

Failure log WR6

But, this one isn't. This one looks odd, it's scaled as if the minimum-scale viewport is applied.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)

(In reply to Sandor Molnar from comment #17)

Failure log WR2

This one, grid-container-auto-margins-scrollbars-001.html is actually another test case we need to specify apz.allow_zooming pref.

Failure log WR6

But, this one isn't. This one looks odd, it's scaled as if the minimum-scale viewport is applied.

Filed bug 1755598 for this issue. There seems to be an issue in our wpt runner that pref values once set in a .ini file persists.

That said, given that we'd eventually want to enable apz..allow_zooming all over the place (bug 1650080). The test should be passed with apz.allow_zooming=true.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)

Failure log WR6

But, this one isn't. This one looks odd, it's scaled as if the minimum-scale viewport is applied.

Filed bug 1755598 for this issue. There seems to be an issue in our wpt runner that pref values once set in a .ini file persists.

That said, given that we'd eventually want to enable apz..allow_zooming all over the place (bug 1650080). The test should be passed with apz.allow_zooming=true.

Filed bug 1755600.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

I've pushed a new try run to make sure there's no failure caused by this change; https://treeherder.mozilla.org/jobs?repo=try&revision=cabd6a2afc46df847e7de4254d0a7fda2f496645. There are two failures, but I believe both are known, bug 1607713 and bug 1712012. I'd hope there's no more overlooked failures.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80dea4887372
Don't notify ScrollbarActivity in scroll frames without scrollbars. r=tnikkel

Comment 24

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/80dea4887372