Closed Bug 1663537 Opened 4 years ago Closed 4 years ago

fix tests to handle desktop zooming scrollbars

Tracking

()

Status:

RESOLVED FIXED

Milestone:

82 Branch

Tracking Flags:

Tracking

Status

firefox82

---

fixed

firefox83

---

fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1663537. Fix layout/base/tests/test_scroll_event_ordering.html to work with desktop zooming scrollbars. r?kats 4 years ago Timothy Nikkel (:tnikkel) 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1663537. Fix various tests for desktop zooming scrollbars. r?kats 4 years ago Timothy Nikkel (:tnikkel) 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1663537. Use overlay scrollbars in a few reftests. r?kats 4 years ago Timothy Nikkel (:tnikkel) 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1663537. Use scrollbar-width: none on some reftests that are not trying to test scrollbars. r?kats 4 years ago Timothy Nikkel (:tnikkel) 47 bytes, text/x-phabricator-request		Details \| Review

Timothy Nikkel (:tnikkel)

Assignee

Description

•

4 years ago

No description provided.

Timothy Nikkel (:tnikkel)

Assignee

Comment 1

•

4 years ago

Attached file Bug 1663537. Fix layout/base/tests/test_scroll_event_ordering.html to work with desktop zooming scrollbars. r?kats — Details

sendKey uses our new relative scroll path that does the scroll by the apz. But we don't have any scroll event ordering expectations when apz is doing the scroll. We trade lossing those expectations for smoother apz scrolling.

So change the test to use a method that doesn't use our new relative apz scroll path and goes back to main thread scrolling.

Timothy Nikkel (:tnikkel)

Assignee

Comment 2

•

4 years ago

Attached file Bug 1663537. Fix various tests for desktop zooming scrollbars. r?kats — Details

The test fixes all fell into the follow categories:

A) The test uses requestAnimationFrame to wait one frame and expects scrolling to be complete. With the desktop zooming scrollbars in order for the scrolling to show up on the main thread we need to send the scroll request to the compositor and then hear back from it via an apz repaint request (apz callback helper). Waiting on requestAnimationFrame will complete the first part, but not necessarily the second part. The fix is to wait for a scroll event.

B) Switching tests to wait for scroll events exposes another problem: the test can do things that cause a scroll in order to setup the test (and that may not be obvious that it causes a scroll) before actually proceeding to do the test and do something that causes a scroll and then checks for the scroll change of the second thing. Waiting for a requestAnimationFrame would include both those scrolls without desktop zooming scrollbars, but if we wait for a scroll event we will get the scroll event for the first thing which we are not interested in. So we need to make sure scroll events are cleared out before waiting for any scroll events. We do this by waiting two requestAnimationFrame's. We also use waiting for two requestAnimationFrame's when a test does something and it wants to test that scrolling is not performed.

The main that that causes scrolling that may not be obvious: calling node.focus(). With stacks like:

from test_scroll_per_page.html

#01: mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d6cc0]
#02: mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, mozilla::ScrollOrigin, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d7732]
#03: mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4742913]
#04: mozilla::PresShell::FlushPendingScrollAnchorAdjustments() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4650069]
#05: mozilla::PresShell::ProcessReflowCommands(bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x465742b]
#06: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4656af8]
#07: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1a87d3c]
#08: mozilla::PresShell::ScrollContentIntoView(nsIContent*, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4652b96]
#09: nsFocusManager::ScrollIntoView(mozilla::PresShell*, nsIContent*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1bedd1c]
#10: nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, bool, nsIContent*) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be6be0]
#11: nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be212f]
#12: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be32ba]
#13: mozilla::dom::Element::Focus(mozilla::dom::FocusOptions const&, mozilla::dom::CallerType, mozilla::ErrorResult&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1aaf283]
#14: mozilla::dom::HTMLElement_Binding::focus(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x2d65f3b]

from editor/libeditor/tests/test_bug549262.html

#01: mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d6cc0]
#02: mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, mozilla::ScrollOrigin, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d7732]
#03: mozilla::PresShell::ScrollFrameRectIntoView(nsIFrame*, nsRect const&, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x46541bc]
#04: mozilla::PresShell::DoScrollContentIntoView() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4653776]
#05: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4656b11]
#06: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1a87d3c]
#07: mozilla::PresShell::ScrollContentIntoView(nsIContent*, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4652b96]
#08: nsFocusManager::ScrollIntoView(mozilla::PresShell*, nsIContent*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1bedd1c]
#09: nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, bool, nsIContent*) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be6be0]
#10: nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be212f]
#11: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be32ba]
#12: mozilla::dom::Element::Focus(mozilla::dom::FocusOptions const&, mozilla::dom::CallerType, mozilla::ErrorResult&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1aaf283]
#13: mozilla::dom::HTMLElement_Binding::focus(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x2d65f3b]

C) Several tests use nsIDOMWindowUtils advanceTimeAndRefresh/restoreNormalRefresh and expect scrolling to be done after a call to advanceTimeAndRefresh. This is basically A), advanceTimeAndRefresh does a refresh driver tick but doesn't allow a repaint request to come back to the main thread.

Timothy Nikkel (:tnikkel)

Assignee

Comment 3

•

4 years ago

Attached file Bug 1663537. Use overlay scrollbars in a few reftests. r?kats — Details

All of these reftests end up using a minimum scale with layout/classic scrollbars. (They hit the assert from the patch in bug 1663534.)

Some of them are only written with overlay scrollbars in mind (for example overflow-hidden-region-with-negative-left-positioned-element.html which I looked at in detail).

