Closed
Bug 1148081
Opened 6 years ago
Closed 6 years ago
Cubic bezier tooltip displays a scrollbar in light theme
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox38 unaffected, firefox39 fixed, firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox39 | --- | fixed |
firefox40 | --- | fixed |
People
(Reporter: ntim, Assigned: jtgi)
References
Details
Attachments
(4 files, 2 obsolete files)
41.38 KB,
image/png
|
Details | |
1.46 MB,
video/mp4
|
Details | |
420.27 KB,
image/jpeg
|
bgrins
:
feedback+
|
Details |
3.78 KB,
patch
|
bgrins
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Doesn't happen in dark theme which has floating scrollbars.
Assignee | ||
Comment 1•6 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.
Reporter | ||
Comment 2•6 years ago
|
||
(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•6 years ago
|
||
Comment 4•6 years ago
|
||
(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•6 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•6 years ago
|
||
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•6 years ago
|
||
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 8•6 years ago
|
||
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 9•6 years ago
|
||
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 10•6 years ago
|
||
Comment on attachment 8588285 [details]
scrollbar-fix-screenshots.jpg
Looks good!
Attachment #8588285 -
Flags: feedback?(bgrinstead) → feedback+
Comment 11•6 years ago
|
||
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)
Reporter | ||
Comment 12•6 years ago
|
||
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•6 years ago
|
||
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 14•6 years ago
|
||
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•6 years ago
|
||
Added proper commit message.
Attachment #8591535 -
Attachment is obsolete: true
Attachment #8591763 -
Flags: review?(bgrinstead)
Updated•6 years ago
|
Attachment #8591763 -
Flags: review?(bgrinstead) → review+
Updated•6 years ago
|
Assignee: nobody → j
Status: NEW → ASSIGNED
Updated•6 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 16•6 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)
Comment 17•6 years ago
|
||
(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•6 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3054afc750a1
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 19•6 years ago
|
||
(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•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3054afc750a1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Comment 21•6 years ago
|
||
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?
Updated•6 years ago
|
status-firefox38:
--- → unaffected
status-firefox39:
--- → affected
Comment 22•6 years ago
|
||
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+
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•