Closed Bug 1795661 Opened 2 years ago Closed 1 year ago

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

Categories

(Core :: Layout, defect, P3)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox-esr102 --- wontfix
firefox106 --- wontfix
firefox107 --- wontfix
firefox108 --- verified

People

(Reporter: atrif, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files)

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

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.
Has STR: --- → yes

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(gijskruitbosch+bugs)

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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(emilio)
Priority: -- → P3
Attached file 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...

Flags: needinfo?(emilio)

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

Assignee: nobody → emilio
Status: NEW → ASSIGNED

Thanks Emilio!

Component: Preferences → Layout
OS: macOS → All
Product: Firefox → Core

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.

Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/726f4fa7e25d
Clean-up ScrollFrameIntoView code. r=hiro
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/0474fa9cabdc
Fix typo fixed in the following patch of the bug that causes orange.
Attachment #9298902 - Attachment description: Bug 1795661 - Don't apply scroll padding on stuck element. r=hiro → Bug 1795661 - Don't apply scroll padding on the sticky sides for a stuck element. r=hiro
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f9a11cc3fcd
Don't apply scroll padding on the sticky sides for a stuck element. r=hiro
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/36548 for changes under testing/web-platform/tests
Upstream PR merged by moz-wptsync-bot

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);
 }
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/ddec1bdaa1e8
Apply patch in comment 16 as it's trivial. a=imoraru
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED

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!

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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

Status: RESOLVED → VERIFIED
Blocks: 1820205
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: