privacybadger icon indicating copy to clipboard operation
privacybadger copied to clipboard

Fix popup for bigger fonts/longer translations

Open ghostwords opened this issue 8 years ago • 7 comments

Fixes #1113, fixes #1077.

The goal is to at least support 110% and 125% page zoom settings in Chrome (and equivalent (up to 1.25?) layout.css.devPixelsPerPx about:config values in Firefox)

ghostwords avatar Jun 20 '17 17:06 ghostwords

Force-pushed a new approach, will update the description. Saving the first try below for posterity:


This (6c0c3aef5239197d9aa10778caf0ed0e19c3a9fa) uses Flexbox (based on a couple of SO threads: one, two) to set up a few fill-all-remaining-space-without-shoving-the-rest-of-the-content-off-screen cells.

  • [x] Use Flexbox to handle >100% page zooms and longer internationalized translations
  • [ ] Fix opening popup in Firefox becoming extra crazy slow after these changes
  • [ ] Fix various popup whitespace discrepancies introduced by these changes
  • [ ] Check if explicitly setting height takes care of way-too-short-popup Firefox issues
  • [ ] Try using tables to achieve intelligent resizing without performance/styling issues

ghostwords avatar Aug 21 '17 16:08 ghostwords

@lemnis @cowlicks We should be close to a solution here. Screenshots follow.

Before at 125% zoom:

screenshot from 2017-08-21 12 16 16

After at 125% zoom:

screenshot from 2017-08-21 12 16 20

ghostwords avatar Aug 21 '17 16:08 ghostwords

The main problem left to solve (that I'm aware of) is that this patch reduces the size of the domain scroll area at 100% zoom, which is undesirable. Screenshots follow.

Before at default (100% zoom):

screenshot from 2017-08-21 12 16 32

After at default zoom:

screenshot from 2017-08-21 12 16 37

ghostwords avatar Aug 21 '17 16:08 ghostwords

@lemnis It's also possible I broke something related to your recently-merged Android compatibility fixes. We should check ... Unfortunately I am not yet set up for Android development.

ghostwords avatar Aug 21 '17 16:08 ghostwords

It seems like the following diff gets us to what we want in terms of popup sizing (more domains shown at 100% zoom, fewer domains at larger zooms), but it introduces bizarre popup resizing behavior, seemingly triggered by tooltips and/or mousing over slider handles:

diff --git a/src/skin/popup-layout.css b/src/skin/popup-layout.css
index 803d44e..df6291b 100644
--- a/src/skin/popup-layout.css
+++ b/src/skin/popup-layout.css
@@ -2,6 +2,7 @@ body {
     overflow: hidden;
     margin: 8px 8px 0;
     padding: 0 7px;
+    height: 100%;
 }
 
 .table {
@@ -17,7 +18,7 @@ body {
 }
 
 .container {
-    height: 38em;
+    height: 100%;
 }
 
 .header {

ghostwords avatar Aug 21 '17 17:08 ghostwords

I have taken a quick look a noticed 2 things:

  • The popup is wider than 100%, beneath code would fix it. (the extra pixel didn't bother me 😈 )
  • overflow: hidden is set on the body, this should be prevented, if for whatever reason the content can not be rendered within the given space the user should still be able to scroll.
    • e.g. When you pinch-zoom you are unable to scroll
diff --git a/src/skin/popup-layout.css b/src/skin/popup-layout.css
index 803d44e..6820c0e 100644
--- a/src/skin/popup-layout.css
+++ b/src/skin/popup-layout.css
@@ -1,7 +1,6 @@
 body {
-    overflow: hidden;
-    margin: 8px 8px 0;
-    padding: 0 7px;
+    padding: 8px 15px;
+    box-sizing: border-box;
 }
 
 .table {
diff --git a/src/skin/popup.css b/src/skin/popup.css
index ab8514e..1886b58 100644
--- a/src/skin/popup.css
+++ b/src/skin/popup.css
@@ -25,8 +25,6 @@ body {
   background: #fefefe;
   color: #333;
   font-family: helvetica, arial, sans-serif;
-  padding: 8px 15px;
-  box-sizing: border-box;
 }
 @media screen and (min--moz-device-pixel-ratio:0) {
     body{

I believe there is more code within popup.css, that should be inside popup-layout.css. At least I added code that should be there.

Your proposed changes in (https://github.com/EFForg/privacybadger/pull/1445#issuecomment-323803046) didn't work on FF desktop screen shot 2017-08-23 at 01 35 32

The only solution for Firefox that I can see is using your proposed changed with some extra behavior. First disable position: absolute on .body-content, so that the popup can render at his natural height, get the innerHeight and set that on the body element and finally re-enabling the position property.
I assume that flexbox is somewhat calculated the same within FF resulting in that slow behavior.

lemnis avatar Aug 23 '17 00:08 lemnis

@ghostwords do you still want to me to review this. Would you mind rebasing it on master first?

cowlicks avatar Sep 01 '17 01:09 cowlicks