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 for bigger fonts/longer translations #1445

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ghostwords
Copy link
Member

@ghostwords ghostwords commented Jun 20, 2017

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 ghostwords added the ui User interface modifications; related to but not the same as the "ux" label label Jun 29, 2017
@andresbase andresbase added this to the W28-29 milestone Jul 6, 2017
@andresbase andresbase modified the milestones: W30-31, W28-29 Jul 24, 2017
@ghostwords
Copy link
Member Author

ghostwords commented Aug 21, 2017

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


This (6c0c3ae) 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.

  • 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 ghostwords requested a review from cowlicks August 21, 2017 16:04
@ghostwords
Copy link
Member Author

ghostwords commented Aug 21, 2017

@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
Copy link
Member Author

ghostwords commented Aug 21, 2017

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
Copy link
Member Author

@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 ghostwords force-pushed the fix-popup-layout branch 2 times, most recently from 14a1a5e to d77686c Compare August 21, 2017 17:04
@ghostwords
Copy link
Member Author

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 {

@lemnis
Copy link
Contributor

lemnis commented Aug 23, 2017

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 (#1445 (comment)) 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.

@cowlicks
Copy link
Contributor

cowlicks commented Sep 1, 2017

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

@@ -0,0 +1,103 @@
body
{

Choose a reason for hiding this comment

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

Whats up with mix and match open brackets?

color: #b0b0b0;
}

.importInput{
Copy link

@the-j0k3r the-j0k3r Dec 16, 2020

Choose a reason for hiding this comment

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

Space before open brackets...

/me thinks this CSS needs some linting and fixing for consistency looking at master its all over the place.


.body-content-outer-wrapper {
height: 100%;

Choose a reason for hiding this comment

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

space between properties?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui User interface modifications; related to but not the same as the "ux" label
Projects
None yet
5 participants