Closed Bug 1401477 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: XUL Widgets, defect, P3)

29 Branch
Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: Adrian.Tiefenbrunner, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

Attached image 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
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,
Blocks: 627974
Status: UNCONFIRMED → NEW
Component: Untriaged → XUL Widgets
Ever confirmed: true
Keywords: regression
OS: Unspecified → Windows
Product: Firefox → Toolkit
Version: 57 Branch → 29 Branch
Interestingly, since the last two Nightlies, the autoscroll widget has been changed and now the bug behaves differently.
Behavior has changed again, now it just gets smaller.
Gijs, can you please prioritize this bug?
Flags: needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jmathies)
(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 :)
Flags: needinfo?(jmathies)
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: -- → P3
Summary: Autoscroll icon cut off → Autoscroll icon gets resized (is small) when starting autoscroll close to the (available) screen edge
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.
Attachment #8927863 - Flags: review?(dao+bmo)
(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).
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
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.
Attachment #8927863 - Flags: review?(dao+bmo) → review-
(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?
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
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?
Attachment #8927863 - Flags: review?(dao+bmo)
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 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
Attachment #8927863 - Flags: review?(dao+bmo) → review+
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. :-)
Pushed by gijskruitbosch@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/a77dc52424ef
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1420241
You need to log in before you can comment on or make changes to this bug.