Skip to content
This repository has been archived by the owner on Dec 3, 2020. It is now read-only.

[Linux] A blank space is displayed on the right side of the door hanger after pinning the Price Wise toolbar button in the Overflow Menu #256

Closed
SoftVision-CarmenFat opened this issue Nov 8, 2018 · 5 comments
Closed

[Linux] A blank space is displayed on the right side of the door hanger after pinning the Price Wise toolbar button in the Overflow Menu #256

SoftVision-CarmenFat opened this issue Nov 8, 2018 · 5 comments
Assignees
@biancadanforth
Labels
[QA]:Normal issue Label for QA to mark normal issues logged [QA]:Verified fixed Label for QA to mark verified fixed issues
Milestone
Post-Launch Devel...

Comments

@SoftVision-CarmenFat
Copy link

[Affected versions]:

  • Firefox Release v63.0.1 and up

[Affected Platforms]:

  • All Linux

[Prerequisites]:

  • Have a Firefox profile with the latest version of the "Price Wise" add-on (10.0.0) downloaded from (download link).
  • The Price Wise button was pinned to the Overflow Menu.

[Steps to reproduce]:

  1. Open the browser with the profile from prerequisites.
  2. Click the "More tools..." toolbar button and select the "Tracked Products" option.
  3. Observe the UI of the "Price Watcher List".

[Expected result]:

  • The door hanger is correctly displayed, without any UI issues.

[Actual result]:

  • A blank space is displayed on the right side of the door hanger.

[Notes]:

  • This issue is also reproducible when there are products in the Price Wise list.
  • Here is a screen recording of the issue:

linux issue
@SoftVision-CarmenFat SoftVision-CarmenFat added the [QA]:Normal issue Label for QA to mark normal issues logged label Nov 8, 2018
@Osmose Osmose added this to the Post-Launch Development-1 milestone Nov 11, 2018
@biancadanforth
Copy link
Collaborator

Thanks for filing this issue.

I'm going to guess the Overflow menu on Linux is even wider than our max-width of 400px from PR #249 .

@Osmose
Copy link
Contributor

Osmose commented Nov 12, 2018

I've seen this on MacOS before, it went away after I closed and reopened the panel. I wonder if it has something to do with how Firefox is determining the panel width?

