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

: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?

status-firefox72: fix-optional → wontfix

Flags: needinfo?(surkov.alexander)

Alice0775 White

Reporter

Updated

•

4 years ago

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

Robin Steuber (they/them) [:bytesized]

Comment 3

•

4 years ago

•

Edited

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.

alexander :surkov (:asurkov)

Comment 4

•

4 years ago

(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)

Robin Steuber (they/them) [:bytesized]

Comment 5

•

4 years ago

(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>.

:Gijs (he/him)

Comment 6

•

4 years ago

•

Edited

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)

alexander :surkov (:asurkov)

Comment 7

•

4 years ago

(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.

Alice0775 White

Reporter

Comment 8

•

4 years ago

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

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

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 9

•

4 years ago

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)

:Gijs (he/him)

Comment 10

•

4 years ago

(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)

alexander :surkov (:asurkov)

Comment 11

•

4 years ago

(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?

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 12

•

4 years ago

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

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 13

•

4 years ago

(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.

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 14

•

4 years ago

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)

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 15

•

4 years ago

Here's such a patch, fwiw: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4c8e82315f806b937be7e3a83ac2341c8a11cfd

Emilio Cobos Álvarez (:emilio)

Assignee

•

4 years ago

leave-open for the "better" fix.

Keywords: leave-open

Pulsebot

Comment 20

•

4 years ago

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

Cristina Coroiu [:ccoroiu]

Comment 21

•

4 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/0aa32e6d6d58

:Gijs (he/him)

Comment 22

•

4 years ago

P1 given this has an assignee.

Priority: -- → P1

:Gijs (he/him)

Comment 23

•

4 years ago

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...

status-firefox73: affected → fixed

Pulsebot

Comment 24

•

4 years ago

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

Tim Nguyen :ntim

Updated

•

Comment 27

•

4 years ago

s/7/8

Flags: needinfo?(emilio)

Phabricator Automation

Updated

•

4 years ago

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

Pulsebot

Comment 28

•

4 years ago

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

Dorel Luca [:dluca]

Comment 29

•

4 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 4 years ago

status-firefox74: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 74

Clara Guerrero ( Need Info Brindusa Tot Please)

Comment 30

•

4 years ago

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

•

4 years ago

Regressions: 1611636

Bug 1606130 - Make display: -moz-box more similar to other display types for block layout. r=jfkthame,surkov 4 years ago Emilio Cobos Álvarez (:emilio) 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1606130 - Make vertical alignment not mess with our <body>. r=Gijs 4 years ago Emilio Cobos Álvarez (:emilio) 47 bytes, text/x-phabricator-request		Details \| Review