Skip to content

[WebProfilerBundle] Fix AJAX panel width for long URLs #20715

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

Merged

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Dec 1, 2016

Q A
Branch? 3.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

When the AJAX url path is very long, the profile <td> token value is not fully displayed.

Before

before

After

after

Other Possible Solutions

  1. Fix max-width from .sf-toolbar-block:hover .sf-toolbar-info class to 512px. (same result but the AJAX panel is a bit longer)
  2. Fix max-width from .sf-toolbar-block:hover .sf-toolbar-info class to none or remove it. It would avoid future issues (mainly with third bundles) with children width greater than 480px. (Promising) ?

//cc @javiereguiluz

@chalasr
Copy link
Member

chalasr commented Dec 1, 2016

@yceruto I believe this is a duplicate of #20713 (somehow implementing one of your proposed solutions to fix the same bug) :/

@yceruto
Copy link
Member Author

yceruto commented Dec 1, 2016

Unbelievable! I researched this (12hr ago) before creating the PR 😢

@yceruto
Copy link
Member Author

yceruto commented Dec 1, 2016

@chalasr Thanks for the warning. I have no problem with close this PR as duplicated, however I will keep it open, since this one must be done in 2.8 and I propose another solution, later the @symfony/deciders will take the right desision.

@ogizanagi
Copy link
Contributor

ogizanagi commented Dec 1, 2016

I don't have any issue using 3.2. I think it has been fixed silently in #19576, which match your solution (but slightly different value).

So 👍 for this one by using the same value if coherent.

BTW, the diff between 2.8 and 3.2
diff --git a/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar.css.twig b/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar.css.twig
index d87e813..1b7b2f5 100644
--- a/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar.css.twig
+++ b/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar.css.twig
@@ -26,6 +26,15 @@
     display: inline;
 }
 
