Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix popup layout for overflow menu in Firefox #2158

Merged
merged 3 commits into from
Sep 20, 2018

Conversation

larabr
Copy link
Contributor

@larabr larabr commented Sep 5, 2018

Fix #1120 by setting ad-hoc styles for the popup shown from the overflow menu.

One odd thing I've noticed, and maybe you can check if it happens to you as well: the first time the popup is opened from the overflow menu, the popup is wider than it should be and there is a blank space on the right. Screenshot:

a1q5ftfstqc_lgigometgw

From then on the popup is shown correctly.
Also, the problem is not present if the option "disable popup auto hide" is set. It looks like firefox is not updating the width of the popup fast enough.

@bcyphers bcyphers self-requested a review September 5, 2018 19:29
@bcyphers
Copy link
Contributor

bcyphers commented Sep 6, 2018

Can't repro the original issue on Ubuntu 18.04, but there's a different issue (both in this branch and in master):
generalbrus-fix-bug

It would be awesome if we could get rid of the scroll bar on the side when we fix this issue.
I'll try this on a Windows machine at home tonight and report back.

@larabr
Copy link
Contributor Author

larabr commented Sep 6, 2018

@bcyphers your popup seems to be normal width. At least on my system (macOS), if I pin the extension to the overflow menu (probably called only "menu" in earlier Firefox releases), the popup is shrunk, which caused some layout issues.

About the scroll bar - do you want to reduce the content height basically, to have it fit without scrolling?

@bcyphers
Copy link
Contributor

Sorry for the delay. Yes, the issue I pointed out above is different, and the problem this PR is trying to address was never a problem on Ubuntu. It would be great if we could fix both problems with one PR, but maybe they're too unrelated.

The original issue isn't fixed for me in Firefox on Windows 10:
pb-firefox-broken

Copy link
Contributor

@bcyphers bcyphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

@larabr
Copy link
Contributor Author

larabr commented Sep 14, 2018

The popup is a bit larger on Windows so the media query wouldn't get applied - now it should work.
I'll look into removing the overflow of the main window later.

Copy link
Contributor

@bcyphers bcyphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! everything works for me now.
Thank you so much for your contribution, @generalbrus !

@bcyphers
Copy link
Contributor

The test failure here looks unrelated to the changes. @ghostwords have you seen this before? https://travis-ci.org/EFForg/privacybadger/jobs/428562652

@ghostwords
Copy link
Member

The same test failed once (also with Firefox) in another PR: https://travis-ci.org/EFForg/privacybadger/builds/429624697. I don't remember seeing this before though.

@ghostwords
Copy link
Member

ghostwords commented Sep 19, 2018

Hi @andresbase, could you check this PR on Firefox for Android please? There should be no changes. Would be great to confirm on Firefox/Chrome on macOS too; no changes to the popup in either browser by default, but the popup should be fully usable now when placed in the overflow menu on Firefox.

@bcyphers
Copy link
Contributor

@ghostwords I finally got the android test setup working, and this looks fine.

@ghostwords ghostwords merged commit c44d7ea into EFForg:master Sep 20, 2018
ghostwords added a commit that referenced this pull request Sep 20, 2018
Fix popup layout problems when opened in the overflow menu in Firefox.
@ghostwords
Copy link
Member

Thanks again!

@DJCrashdummy
Copy link
Contributor

#1120 seems to be fixed now. thank you! 👍
but as shown in #2146 (comment) the last "banner/button/link" is still cut off.

@ghostwords
Copy link
Member

Please subscribe to #1077 for the clipped buttons bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants