Closed
Bug 1401477
Opened 7 years ago
Closed 7 years ago
Autoscroll icon gets resized (is small) when starting autoscroll close to the (available) screen edge
Categories
(Toolkit :: XUL Widgets, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: adrian.tiefenbrunner+moz, Assigned: Gijs)
References
Details
(Keywords: regression)
Attachments
(3 files)
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•7 years ago
|
||
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
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox57:
--- → affected
status-firefox-esr52:
--- → wontfix
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.
Comment 4•7 years ago
|
||
Gijs, can you please prioritize this bug?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 5•7 years ago
|
||
(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)
Comment 6•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Summary: Autoscroll icon cut off → Autoscroll icon gets resized (is small) when starting autoscroll close to the (available) screen edge
Comment 9•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 10•7 years ago
|
||
(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)
Comment 11•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 14•7 years ago
|
||
(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)
Comment 15•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
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•7 years ago
|
||
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
Comment 24•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 25•7 years ago
|
||
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?
status-firefox58:
--- → fix-optional
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 26•7 years ago
|
||
(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)
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•