1401477 - Autoscroll icon gets resized (is small) when starting autoscroll close to the (available) screen edge

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P3)

Description

[u548634]

Attachment: Autoscroll-Icon Bug.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170919220202

Steps to reproduce:

Use Windows 10 and Firefox 57.0a1 (2017-09-19) (64-Bit)
Use autoscroll at the bottom of a maximized Firefox window


Actual results:

Icon is cut off at the top


Expected results:

Icon shouldn't be cut off at the top

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9fb9effa74a6&tochange=1bf0b04301ca

Regressed by: 95fc024ee46d	Gijs Kruitbosch — Bug 627974 - panels that aren't toplevel shouldn't overlap the taskbar,

Comment 2

[u548634]

Attachment: Autoscroll-Icon Bug 2.png

Interestingly, since the last two Nightlies, the autoscroll widget has been changed and now the bug behaves differently.

Comment 3

[u548634]

Behavior has changed again, now it just gets smaller.

Comment 4

[Jim Mathies [:jimm]]

Gijs, can you please prioritize this bug?

Comment 5

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] from comment #4)
> Gijs, can you please prioritize this bug?

Are you asking me to set a priority on this bug, or are you asking me to drop other stuff and work on this bug?

Comment 6

[Jim Mathies [:jimm]]

(In reply to :Gijs (slow, PTO recovery mode) from comment #5)
> (In reply to Jim Mathies [:jimm] from comment #4)
> > Gijs, can you please prioritize this bug?
> 
> Are you asking me to set a priority on this bug, or are you asking me to
> drop other stuff and work on this bug?

Just set the priority :)

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

Review commit: https://reviewboard.mozilla.org/r/199142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199142/

Comment 8

[:Gijs (he/him)]

Realistically this isn't very important (not a lot of people use autoscroll, and this only happens if you start autoscroll at the edge of the screen), but trying to figure out why I broke this in bug 627974 I realized fixing this was pretty trivial, so here's a patch that seems to WFM.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

https://reviewboard.mozilla.org/r/199142/#review204440

::: toolkit/content/widgets/browser.xml:1318
(Diff revision 1)
>              this._autoScrollPopup.removeAttribute("hidden");
>              this._autoScrollPopup.setAttribute("noautofocus", "true");
>              this._autoScrollPopup.setAttribute("scrolldir", scrolldir);
>              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> +            // Sanitize screenX/screenY for available screen size + size of the popup.
> +            let coordX = Math.min(window.screen.availWidth - 14, screenX);

The comment and magic number are confusing -- the popup is 28px wide and tall.

Comment 10

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #9)
> Comment on attachment 8927863 [details]
> Bug 1401477 - limit autoscroll popup coordinates so we always display it
> full-size, instead of having the popup code constrain its size to fit
> on-screen,
> 
> https://reviewboard.mozilla.org/r/199142/#review204440
> 
> ::: toolkit/content/widgets/browser.xml:1318
> (Diff revision 1)
> >              this._autoScrollPopup.removeAttribute("hidden");
> >              this._autoScrollPopup.setAttribute("noautofocus", "true");
> >              this._autoScrollPopup.setAttribute("scrolldir", scrolldir);
> >              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> > +            // Sanitize screenX/screenY for available screen size + size of the popup.
> > +            let coordX = Math.min(window.screen.availWidth - 14, screenX);
> 
> The comment and magic number are confusing -- the popup is 28px wide and
> tall.

It has a negative margin of 14px so it centers on the point that we pass here (which is normally the point at which the user clicks):

https://dxr.mozilla.org/mozilla-central/rev/fc194660762d1b92e1679d860a8bf41116d0f54f/toolkit/themes/linux/global/global.css#286

I guess I can make this explicit in the comment? Is that what you meant, or do you want something else?

Alternatively, I can put this stuff in a CSS variable, but then we'll need a style flush to read it in this code, which seems unfortunate. Or I guess we could put the sizing constants in the JS and set them from within this code... Please let me know what you would prefer (and/or if there's some other option I missed).

Comment 11

[Dão Gottwald [:dao]]

