Closed Bug 1606130 Opened 2 years ago Closed 2 years ago

Auto scroll marker is displayed when sub dialog is closed with middle mouse button clicking

Categories

(Firefox :: Preferences, defect, P1)

Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 74
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 --- wontfix
firefox73 --- verified
firefox74 --- verified

People

(Reporter: alice0775, Assigned: emilio)

References

(Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(2 files)

Reproducible: always

Steps To Reproduce:

  1. Open Preferences (about:preferences)
  2. Open sub-dialog. e.g. Click [Advanced...] button of about:preferences
  3. Close the sub-dialog with middle mouse button clicking

Actual Results:
The sub-dialog is closed as expected.
However, Auto scroll maker is displayed.

Expected Results:
Auto scroll maker should not be displayed.

Has Regression Range: --- → yes
Has STR: --- → yes

:surkov, did we except XUL documents from autoscroll somehow? And/or do you have other ideas why this would have regressed when switching to a <body> tag?

Flags: needinfo?(surkov.alexander)
Summary: Auto scroll maker is displayed when sub dialog is closed with middle mouse button clicking → Auto scroll marker is displayed when sub dialog is closed with middle mouse button clicking

I can reproduce this only by middle clicking outside the bounds of the dialog, not by middle clicking the X button. I am guessing that this what was intended by the STR, but I thought I'd mention it since the description is a little vague on that point.

I chased this bug around a bit, but I've tracked it to a point where I believe that the answer involves understanding a bit more XUL magic than I understand. The difference in behavior essentially occurs at the location described by this stack (most recent call listed last):
AutoScrollChild.startScroll:158
AutoScrollChild.findNearestScrollableElement:144
AutoScrollChild.computeWindowScrollDirection:83

At this point, it is trying to determine the scroll direction for the subdialog overlay (which sits between the preferences page and the dialog, created here), since that was the target of the middle click event. Prior to the introduction of this bug, this if conditional evaluated to false (i.e. global.scrollMaxY == global.scrollMinY), resulting in null being returned for the scroll direction, and auto scroll not being activated. This makes sense as the dialog overlay is intended to fill the screen exactly and thus should not be scrollable in any direction. Now, however, the if conditional is evaluating to true (i.e. global.scrollMaxY != global.scrollMinY), indicating that the dialog overlay is too big and thus can be scrolled.

Unfortunately, this is where I got stuck. The styles applied to the dialog overlay seem to be identical in both cases. I wasn't able to turn up any clues in the inspector. But this difficulty seems fairly consistent with my past experiences trying to debug style differences in XUL documents. It seems like XUL styling has some quirks in the Firefox inspector, which I don't really understand. So I'm hoping that someone else can help figure out why the dialog overlay sizing has changed.

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #3)

At this point, it is trying to determine the scroll direction for the subdialog overlay (which sits between the preferences page and the dialog, created here), since that was the target of the middle click event. Prior to the introduction of this bug, this if conditional evaluated to false (i.e. global.scrollMaxY == global.scrollMinY), resulting in null being returned for the scroll direction,

I'm curious if global.scrollMaxY returned any valid value at all.

and auto scroll not being activated. This makes sense as the dialog overlay is intended to exactly fill the screen exactly and thus should not be scrollable in any direction.

yeah, but those are not real dialogs, so the usual window-based code flow is not applied here I think.

I can reproduce this only by middle clicking outside the bounds of the dialog, not by middle clicking the X button. I am guessing that this what was intended by the STR, but I thought I'd mention it since the description is a little vague on that point.
Unfortunately, this is where I got stuck.

I'd say AutoScroll should know about dialogs, and stop walking the chain if a dialog element is encountered [1]

[1] https://searchfox.org/mozilla-central/source/toolkit/actors/AutoScrollChild.jsm#127

Flags: needinfo?(surkov.alexander)

(In reply to alexander :surkov (:asurkov) from comment #4)

I'm curious if global.scrollMaxY returned any valid value at all.

IIRC I checked and it returned 3. I don't really know what kind of values are expected or what units are implied here, so I don't really know how unexpected/invalid that value is.

I'd say AutoScroll should know about dialogs, and stop walking the chain if a dialog element is encountered [1]

Maybe I'm misunderstanding, but I don't think it ever encounters the <dialog> element. The handleEvent call that calls startScroll directly passes it an event where event.originalTarget is the dialog overlay. The overlay is a <vbox> that is part of the preferences page. It isn't even part of the same document as the <dialog>.

All the elements here are getting an offset of about 3 css pixels on my machine (ie getBoundingClientRect() returns an object with top/y -3.3333282470702125), which is what's leading to the scrollable container (window.scrollMaxY is 3), which doesn't actually exist - and of course gets removed when we clicking outside the dialog closes the dialog.

A simple workaround is to make the dialog's overlay call preventDefault() on the mousedown event. But I'm confused where these 3px are coming from - as far as I can tell it's happening to the entire stack of elements from #dialogOverlay-0 up to body and html - the latter two having margin/padding: 0 and height/width: 100%. So I don't understand how they can be 3 pixels off to the top of the document.

Emilio, can you enlighten us?

Flags: needinfo?(emilio)

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #5)

(In reply to alexander :surkov (:asurkov) from comment #4)

I'm curious if global.scrollMaxY returned any valid value at all.

IIRC I checked and it returned 3. I don't really know what kind of values are expected or what units are implied here, so I don't really know how unexpected/invalid that value is.

I guessed that scrollMaxY was not implemented in XUL windows, what would all explain, but I was wrong.

I'd say AutoScroll should know about dialogs, and stop walking the chain if a dialog element is encountered [1]

Maybe I'm misunderstanding, but I don't think it ever encounters the <dialog> element. The handleEvent call that calls startScroll directly passes it an event where event.originalTarget is the dialog overlay. The overlay is a <vbox> that is part of the preferences page. It isn't even part of the same document as the <dialog>.

You're right, there's no real dialogs here, those dialogs can be probably detected via @role='dialog' though if needed, but the dialog is not in direct hierarchy of the main scrollable content, so this approach won't work here. Luckily I think Gijs has a point and a workaround.

This is something related. Autoscroll marker will display, and scroll few pixels
STR

  1. Open about:preferences
  2. Middle mouse button click on side menu pane(This pane should not be scrollable)
    --- Autoscroll marker will display --- BUG
  3. Move mouse up and down
    --- The content area scrolls up and down a few pixels. --- BUG

If you just leave the <html> and <body> elements you still have some scroll range. This is related to the body being a -moz-box, if it becomes a block then it works as expected.

This is more than likely a xul layout bug.

Is there any chance to do something like:

#preferences-body { display: block }
#preferences-stack > hbox { max-height: 100vh }

That seems to achieve the desired result here. Though more generally moving preferences away from XUL layout would be great.

Relatedly, should auto-scroll even allow scrolling elements with overflow: hidden? Seems it shouldn't.

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

If you just leave the <html> and <body> elements you still have some scroll range. This is related to the body being a -moz-box, if it becomes a block then it works as expected.

This is more than likely a xul layout bug.

I'm still confused how "even" a XUL box would end up being positioned 3px above the viewport given the CSS involved. Given we've used it for 20 years it can't be that buggy.

Is there any chance to do something like:

#preferences-body { display: block }
#preferences-stack > hbox { max-height: 100vh }

That seems to achieve the desired result here. Though more generally moving preferences away from XUL layout would be great.

I want this too, but I'm already reeling from having to deal with numerous (often not universally reproducible) regressions (either in behaviour or performance) in components I'm triaging as a result of XUL layout and fluent refactors, with mixtures of backouts on beta and patches on nightly that then cause their own regressions. Trying to convert more of the prefs here while in soft freeze doesn't strike me as a sustainable solution - we're already playing whack-a-mole with regressions from fixes to address regressions from these refactors.

Are there any safer alternatives?

Relatedly, should auto-scroll even allow scrolling elements with overflow: hidden? Seems it shouldn't.

I suspect with the way autoscroll is implemented today (ie in frontend + JS), it's not necessarily easy to tell. I'm also not sure if there aren't webpages that abuse overflow hidden with custom scroll bars + handling to do their own scrolling, so I'd prefer not to touch that for now.

Flags: needinfo?(emilio)

(In reply to :Gijs (he/him) from comment #10)

Relatedly, should auto-scroll even allow scrolling elements with overflow: hidden? Seems it shouldn't.

I suspect with the way autoscroll is implemented today (ie in frontend + JS), it's not necessarily easy to tell. I'm also not sure if there aren't webpages that abuse overflow hidden with custom scroll bars + handling to do their own scrolling, so I'd prefer not to touch that for now.

I think Emilio meant to use overflow:hidden in about:preferences since overflow:hidden indeed prevents autoscrolling:

data:text/html,<div style='width: 100%; max-height: 90vh; background: blue;overflow:hidden'><div style='max-width: 50%; height: 2000px; background:red'></div></div>

which should be safe option?

My point is that about:preferences does have overflow:hidden on the root element already.

(In reply to :Gijs (he/him) from comment #10)

I'm still confused how "even" a XUL box would end up being positioned 3px above the viewport given the CSS involved. Given we've used it for 20 years it can't be that buggy.

Well, let me disagree... I think this is more about the interaction between XUL and block layout, but this shows scrollbars (you need layout.css.xul-box-display-values.content.enabled):

<!doctype html>
<style>
  :root, body {
    height: 100%;
    width: 100%;
    margin: 0;
    padding: 0;
  }
  body {
    display: -moz-box;
  }
</style>
<body></body>

Anyhow given the reduced test-case is so minimal, I may as well take a look. But not excited about digging again in the XUL layout code. Last time I did I was horrified.

So what's going on is that -moz-box is not considered a "block", so vertical alignment applies to it: https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/servo/components/style/values/specified/box.rs#224

This is long-standing behavior... We could try changing it I guess, though not sure what might blow up...

I'll give it a quick try, but otherwise alternative is just #prefsBody { vertical-align: top } or such.

Flags: needinfo?(emilio)

Some minor fixes and...

It is unexpected (see bug) that a -moz-box is affected by baseline alignment.
Make -moz-box be block-outside, and -moz-inline-box be inline-outside, instead
of the bespoke thing we have now.

This is more similar to everything else, and fixes the bug.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

Safe CSS hack for uplifting or what not.

leave-open for the "better" fix.

Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0aa32e6d6d58
Make vertical alignment not mess with our <body>. r=Gijs

P1 given this has an assignee.

Priority: -- → P1

Note for posterity: the hacky fix here made 73. I guess the flags are going to get messy with the two patches land in different releases...

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23d862866f3a
Make display: -moz-box more similar to other display types for block layout. r=jfkthame,surkov
Blocks: 1607465
Keywords: leave-open
Regressions: 1607557

Backed out changeset 23d862866f3a (Bug 1606130) for android crashtest complaining about 1547420-1.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=1a8dc8ffd7374ca12ad36d92abc0bca3e06c9eaa&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Creftests%2Ctest-android-em-7.0-x86_64%2Fdebug-geckoview-crashtest-e10s%2Cr%28c%29&tochange=6fddcfe8a22858273ad65a0ec4d28f359e5bae17&selectedJob=283887154

Backout link: https://hg.mozilla.org/integration/autoland/rev/6fddcfe8a22858273ad65a0ec4d28f359e5bae17

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283887154&repo=autoland&lineNumber=13217

[task 2020-01-07T20:44:16.123Z] 20:44:16 INFO - REFTEST TEST-START | http://10.0.2.2:8854/tests/layout/painting/crashtests/1547420-1.html
[task 2020-01-07T20:44:16.124Z] 20:44:16 INFO - REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/painting/crashtests/1547420-1.html | 2831 / 3750 (75%)
[task 2020-01-07T20:44:16.125Z] 20:44:16 INFO - REFTEST TEST-PASS | http://10.0.2.2:8854/tests/layout/painting/crashtests/1547420-1.html | (LOAD ONLY)
[task 2020-01-07T20:44:16.126Z] 20:44:16 INFO - REFTEST TEST-END | http://10.0.2.2:8854/tests/layout/painting/crashtests/1547420-1.html
[task 2020-01-07T20:44:16.126Z] 20:44:16 WARNING - REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/painting/crashtests/1547420-1.html | assertion count 8 is more than expected 4 to 7 assertions
[task 2020-01-07T20:44:16.126Z] 20:44:16 INFO - REFTEST TEST-START | http://10.0.2.2:8854/tests/layout/painting/crashtests/1549909.html
[task 2020-01-07T20:44:16.126Z] 20:44:16 INFO - REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/painting/crashtests/1549909.html | 2832 / 3750 (75%)
[task 2020-01-07T20:44:16.127Z] 20:44:16 INFO - REFTEST TEST-PASS | http://10.0.2.2:8854/tests/layout/painting/crashtests/1549909.html | (LOAD ONLY)
[task 2020-01-07T20:44:16.128Z] 20:44:16 INFO - REFTEST TEST-END | http://10.0.2.2:8854/tests/layout/painting/crashtests/1549909.html

Flags: needinfo?(emilio)

s/7/8

Flags: needinfo?(emilio)
Attachment #9118727 - Attachment description: Bug 1606130 - Make display: -moz-box more similar to other display types for block layout. r=jfkthame → Bug 1606130 - Make display: -moz-box more similar to other display types for block layout. r=jfkthame,surkov
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be0ddae5d705
Make display: -moz-box more similar to other display types for block layout. r=jfkthame,surkov
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74

Hi,
I just checked the last nightly 74.0a1 (2020-01-08) (64-bit) and Beta 73.015 (64-bit) builds and I was able to verify the fix in both, now when opening submenu within about:preferences section, clicking middle mouse button won't trigger the autoscroll marker (release version does, due to being marked as won't fix)
Thanks, I've updated the flags for firefox73 and firefox74.
Best,
Clara.

Status: RESOLVED → VERIFIED
Version: 72 Branch → Trunk
Regressions: 1607943
Regressions: 1607944
Regressions: 1608159
Regressions: 1608802
Regressions: 1608829
Regressions: 1609420
Regressions: 1608765
Regressions: 1611636
You need to log in before you can comment on or make changes to this bug.