1795661 - [macOS] The about:preferences#general page is scrolled up after closing a window inside

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Attachment: Screen Recording 2022-10-17 at 2.mov

Found in

• 107.0a1 (20221016093143)

Affected versions

• 107.0a1 (20221016093143)
• 106.0
• 105.0.3
• 102.4.0esr

Tested platforms

• Affected platforms: macOS 10.15, macOS 13
• Unaffected platforms: Ubuntu 20.04, Windows 10x64

Steps to reproduce

1. Open about:preferences#general page and scroll down to the Fonts area.
2. Click on the Advanced button and close the window.
3. Observe the scroll position.

Expected result

• The scroll position does not change.

Actual result

• The page is scrolled upwards.

Regression range

• Last good revision: f0cad1d9e20e16f6cacd1a3c01f52fdca226c5b5
  16:28.16 INFO: First bad revision: 67d5d9f8377572a37548555fe9c9a7d508c39514
  16:28.16 INFO: Pushlog:
  https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f0cad1d9e20e16f6cacd1a3c01f52fdca226c5b5&tochange=67d5d9f8377572a37548555fe9c9a7d508c39514
  Potential regressor: bug 1400420.

Additional notes

• Attached a screen recording of the issue.
• The issue reproduced only on about:preferences#general page and does not reproduce on other Preferences page menus like Privacy and Security.
• After opening another window inside about:preferecens#privacy menu like Exceptions the issue is no longer reprodcing inside the about:preferences#general page until restart.

Comment 1

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

:Gijs, since you are the author of the regressor, bug 1400420, could you take a look?

For more information, please visit auto_nag documentation.

Comment 2

[:Gijs (out for now; he/him)]

I don't understand why the dialog closing causes layout to move the scroll position by the amount of scroll padding on the pref pane. I can't see anything in JS doing this, but I'm probably missing something... Emilio, any ideas?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

The issue is that the scroll-padding makes us think that the search textbox is not visible (so we scroll up to show it).

There might be use cases for scroll-padding actually jumping here, and other browsers also jump...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1795661 - Clean-up ScrollFrameIntoView code. r=hiro

I want to do the whole target -> container chain walk in the same
function, reason will be apparent in a second :)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1795661 - Don't apply scroll padding on the sticky sides for a stuck element. r=hiro

Depends on D159531

Comment 6

[:Gijs (out for now; he/him)]

Thanks Emilio!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Another test case I tested locally

Comment 8

[Emilio Cobos Álvarez (:emilio)]

So my code doesn't fix comment 7, but there's a question of whether it should. As is, my code only prevents scroll-padding from applying on the side that we're stuck on. I think that's sensible since scroll-padding is intended to cover up for fixed/abspos/sticky headers.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/726f4fa7e25d
Clean-up ScrollFrameIntoView code. r=hiro

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0474fa9cabdc
Fix typo fixed in the following patch of the bug that causes orange.

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/726f4fa7e25d
https://hg.mozilla.org/mozilla-central/rev/0474fa9cabdc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f9a11cc3fcd
Don't apply scroll padding on the sticky sides for a stuck element. r=hiro

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36548 for changes under testing/web-platform/tests

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/2f9a11cc3fcd

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 16

[[:fabrice] Fabrice Desré]

This breaks (some) unified builds:

0:12.41 In file included from Unified_cpp_tools_profiler1.cpp:29:
 0:12.41 In file included from /media/external/dev/gecko-dev/tools/profiler/gecko/nsProfiler.cpp:27:
 0:12.41 In file included from /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/mozilla/dom/Promise.h:24:
 0:12.41 In file included from /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/mozilla/dom/ToJSValue.h:25:
 0:12.41 In file included from /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/mozilla/dom/BindingUtils.h:30:
 0:12.41 In file included from /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/mozilla/dom/Document.h:18:
 0:12.41 In file included from /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/Units.h:12:
 0:12.41 In file included from /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/mozilla/gfx/Coord.h:11:
 0:12.41 /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/mozilla/gfx/Types.h:1054:41: error: reference to 'Side' is ambiguous
 0:12.41 inline constexpr SideBits SideToSideBit(Side aSide) {
 0:12.41                                         ^
 0:12.41 /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/mozilla/gfx/Types.h:1033:6: note: candidate found by name lookup is 'mozilla::Side'
 0:12.41 enum Side : uint8_t { eSideTop, eSideRight, eSideBottom, eSideLeft };
 0:12.41      ^
 0:12.41 /media/external/dev/gecko-dev/obj-b2g-desktop/dist/include/mozilla/ipc/MessageLink.h:44:6: note: candidate found by name lookup is 'mozilla::ipc::Side'
 0:12.41 enum Side : uint8_t { ParentSide, ChildSide, UnknownSide };
 0:12.41      ^
 0:12.41 1 error generated.

This patch fixes it - feel free to steal it:

diff --git a/gfx/2d/Types.h b/gfx/2d/Types.h
index f2ace5e602277..fc3a910daa3c4 100644
--- a/gfx/2d/Types.h
+++ b/gfx/2d/Types.h
@@ -1051,7 +1051,7 @@ enum class SideBits {
 
 MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(SideBits)
 
-inline constexpr SideBits SideToSideBit(Side aSide) {
+inline constexpr SideBits SideToSideBit(mozilla::Side aSide) {
   return SideBits(1 << aSide);
 }

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddec1bdaa1e8
Apply patch in comment 16 as it's trivial. a=imoraru

Comment 18

[Serban Stanca [:SerbanS]]

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

Comment 19

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Hello! Verified the issue with Firefox 108.0a1 (2022-10-27) on macOS 10.15 and macOS 12. I about:preferences page is no longer scrolled after following steps from comment 0 or when switching focus on the reduced test case from comment 3.

However, the page is scrolled down when switching focus on the test case from comment 7. From comment 8 I understand that this is the expected behavior.
Emilio, just to be extra safe here is it ok that when switching focus on the test case from comment 7 the page is scrolled down? Thank you in advance!

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Yes, that's expected for now at least, pending other discussion. Thanks for checking!

Comment 21

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Thank you, Emilio! Closing as verified per the above comments.