(In reply to :Gijs (slow, PTO recovery mode) from comment #10)
> Or I guess we
> could put the sizing constants in the JS and set them from within this
> code...

I hadn't considered this but it sounds like a reasonable option. Since the icon is SVG the stylesheets don't really care how big exactly the popup is.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199142/diff/1-2/

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

https://reviewboard.mozilla.org/r/199142/#review205494

::: toolkit/content/widgets/browser.xml:1318
(Diff revision 2)
>  
>              this._autoScrollPopup.removeAttribute("hidden");
>              this._autoScrollPopup.setAttribute("noautofocus", "true");
>              this._autoScrollPopup.setAttribute("scrolldir", scrolldir);
>              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> +            this._autoScrollPopup.style.setProperty("--autoscroller-size", POPUP_SIZE + "px");

Why not just set height, weight and margin directly on the popup?

::: toolkit/content/widgets/browser.xml:1322
(Diff revision 2)
>              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> +            this._autoScrollPopup.style.setProperty("--autoscroller-size", POPUP_SIZE + "px");
> +            // Sanitize screenX/screenY for available screen size with half the size
> +            // of the popup removed. The popup uses negative margins to center on the
> +            // coordinates we pass.
> +            let coordX = Math.min(window.screen.availWidth - 0.5 * POPUP_SIZE, screenX);

"coordX" doesn't seem like a meaningful distinction from "screenX". How about popupX?

::: toolkit/content/widgets/browser.xml:1322
(Diff revision 2)
>              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> +            this._autoScrollPopup.style.setProperty("--autoscroller-size", POPUP_SIZE + "px");
> +            // Sanitize screenX/screenY for available screen size with half the size
> +            // of the popup removed. The popup uses negative margins to center on the
> +            // coordinates we pass.
> +            let coordX = Math.min(window.screen.availWidth - 0.5 * POPUP_SIZE, screenX);

You probably need to take into account screen.availLeft as well?

::: toolkit/content/widgets/browser.xml:1323
(Diff revision 2)
> +            this._autoScrollPopup.style.setProperty("--autoscroller-size", POPUP_SIZE + "px");
> +            // Sanitize screenX/screenY for available screen size with half the size
> +            // of the popup removed. The popup uses negative margins to center on the
> +            // coordinates we pass.
> +            let coordX = Math.min(window.screen.availWidth - 0.5 * POPUP_SIZE, screenX);
> +            let coordY = Math.min(window.screen.availHeight - 0.5 * POPUP_SIZE, screenY);

ditto

::: toolkit/themes/windows/global/global.css:315
(Diff revision 2)
>    background-image: url("chrome://global/skin/icons/autoscroll.svg");
>    background-size: contain;
>    background-color: transparent;
>    background-repeat: no-repeat;
>    -moz-appearance: none;
>  }

Also need to update the mac and linux stylesheets.

Comment 14

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #13)
> ::: toolkit/content/widgets/browser.xml:1322
> (Diff revision 2)
> >              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> > +            this._autoScrollPopup.style.setProperty("--autoscroller-size", POPUP_SIZE + "px");
> > +            // Sanitize screenX/screenY for available screen size with half the size
> > +            // of the popup removed. The popup uses negative margins to center on the
> > +            // coordinates we pass.
> > +            let coordX = Math.min(window.screen.availWidth - 0.5 * POPUP_SIZE, screenX);
> 
> You probably need to take into account screen.availLeft as well?

It appears from searching the internet and MDN that this value may be unreliable in the face of multi-monitor setups. Are you in a position to test those?

Comment 15

[Dão Gottwald [:dao]]