@biancadanforth biancadanforth self-assigned this Nov 14, 2018
biancadanforth added a commit to biancadanforth/price-tracker that referenced this issue Nov 15, 2018
@biancadanforth
Fix mozilla#256: Change popup width in overflow menu
29e47b1 
With CustomizableUI.jsm (via the experimental `browser.customizableUI` API added in mozilla#157), it is possible to detect when the browserAction toolbar button is and is not in the Overflow menu.

Because the overflow menu width varies by OS (for example, it is 377px wide on my Mac 10.13 and 425px wide on a Linux 14.04), this patch will detect whether or not the toolbar button is in the Overflow menu and adjust the width accordingly.

This fix has the added benefit of allowing us to constrain the non-Overflow menu popup width (which is hopefully the vast majority use case) to the UX spec of 320px where previously we had set it to a max-width of 400px to try to accomodate Overflow menu issues.
biancadanforth added a commit to biancadanforth/price-tracker that referenced this issue Nov 16, 2018
@biancadanforth
Fix mozilla#256: Change popup width in overflow menu
5b4e159 
With CustomizableUI.jsm (via the experimental `browser.customizableUI` API added in mozilla#157), it is possible to detect when the browserAction toolbar button is and is not in the Overflow menu.

Because the overflow menu width varies by OS (for example, it is 377px wide on my Mac 10.13 and 425px wide on a Linux 14.04), this patch will detect whether or not the toolbar button is in the Overflow menu and adjust the width accordingly.

This fix has the added benefit of allowing us to constrain the non-Overflow menu popup width (which is hopefully the vast majority use case) to the UX spec of 320px where previously we had set it to a max-width of 400px to try to accomodate Overflow menu issues.
biancadanforth added a commit to biancadanforth/price-tracker that referenced this issue Nov 17, 2018
@biancadanforth
Fix mozilla#274: Add UR study participation UI
6e58b15 
Notes:
- I had to put the copy in brackets in StudyInvitation.jsx to be able to use the apostrophe it contains. This avoids the eslint rule error react/no-unescaped-entities.
- The panels are wider than bryanbell's mocks, but the PR for mozilla#256 will fix that.
biancadanforth added a commit to biancadanforth/price-tracker that referenced this issue Nov 17, 2018
@biancadanforth
Fix mozilla#274: Add UR study participation UI
9099d1a 
Notes:
- I had to put the copy in brackets in StudyInvitation.jsx to be able to use the apostrophe it contains. This avoids the eslint rule error react/no-unescaped-entities.
- The panels are wider than bryanbell's mocks, but the PR for mozilla#256 will fix that.
biancadanforth added a commit to biancadanforth/price-tracker that referenced this issue Nov 17, 2018
@biancadanforth
Fix mozilla#274: Add UR study participation UI
5accc1e 
Notes:
- I had to put the copy in brackets in StudyInvitation.jsx to be able to use the apostrophe it contains. This avoids the eslint rule error react/no-unescaped-entities.
- The panels are wider than bryanbell's mocks, but the PR for mozilla#256 will fix that.
@biancadanforth biancadanforth mentioned this issue Nov 19, 2018
Merged
2 tasks
biancadanforth added a commit that referenced this issue Nov 21, 2018
@biancadanforth
Merge pull request #271 from biancadanforth/256-overflow-width
5867e5e 
Fix #256: Change popup width in overflow menu
@SoftVision-CarmenFat
Copy link
Author

Tested this again using the latest Price Wise version (15.0.0) from Test Pilot platform on Ubuntu 18.04 x64 and the blank space is still displayed on the right side of the door hanger. Reopening the issue accordingly.

@biancadanforth
Copy link
Collaborator

@SoftVision-CarmenFat , the fix for this landed after v15.0.0 was released, so I would not expect any changes in v15.0.0. As a result, I am going to close this issue again.

We will likely cut a new release later this week that will hopefully include this patch, but there is a way for you to check whether you can verify an issue with the latest official release or not:

  • In this case, any commits on master above the commit labeled 15.0.0 will not have their changes reflected in v15.0.0.

@Softvision-RemusDranca
Copy link

I have verified this issue using the latest Price Wise version (15.0.0) custom build and the issue is no longer reproducible. Tested on Ubuntu 18.04 x64 and Arch Linux 4.16.6 x64.

@Softvision-RemusDranca Softvision-RemusDranca added the [QA]:Verified fixed Label for QA to mark verified fixed issues label Nov 27, 2018
Osmose pushed a commit to Osmose/price-wise that referenced this issue Nov 28, 2018
Fix mozilla#290: Do not allow scrollbars on the entire popup itself.
4a673dc 
This overflow declaration was added in mozilla#215, nominally to help deal with
overflow issues when pinned to the overflow menu, but it's not clear how this
particular line helps vs the other changes to the width of the onboarding
view.

My theory on why this is biting us now:

- mozilla#256 updated the popup to a fixed width when not in the overflow menu, and a
  flexible width when it is.
- When we have more items than the 600px max height in the popup, we overflow
  and add a vertical toolbar inside.
- The vertical toolbar increases the desired width of the popup, causing the
  body element to add a horizontal scrollbar.
- The horizontal scrollbar increases the height beyond the 600px limit (since
  the initial overflow is contained within the product listing), causing the
  body element to add a vertical scrollbar.

If we force the body element to hide overflow, it refuses to add scrollbars, and
the overflow scrollbar on the product listing is overlaid correctly. This should
be safe to do now that we flex the width of the popup based on where it is
located.
@Osmose Osmose mentioned this issue Nov 28, 2018
Merged
Osmose pushed a commit to Osmose/price-wise that referenced this issue Nov 28, 2018
Fix mozilla#290: Do not allow scrollbars on the entire popup itself.
9e78d8b 
This overflow declaration was added in mozilla#215, nominally to help deal with
overflow issues when pinned to the overflow menu, but it's not clear how this
particular line helps vs the other changes to the width of the onboarding
view.

My theory on why this is biting us now:

- mozilla#256 updated the popup to a fixed width when not in the overflow menu, and a
  flexible width when it is.
- When we have more items than the 600px max height in the popup, we overflow
  and add a vertical toolbar inside.
- The vertical toolbar increases the desired width of the popup, causing the
  body element to add a horizontal scrollbar.
- The horizontal scrollbar increases the height beyond the 600px limit (since
  the initial overflow is contained within the product listing), causing the
  body element to add a vertical scrollbar.

If we force the body element to hide overflow, it refuses to add scrollbars, and
the overflow scrollbar on the product listing is overlaid correctly. This should
be safe to do now that we flex the width of the popup based on where it is
located.
@biancadanforth biancadanforth mentioned this issue Jul 18, 2019
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@biancadanforth biancadanforth

Labels
[QA]:Normal issue Label for QA to mark normal issues logged [QA]:Verified fixed Label for QA to mark verified fixed issues
Projects
None yet
Milestone
Post-Launch Development-1
Development

No branches or pull requests
4 participants
@Osmose @biancadanforth @SoftVision-CarmenFat @Softvision-RemusDranca