The change that causes them to fail is the code in nsHTMLScrollFrame::TryLayout that decides if we need scrollbars. Before desktop zooming scrollbars we compared the visual viewport size and the scrolled rect size. With desktop zooming scrollbars we compare the (layout) scrollport and the scrolled rect to determine if we need regular scrollbars and then compare the visual viewport size to the (layout) scrollport to determine if we need scrollbars to scroll the visual viewport inside the scrollport. Then can get different results.

Timothy Nikkel (:tnikkel)

Assignee

Updated

•

4 years ago

Depends on: 1663532

Timothy Nikkel (:tnikkel)

Assignee

Updated

•

4 years ago

Depends on: 1663562

Pulsebot

Comment 4

•

4 years ago

Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/397ccd5f5991
Fix layout/base/tests/test_scroll_event_ordering.html to work with desktop zooming scrollbars. r=kats

Pulsebot

Comment 5

•

4 years ago

Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95fc865fc8cf
Use overlay scrollbars in a few reftests. r=kats

Narcis Beleuzu [:NarcisB]

Comment 6

•

4 years ago

Backed out for reftest failures on font-inflation-1.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/54db58fc6eb4cd74bbcdb89cab54609e7d01fa2e
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315072817&repo=autoland&lineNumber=28080

Flags: needinfo?(tnikkel)

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Updated

•

4 years ago

Regressions: 1663625

Atila Butkovits

Comment 7

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/397ccd5f5991

Status: NEW → RESOLVED

Closed: 4 years ago

status-firefox82: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 82 Branch

Timothy Nikkel (:tnikkel)

Assignee

Updated

•

4 years ago

Status: RESOLVED → REOPENED

Flags: needinfo?(tnikkel)

Resolution: FIXED → ---

BugBot [:suhaib / :marco/ :calixte]

Updated

•

4 years ago

status-firefox82: fixed → affected

Timothy Nikkel (:tnikkel)

Assignee

Updated

•

4 years ago

Keywords: leave-open

Timothy Nikkel (:tnikkel)

Assignee

Comment 8

•

4 years ago

(In reply to Narcis Beleuzu [:NarcisB] from comment #6)

Backed out for reftest failures on font-inflation-1.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/54db58fc6eb4cd74bbcdb89cab54609e7d01fa2e
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315072817&repo=autoland&lineNumber=28080

It's a failure in layout/reftests/mathml/font-inflation-1.html where it looks like we reserve space for a horizontal scrollbar but don't draw it. This failure does not happen when I enable the new desktop zooming scrollbars, and handling scrollbars correctly in these unusual situations (where the visual viewport, the scrollport, and the scrolled rect have relationships that we don't usually see because of min scale) is exactly one of the things the new desktop zooming scrollbar code is meant to fix. So I think I'll just add the overlay scrollbars pref to this one reftest when I flip the pref to enable the new desktop zooming scrollbars.

Pulsebot

Comment 9

•

4 years ago

Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/657830ceac83
Use overlay scrollbars in a few reftests. r=kats

Dorel Luca [:dluca]

Comment 10

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/657830ceac83

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Updated

•

4 years ago

Blocks: 1663534

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Updated

•

4 years ago

Regressions: 1664064

Pulsebot

Comment 11

•

4 years ago

Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97bb7fd483e0
Backed out changeset 657830ceac83 for causing Bug 1664064. CLOSED TREE

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 12

•

4 years ago

Backout merged: https://hg.mozilla.org/mozilla-central/rev/97bb7fd483e0

Pulsebot

Comment 13

•

4 years ago

Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30cd33572b3d
Fix various tests for desktop zooming scrollbars. r=kats

Timothy Nikkel (:tnikkel)

Assignee

Updated

•

4 years ago

Depends on: 1664419

Bogdan Tara[:bogdan_tara | bogdant]

Comment 14

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/30cd33572b3d

Ryan VanderMeulen [:RyanVM]

Updated

•

4 years ago

Status: REOPENED → ASSIGNED

Target Milestone: 82 Branch → ---

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Updated

•

4 years ago

Regressions: 1664446

Timothy Nikkel (:tnikkel)

Assignee

Comment 15

•

4 years ago

Attached file Bug 1663537. Use scrollbar-width: none on some reftests that are not trying to test scrollbars. r?kats — Details

These tests only failed on android, probably because the visual viewport size is different so we get a different result from the scrollbar calculation.

These tests seem to have an inconsistent mix of overflow: hidden and scrollbar-width: none. The desktop zooming scrollbars sometimes create scrollbars for overflow hidden now, so overflow hidden isn't enough, we need scrollbar-width: none.

layout/reftests/transform/compound-1-fail.html is the only file modified here that doesn't have overflow hidden or scrollbar-width: none already. Looking at the test it does not seem to be wanting to be anti-ref because of scrollbars (the transformed item looks different), so this seems to be an improvement (ie we won't pass because only the scrollbar differed).

Pulsebot

Comment 16

•

4 years ago

Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/872db70a1122
Use scrollbar-width: none on some reftests that are not trying to test scrollbars. r=botond

Natalia Csoregi [:nataliaCs]

Comment 17

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/872db70a1122

Pulsebot

Comment 18

•

4 years ago

Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de1e92b1db17
Use overlay scrollbars in a few reftests. r=kats

Noemi Erli[:noemi_erli]

Comment 19

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/de1e92b1db17

Timothy Nikkel (:tnikkel)

Assignee

Comment 20

•

4 years ago

Let's call this fixed. If any further changes are needed they can get landed in new bugs.

Status: ASSIGNED → RESOLVED

Closed: 4 years ago → 4 years ago

status-firefox82: affected → fixed

status-firefox83: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → 82 Branch

BugBot [:suhaib / :marco/ :calixte]

Updated

•

4 years ago

Keywords: leave-open

You need to log in before you can comment on or make changes to this bug.