(In reply to :Gijs (away until Nov 20) from comment #14)
> (In reply to Dão Gottwald [::dao] from comment #13)
> > ::: toolkit/content/widgets/browser.xml:1322
> > (Diff revision 2)
> > >              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> > > +            this._autoScrollPopup.style.setProperty("--autoscroller-size", POPUP_SIZE + "px");
> > > +            // Sanitize screenX/screenY for available screen size with half the size
> > > +            // of the popup removed. The popup uses negative margins to center on the
> > > +            // coordinates we pass.
> > > +            let coordX = Math.min(window.screen.availWidth - 0.5 * POPUP_SIZE, screenX);
> > 
> > You probably need to take into account screen.availLeft as well?
> 
> It appears from searching the internet and MDN that this value may be
> unreliable in the face of multi-monitor setups. Are you in a position to
> test those?

I have an external monitor but it isn't set up at the moment. I could look into that next week.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199142/diff/2-3/

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

https://reviewboard.mozilla.org/r/199142/#review206842

::: toolkit/content/widgets/browser.xml:1324
(Diff revision 3)
>              this._autoScrollPopup.setAttribute("noautofocus", "true");
>              this._autoScrollPopup.setAttribute("scrolldir", scrolldir);
>              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> +            this._autoScrollPopup.style.height = POPUP_SIZE + "px";
> +            this._autoScrollPopup.style.width = POPUP_SIZE + "px";
> +            this._autoScrollPopup.style.margin = -POPUP_SIZE / 2 + "px";

Please move this into the |if (!this._autoScrollPopup)| branch.

::: toolkit/content/widgets/browser.xml:1331
(Diff revision 3)
> +            // of the popup removed. The popup uses negative margins to center on the
> +            // coordinates we pass. Unfortunately `window.screen.availLeft` can be negative
> +            // on Windows in multi-monitor setups, so we use nsIScreenManager instead:
> +            let left = {}, top = {}, width = {}, height = {};
> +            screen.GetAvailRectDisplayPix(left, top, width, height);
> +            // On Windows, with 175% DPI, window.devicePixelRatio produces

This probably isn't Windows-specific.

::: toolkit/content/widgets/browser.xml:1333
(Diff revision 3)
> +            // on Windows in multi-monitor setups, so we use nsIScreenManager instead:
> +            let left = {}, top = {}, width = {}, height = {};
> +            screen.GetAvailRectDisplayPix(left, top, width, height);
> +            // On Windows, with 175% DPI, window.devicePixelRatio produces
> +            // 1.7647... because of rounding inside gecko. We need to match:
> +            const scaleFactor = 60 / Math.round(60 / screen.defaultCSSScaleFactor);

How is window.devicePixelRatio relevant here? Can you clarify?

Comment 18

[:Gijs (he/him)]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199142/diff/3-4/

Comment 19

[:Gijs (he/him)]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

https://reviewboard.mozilla.org/r/199142/#review206852

::: toolkit/content/widgets/browser.xml:1324
(Diff revision 3)
>              this._autoScrollPopup.setAttribute("noautofocus", "true");
>              this._autoScrollPopup.setAttribute("scrolldir", scrolldir);
>              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> +            this._autoScrollPopup.style.height = POPUP_SIZE + "px";
> +            this._autoScrollPopup.style.width = POPUP_SIZE + "px";
> +            this._autoScrollPopup.style.margin = -POPUP_SIZE / 2 + "px";

I figured I might as well move the `hidden` attribute removal and the noautofocus setting, as I don't see anything re-setting either. I kind of wanted to get rid of the `hidden` thing, but it's not clear there isn't a (very small) perf implication, and it's not related to this bug.

::: toolkit/content/widgets/browser.xml:1333
(Diff revision 3)
> +            // on Windows in multi-monitor setups, so we use nsIScreenManager instead:
> +            let left = {}, top = {}, width = {}, height = {};
> +            screen.GetAvailRectDisplayPix(left, top, width, height);
> +            // On Windows, with 175% DPI, window.devicePixelRatio produces
> +            // 1.7647... because of rounding inside gecko. We need to match:
> +            const scaleFactor = 60 / Math.round(60 / screen.defaultCSSScaleFactor);

I've updated the comment. Let me know if there's anything else you're missing.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

https://reviewboard.mozilla.org/r/199142/#review206862

::: toolkit/content/widgets/browser.xml:1325
(Diff revision 4)
>  
> -            this._autoScrollPopup.removeAttribute("hidden");
> -            this._autoScrollPopup.setAttribute("noautofocus", "true");
>              this._autoScrollPopup.setAttribute("scrolldir", scrolldir);
>              this._autoScrollPopup.addEventListener("popuphidden", this, true);
> +            // Sanitize screenX/screenY for available screen size with half the size

nit: please add a blank line before this comment

::: toolkit/content/widgets/browser.xml:1331
(Diff revision 4)
> +            // of the popup removed. The popup uses negative margins to center on the
> +            // coordinates we pass. Unfortunately `window.screen.availLeft` can be negative
> +            // on Windows in multi-monitor setups, so we use nsIScreenManager instead:
> +            let left = {}, top = {}, width = {}, height = {};
> +            screen.GetAvailRectDisplayPix(left, top, width, height);
> +            // We need to get screen CSS-pixel (rather than display-pixel) coordinates.

ditto

Comment 21

[:Gijs (he/him)]

Comment on attachment 8927863 [details]
Bug 1401477 - limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199142/diff/4-5/

Comment 22

[:Gijs (he/him)]

Updating this to fix the whitespace nits, but I also realized I needed to add half the popup size to the availLeft/availTop based values, just like I'm subtracting it from the maximum x/y values. Landing with all of that fixed. :-)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a77dc52424ef
limit autoscroll popup coordinates so we always display it full-size, instead of having the popup code constrain its size to fit on-screen, r=dao

Comment 24

[Cristina Coroiu [:ccoroiu]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Given the age of this bug, I'm considering Fx58 to be fix-optional, but is the risk low enough to be worth uplift consideration?

Comment 26

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> Given the age of this bug, I'm considering Fx58 to be fix-optional, but is
> the risk low enough to be worth uplift consideration?

Meh, all the multi-screen stuff was annoying enough that some extra baking won't hurt, and I don't think the severity of the bug warrants uplift, given it's cosmetic and trivial workarounds are available.