+.sf-toolbar-clearer {
+    clear: both;
+    height: 36px;
+}
+
+.sf-display-none {
+    display: none;
+}
+
 .sf-toolbarreset * {
     -webkit-box-sizing: content-box;
     -moz-box-sizing: content-box;
@@ -36,7 +45,7 @@
 .sf-toolbarreset {
     background-color: #222;
     bottom: 0;
-    box-shadow: 0 -1px 0px rgba(0, 0, 0, 0.2);
+    box-shadow: 0 -1px 0 rgba(0, 0, 0, 0.2);
     color: #EEE;
     font: 11px Arial, sans-serif;
     left: 0;
@@ -139,7 +148,7 @@
 
 .sf-toolbar-block .sf-toolbar-info-piece .sf-toolbar-status {
     padding: 2px 5px;
-    margin-bottom: 0px;
+    margin-bottom: 0;
 }
 .sf-toolbar-block .sf-toolbar-info-piece .sf-toolbar-status + .sf-toolbar-status {
     margin-left: 4px;
@@ -233,6 +242,16 @@
 .sf-toolbar-block-request .sf-toolbar-info-piece a:hover {
     text-decoration: underline;
 }
+.sf-toolbar-block-request .sf-toolbar-redirection-status {
+    font-weight: normal;
+    padding: 2px 4px;
+    line-height: 18px;
+}
+.sf-toolbar-block-request .sf-toolbar-info-piece span.sf-toolbar-redirection-method {
+    font-size: 12px;
+    height: 17px;
+    line-height: 17px;
+}
 
 .sf-toolbar-status-green .sf-toolbar-label,
 .sf-toolbar-status-yellow .sf-toolbar-label,
@@ -315,7 +334,7 @@
     padding: 4px;
 }
 .sf-ajax-request-url {
-    max-width: 300px;
+    max-width: 250px;
     line-height: 9px;
     overflow: hidden;
     text-overflow: ellipsis;
@@ -381,7 +400,7 @@
 
     .sf-toolbarreset {
         bottom: auto;
-        box-shadow: 0 1px 0px rgba(0, 0, 0, 0.2);
+        box-shadow: 0 1px 0 rgba(0, 0, 0, 0.2);
         top: 0;
     }
 
@@ -451,9 +470,15 @@
         padding-left: 0;
         padding-right: 0;
     }
-    .sf-toolbar-block-request .sf-toolbar-status + .sf-toolbar-label {
-        margin-left: 4px;
+    .sf-toolbar-block-request .sf-toolbar-label {
+        margin-left: 5px;
+    }
+    .sf-toolbar-block-request .sf-toolbar-status + svg {
+        margin-left: 5px;
     }
+    .sf-toolbar-block-request .sf-toolbar-icon svg + .sf-toolbar-label {
+        margin-left: 0;
+     }
     .sf-toolbar-block-request .sf-toolbar-label + .sf-toolbar-value {
         margin-right: 10px;
     }

@yceruto yceruto changed the title [WebProfileBundle] Fix AJAX panel width for long URLs [WebProfilerBundle] Fix AJAX panel width for long URLs Dec 1, 2016
@yceruto
Copy link
Member Author

yceruto commented Dec 1, 2016

(but slightly different value) 250px vs 268px vs 300px

Analyzing it again, it depends of current <td> content (only "Method", "URL" and "Time" vary in size (in 2.8) and .sf-ajax-request-duration class don't has max-width) and still with URL max-width: 250px and "Time" equal to <td>1234567890ms</td> (absurd 😈) we have the same issue, so I think the right solution should be another one instead :/ ?

What do you think about possible solution (1) or (2) preferably ?

@ogizanagi
Copy link
Contributor

ogizanagi commented Dec 1, 2016

(1) Ok, but that's not much different from the current solution 😕
(2) I don't think it's a good idea to not restrict at all the panels size.

@yceruto yceruto force-pushed the fix/2.8/web_profile_bundle/fix_ajax_panel_width branch from c825a94 to b0a8f8e Compare December 1, 2016 20:56
@yceruto
Copy link
Member Author

yceruto commented Dec 1, 2016

Well, anyway a smaller value should cover the issue currently, so I fixed the value to 250px as https://github.com/symfony/symfony/pull/19576/files#diff-485e88132bac45e62c6d82940119bcbaR337

@ghost
Copy link

ghost commented Dec 1, 2016

Hi @yceruto, maybe you have the same issue than me on #20713

Do you have change the default font for the toolbar ? Because I have try with a fresh 2.8 install, and I think I dont have your issue

test

@ogizanagi
Copy link
Contributor

Actually I think the issue only exists after #17540 (which has been merged in 3.1) and adds the HTTP status code, which explains the width issue as the css wasn't updated.

So this should probably target 3.1.

@yceruto
Copy link
Member Author

yceruto commented Dec 1, 2016

Do you have change the default font for the toolbar ? Because I have try with a fresh 2.8 install, and I think I dont have your issue

Nope! I don't have any CSS, to reproduce it in 2.8, just adds sleep(10) to ajax action and returns a valid response to create a valid profile token value.

@ghost
Copy link

ghost commented Dec 1, 2016

@ogizanagi , yes, in 3.1 I have this issue with the same code

test

@yceruto 2.8, with sleep(10) with valid response :

test2

@ogizanagi
Copy link
Contributor

ogizanagi commented Dec 1, 2016

@yceruto : Even having a request with a time faked to 10324000ms will not be sufficient to reproduce on 2.8 which does not have the Status column. I can see this column on your screens, so I assume you're using 3.1, right?

screenshot 2016-12-01 a 22 48 14

@yceruto
Copy link
Member Author

yceruto commented Dec 1, 2016

Actually I think the issue only exists after #17540 (which has been merged in 3.1) and adds the HTTP status code, which explains the width issue as the css wasn't updated.

Right, it's an extra point, but the issue is possible since 2.8 even when my screenshots were taken in 3.1

2.8 screenshot:

2 8

We can see "apparently" no padding-right.

@yceruto
Copy link
Member Author

yceruto commented Dec 1, 2016

@ogizanagi, @NicolasPion You can reproduce it with OPTIONS method additionally? If no, let's change to 3.1 base branch.

@ghost
Copy link

ghost commented Dec 1, 2016

@yceruto yes, I think you can reproduce this bug only if you have many 000000 in the time, and using a sleep(100) in PHP + using "OPTIONS" method in Ajax

In my exemple I have some margin to the right because 556 is smaller than 000

test3

@yceruto
Copy link
Member Author

yceruto commented Dec 6, 2016

@NicolasPion, @ogizanagi However, do you think it's worth fixing it at 2.8? Or do you really think that the problem is more serious from 3.1?

@ogizanagi
Copy link
Contributor

@yceruto : Sorry, I forgot to answer you last time. No I think it's ok in 2.8 even with OPTIONS because the "Status" column does not exist yet and the time needed to overflow is not realistic.

@ghost
Copy link

ghost commented Dec 6, 2016

@yceruto : imo, in 2.8 this is not really a "bug" and it is very difficult to reproduce :

  • You need to have long URL
  • You need to use an "OPTIONS" ajax method.
  • The response need to be more slower than 100000ms

But in 3.1 why not, the only requirement to reproduce this is to have a long URL

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Dec 6, 2016
@yceruto yceruto changed the base branch from 2.8 to 3.1 December 6, 2016 14:42
@yceruto
Copy link
Member Author

yceruto commented Dec 6, 2016

Agreed, changed to 3.1 base branch.

@nicolas-grekas nicolas-grekas modified the milestones: 3.1, 2.8 Dec 6, 2016
@fabpot
Copy link
Member

fabpot commented Dec 13, 2016

Thank you @yceruto.

@fabpot fabpot merged commit b0a8f8e into symfony:3.1 Dec 13, 2016
fabpot added a commit that referenced this pull request Dec 13, 2016
…yceruto)

This PR was merged into the 3.1 branch.

Discussion
----------

[WebProfilerBundle] Fix AJAX panel width for long URLs

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

When the AJAX url path is very long, the **profile ``<td>``** token value is not fully displayed.

### Before

![before](https://cloud.githubusercontent.com/assets/2028198/20801819/16949c88-b7b8-11e6-9186-c350cb0f6868.png)

### After

![after](https://cloud.githubusercontent.com/assets/2028198/20804230/1519a7ec-b7c0-11e6-8ebe-f2ebfa5ab08e.png)

### Other Possible Solutions

1. Fix ``max-width`` from ``.sf-toolbar-block:hover .sf-toolbar-info`` class to ``512px``. (same result but the AJAX panel is a bit longer)
2. Fix ``max-width`` from ``.sf-toolbar-block:hover .sf-toolbar-info`` class to ``none`` or remove it. It would avoid future issues (mainly with third bundles) with children width greater than ``480px``. (Promising) ?

//cc @javiereguiluz

Commits
-------

b0a8f8e Fixed max width from ajax request url element (td)
@yceruto yceruto deleted the fix/2.8/web_profile_bundle/fix_ajax_panel_width branch June 9, 2017 12:46
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
… URLs (yceruto)

This PR was merged into the 3.1 branch.

Discussion
----------

[WebProfilerBundle] Fix AJAX panel width for long URLs

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

When the AJAX url path is very long, the **profile ``<td>``** token value is not fully displayed.

### Before

![before](https://cloud.githubusercontent.com/assets/2028198/20801819/16949c88-b7b8-11e6-9186-c350cb0f6868.png)

### After

![after](https://cloud.githubusercontent.com/assets/2028198/20804230/1519a7ec-b7c0-11e6-8ebe-f2ebfa5ab08e.png)

### Other Possible Solutions

1. Fix ``max-width`` from ``.sf-toolbar-block:hover .sf-toolbar-info`` class to ``512px``. (same result but the AJAX panel is a bit longer)
2. Fix ``max-width`` from ``.sf-toolbar-block:hover .sf-toolbar-info`` class to ``none`` or remove it. It would avoid future issues (mainly with third bundles) with children width greater than ``480px``. (Promising) ?

//cc @javiereguiluz

Commits
-------

b0a8f8e Fixed max width from ajax request url element (td)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants