1148081 - Cubic bezier tooltip displays a scrollbar in light theme

Status: RESOLVED FIXED

Description

[Tim Nguyen :ntim]

Created attachment 8584050 [details]
Capture d’écran (6).png

Doesn't happen in dark theme which has floating scrollbars.

Comment 1

[John Giannakos [:jtgi]]

Thanks Tim. Can I get details on your environment so I can repro.
I can take this on if someone wants to assign it to me.

Comment 2

[Tim Nguyen :ntim]

(In reply to John Giannakos [:jtgi] from comment #1)
> Thanks Tim. Can I get details on your environment so I can repro.
> I can take this on if someone wants to assign it to me.

I reproduced this bug on Windows 8.1 and Windows 10.

Comment 3

[John Giannakos [:jtgi]]

Created attachment 8585720 [details]
Spotted this same issue on what looks to be OSX in the Nightly.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to John Giannakos [:jtgi] from comment #3)
> Created attachment 8585720 [details]
> Spotted this same issue on what looks to be OSX in the Nightly.

Since you can reproduce, we may as well just shrink the width of the previews a bit until it fits into the container.  Is that going to cause alignment issues in the dark theme?

Comment 5

[John Giannakos [:jtgi]]

Thanks Brian, I'll try that and a few other things out.

To be clear, I can't reproduce at any resolution on my dev machine (Macbook Pro Retina). The screen was taken from twitter. Since it looks like we are both running Yosemite on OSX, I am guessing the difference may lie in retina display vs non retina.

I will set up a dev env on my win8 setup at home and debug from there. 

Unfortunately, the earliest I can get to this is Thursday.

Comment 6

[John Giannakos [:jtgi]]

Created attachment 8588282 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

I was able to reproduce the bug on my Windows 8.1 setup. 
I was *not* able to reproduce the original bug on another Macbook Pro (non-retina) running Mountain Lion.

Attached is a fix to:
1) Prevent the scroll bar from displaying if content does not overflow.
2) Properly reflow the preset display on all resolutions and themes if and when more presets are added and content does overflow.

Please see the follow up image to get a sense of how this scales.

Comment 7

[John Giannakos [:jtgi]]

Created attachment 8588285 [details]
scrollbar-fix-screenshots.jpg

Attached is an image showing how the layout changes depending on resolution or if content overflows the container.

To be clear, with the current amount of presets we should not see a scrollbar on any device.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8588282 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Tim, what do you think about this?  Does it fix the problem for you?

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8588282 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Review of attachment 8588282 [details] [diff] [review]:
-----------------------------------------------------------------

This seems generally good - just a few notes

::: browser/devtools/shared/widgets/cubic-bezier.css
@@ +138,2 @@
>  .preset-pane {
>    width:50%;

Nit: whitespace between : and 50%

@@ +138,4 @@
>  .preset-pane {
>    width:50%;
>    height: 100%;
>    border-right: 1px solid var(--theme-splitter-color);

I think the padding-right on active-preset-list (and margin-left on #preset-categories) is intending to get things to appear centered.  They are appearing off centered because of the 4px padding around the panel-arrowcontent.  So, instead of those two things, I think you could just do this here:

padding-right: 4px; /* Visual balance for the panel-arrowcontent border on the left */

@@ +146,4 @@
>    border: 1px solid var(--theme-splitter-color);
>    border-radius: 2px;
>    background-color: var(--theme-toolbar-background);
> +  margin-left: 3px;

If you make the suggested change to preset-pane I believe that you can do:

  margin: 3px auto 0 auto;

@@ +190,5 @@
>    display: flex;
>    flex-wrap: wrap;
> +  justify-content: flex-start;
> +  padding-top: 6px;
> +  padding-right: 3px;

A couple of notes:

1) I think any rules (except for the display property) on the preset list should be moved to the .preset-list selector above.  This one can just toggle the display.
2) It feels weird that there would need to be a padding on this element - I don't think it's needed if you make the suggested change on .preset-pane above

@@ +195,4 @@
>  }
>  .preset {
>    cursor: pointer;
> +  width: 33%;

Should this be 33.33%?

@@ +213,2 @@
>    font-size: 0.9em;
> +  margin: 2px auto 6px auto;

This 6px on the bottom makes more sense on the .preset element (since I don't think there is ever a preset without a p inside of it).  Could make it 5 and do margin: 5px 0; on that element

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8588285 [details]
scrollbar-fix-screenshots.jpg

Looks good!

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8588282 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Clearing feedback - I think Comment 9 covers it

Comment 12

[Tim Nguyen :ntim]

Sorry, I haven't got time to test the patch, I'm currently pretty busy with exams. I'll be able to get to it next week.

Comment 13

[John Giannakos [:jtgi]]

Created attachment 8591535 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Brian, thanks for the review. The patch has been updated in response to your feedback. 

I've verified the fix is still good to go on Win8.1 and MBP Retina.

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8591535 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Review of attachment 8591535 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=529f20c651d7.  Can you reupload with a commit message on the patch?  Something like: 'Bug 1148081: Cubic bezier tooltip styling updates to prevent unnecessary overflow; r=bgrins'

Comment 15

[John Giannakos [:jtgi]]

Created attachment 8591763 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Added proper commit message.

Comment 16

[John Giannakos [:jtgi]]

Is there anything I should do to flag that this fix is to be included with the original patch for any major release or will that work itself out?

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to John Giannakos [:jtgi] from comment #16)
> Is there anything I should do to flag that this fix is to be included with
> the original patch for any major release or will that work itself out?

Once this lands in m-c we will want to request aurora uplift to make sure it gets bundled in 39 with Bug 1134568.  I'll leave this needinfo pending so I remember to do that.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/3054afc750a1

Comment 19

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #18)
> https://hg.mozilla.org/integration/fx-team/rev/3054afc750a1

John, also thanks for contributing to mozilla!

Comment 20

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/3054afc750a1

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8591763 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Approval Request Comment
[Feature/regressing bug #]: 1134568 
[User impact if declined]: An inappropriate scrollbar shows up in the cubic bezier tooltip within devtools
[Describe test coverage new/current, TreeHerder]: The feature has tests, but no new ones are added here.  It's styling changes only.
[Risks and why]: Low risk, styling change to DevTools only
[String/UUID change made/needed]:

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8591763 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Looks straightforward enough. Aurora+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4373b504af7