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

Long tooltips may not fit vertically, and we flip them upwards even if they won't fit upwards either #883

Closed
mstange opened this issue Mar 22, 2018 · 13 comments
Labels
accessibility Related to making the profiler UI accessible assigned A developer has requested to work on this issue. good first issue Good issue for new contributors, the issue must have clear instructions on how to complete the work help wanted Things ready to be worked on by anyone. Issues must include instructions on how to complete the task polish Small features or changes that do not require planning to work on. These help out our end users. ready Issue has defined requirements. It can be grabbed and worked on

Comments

@mstange
Copy link
Contributor

mstange commented Mar 22, 2018

No description provided.

@mstange mstange added bug Very important to fix, typically this means that the tool is broken or lying polish Small features or changes that do not require planning to work on. These help out our end users. accessibility Related to making the profiler UI accessible labels Mar 22, 2018
@julienw
Copy link
Contributor

julienw commented Mar 22, 2018

This is made very visible when zoomed in (ctrl +) with high tooltips (like the ones with causes).

We should always try to keep the top aligned with the top of the window.

@gregtatum
Copy link
Member

What is the desired outcome here? The information will be truncated regardless of what we do. I'm going to remove the bug label, as the polish and accessibility seem to be the proper labels, but this is not truly breaking things for our users, as some information is hidden anyway. I'm happy to add the bug label back if someone disagrees.

I think a better "steps to reproduce" profile would be helpful to determine the course of action.

Perhaps if we overflow the height, we need to fit the tooltip to be the size of the vertical window, and then truncate the contents.

@gregtatum gregtatum removed the bug Very important to fix, typically this means that the tool is broken or lying label Sep 17, 2018
@julienw
Copy link
Contributor

julienw commented Sep 18, 2018

Some information is hidden anyway, but I think the most important information is hidden while the less important is shown.

I find it easy to see on this profile, the STR is:

  1. zoom in the page (ctrl +) several times
  2. reduce the height of your browser window
  3. hover on top of "reflow" markers (you can search them using the excellent search field)
  4. If everything looks normal, try zooming in a bit more, or reduce the browser window's height more, and try again.

capture d ecran de 2018-09-18 14-55-40

@julienw
Copy link
Contributor

julienw commented Sep 18, 2018

Note that I agree it's a "polish" bug.

@mstange
Copy link
Contributor Author

mstange commented Sep 19, 2018

I propose the following positioning scheme:

  1. If the tooltip fits between the mouse and the window's bottom edge, position the tooltip's top edge at the mouse.
  2. Otherwise, if the tooltip fits between the window's top edge and the mouse, position the tooltip's bottom edge at the mouse.
  3. Otherwise, if the tooltip fits between the window's top edge and the window's bottom edge, position the tooltip's bottom edge at the window's bottom edge.
  4. Otherwise, position the tooltip's top edge at the window's top edge.

@julienw
Copy link
Contributor

julienw commented Sep 20, 2018

This sounds good; except maybe we don't need the 3rd step and could move to 4th step directly.

@julienw julienw added help wanted Things ready to be worked on by anyone. Issues must include instructions on how to complete the task good first issue Good issue for new contributors, the issue must have clear instructions on how to complete the work ready Issue has defined requirements. It can be grabbed and worked on labels Oct 1, 2018
@julienw
Copy link
Contributor

julienw commented Oct 1, 2018

The logic is in src/shared/Tooltip.js's render function.

@subashiniganesh
Copy link

Hello @julienw ,

I am introduced to Outreachy and about this project today via Outreachy website. Is it too late to start? May I please attempt on this issue and try to make a Pull Request?

@julienw
Copy link
Contributor

julienw commented Oct 3, 2018

He @subashiniganesh, sure, it's not late at all !

Are the information in this issue good enough or do you need some more things to start working on this ?

subashiniganesh pushed a commit to subashiniganesh/perf.html that referenced this issue Oct 3, 2018
Previously, long tooltips were flipped upwards and so
sometimes, it goes beyond the window's top edge.
This makes it difficult to view the information.
This fix will make sure that, the tooltip header always
stays within the window.

Resolves: firefox-devtools#883
@subashiniganesh
Copy link

Hello @julienw ,

The information provided in the issue was very much helpful. Thanks a lot. I am happy that I was able to pick and attempt an accessibility related issue as the primary project is about improving accessibility. Let me please submit a PR. Kindly help me for any improvements.

@julienw julienw added the assigned A developer has requested to work on this issue. label Oct 4, 2018
@julienw julienw removed the assigned A developer has requested to work on this issue. label Oct 25, 2018
@julienw
Copy link
Contributor

julienw commented Oct 25, 2018

No answer from @subashiniganesh so let's mark this issue as unassigned. Please look at the existing PR #1326 to start working on this.

@gregtatum
Copy link
Member

Including @bgrins steps to reproduce (STR) here from #1462:

STR

I can see just the bottom of "Styles reused" metadata, but there's more interesting stuff above about the number of elements traversed / matched / etc. If the tooltip doesn't have enough space I'd rather see the top of it and have the bottom stack portion get clipped.

screenshot 2018-11-06 10 20 05

@julienw julienw added the assigned A developer has requested to work on this issue. label Nov 7, 2018
@julienw
Copy link
Contributor

julienw commented Nov 7, 2018

Because this is blocking some gecko devs, I'll finish up the existing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Related to making the profiler UI accessible assigned A developer has requested to work on this issue. good first issue Good issue for new contributors, the issue must have clear instructions on how to complete the work help wanted Things ready to be worked on by anyone. Issues must include instructions on how to complete the task polish Small features or changes that do not require planning to work on. These help out our end users. ready Issue has defined requirements. It can be grabbed and worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants