Cubic bezier tooltip displays a scrollbar in light theme

RESOLVED FIXED in Firefox 39

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ntim, Assigned: jtgi)

Tracking

Trunk
Firefox 40
Points:
---

Firefox Tracking Flags

(firefox38 unaffected, firefox39 fixed, firefox40 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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

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

Comment 1

2 years ago
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.
(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.
(Assignee)

Comment 3

2 years ago
Created attachment 8585720 [details]
Spotted this same issue on what looks to be OSX in the Nightly.
(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?
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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.
Attachment #8588282 - Flags: feedback?(bgrinstead)
(Assignee)

Comment 7

2 years ago
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.
Attachment #8588285 - Flags: feedback?(bgrinstead)
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?
Attachment #8588282 - Flags: feedback?(ntim.bugs)
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
Attachment #8588282 - Flags: feedback?(bgrinstead) → feedback+
Comment on attachment 8588285 [details]
scrollbar-fix-screenshots.jpg

Looks good!
Attachment #8588285 - Flags: feedback?(bgrinstead) → feedback+
Comment on attachment 8588282 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Clearing feedback - I think Comment 9 covers it
Attachment #8588282 - Flags: feedback?(ntim.bugs)
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.
(Assignee)

Comment 13

2 years ago
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.
Attachment #8588282 - Attachment is obsolete: true
Attachment #8591535 - Flags: feedback?(bgrinstead)
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'
Attachment #8591535 - Flags: feedback?(bgrinstead) → review+
(Assignee)

Comment 15

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

Added proper commit message.
Attachment #8591535 - Attachment is obsolete: true
Attachment #8591763 - Flags: review?(bgrinstead)
Attachment #8591763 - Flags: review?(bgrinstead) → review+
Assignee: nobody → j
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Assignee)

Comment 16

2 years ago
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?
Flags: needinfo?(bgrinstead)
(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.
https://hg.mozilla.org/integration/fx-team/rev/3054afc750a1
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
(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

2 years ago
https://hg.mozilla.org/mozilla-central/rev/3054afc750a1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
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]:
Flags: needinfo?(bgrinstead)
Attachment #8591763 - Flags: approval-mozilla-aurora?
status-firefox38: --- → unaffected
status-firefox39: --- → affected
Comment on attachment 8591763 [details] [diff] [review]
cubic-bezier-scrollbar-fix.patch

Looks straightforward enough. Aurora+
Attachment #8591763 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4373b504af7
status-firefox39: affected → fixed
You need to log in before you can comment on or make changes to this bug.