Closed Bug 931041 Opened 6 years ago Closed 6 years ago

Floating Scrollbars do not work with Code Mirror

Categories

(DevTools :: Source Editor, defect, P3)

x86_64
Windows 7
defect

Tracking

(firefox27 verified, firefox28 verified)

VERIFIED FIXED
Firefox 28
Tracking Status
firefox27 --- verified
firefox28 --- verified

People

(Reporter: Optimizer, Assigned: Optimizer)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Since bug 895561 introduced dark modes for editors too, there are no scrollbars in the editors.
Turns out that CM does not work well with floating scrollbars. No idea why.
(In reply to Girish Sharma [:Optimizer] from comment #0)
> Since bug 895561 introduced dark modes for editors too, there are no
> scrollbars in the editors.
> Turns out that CM does not work well with floating scrollbars. No idea why.

What platform(s) should we be seeing this on?  I'm seeing srcollbars in the editors, but maybe I'm not understanding.  Could you add a little more info or a screenshot?
Flags: needinfo?(scrapmachines)
See the screenshot here : http://i.snag.gy/Ekxjz.jpg

The script is very long, but there is no scrollbar on the right. Paul tells me that it is due to subframes. The floating scrollbar logic does not handle subframes.
He also says that it is easy to fix.
Flags: needinfo?(scrapmachines) → needinfo?(paul)
Wait… there's no scrollbars at all? Not native or floating scrollbar?
If you comment `loadSheet(scrollbarsUrl)` in theme-switching.js, do you see the native scrollbars?

If you use winUtils.loadSheet on the CM window, does it work?
Flags: needinfo?(paul)
Girish, could you answer Paul's questions please? Benvie has a Windows machine so, as the last resort, I can work with him to fix this issue.
Flags: needinfo?(scrapmachines)
Priority: -- → P3
Paul, native scrollbars appear when using the light theme, but there are no scrollbars, native or floating, when using the dark theme.  This affects both my Windows 7 64bit machine and Linux VM using the latest nightly.
Okay, so the issue is not with the loading of the sheet. I had a hunch from the beginning that it has to do something with the fact that codemirror has 2 DIV just for scrollbars and when the scrollbars are floating, the div's height and width become 0 as there's nothing inside of it.

To fix this, the solution is to add a min-width and min-height to the classes CodeMirror-vscrollbar and CodeMirror-hscrollbar respectively.

We should add a min-width/height of 17 px.
Flags: needinfo?(scrapmachines)
Attached patch patch (obsolete) — Splinter Review
This brings back the floating scrollbars in dark theme.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #829886 - Flags: review?(paul)
Attachment #829886 - Flags: review?(anton)
Comment on attachment 829886 [details] [diff] [review]
patch

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

I assume you tested this both on light and dark themes? Also, does it affect OS X native floating scroll bars in any way?
Attachment #829886 - Flags: review?(anton) → review+
(In reply to Anton Kovalyov (:anton) from comment #8)
> Comment on attachment 829886 [details] [diff] [review]
> patch
> 
> Review of attachment 829886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume you tested this both on light and dark themes? Also, does it affect
Yeah, since its a min-width/height, it does not hamper the light theme where the scrollbars are actually present. (All scrollbars are minimum 17 px width as far as I have tested)

> OS X native floating scroll bars in any way?

Nope, tested on my Mac (osx 10.8). No effect what so ever.
Blocks: 933215
No longer blocks: 933215
Comment on attachment 829886 [details] [diff] [review]
patch

You need to convince me first that we actually need 17px. I doubt that.
Attachment #829886 - Flags: review?(paul)
Attached patch patch v2Splinter Review
Uses max of floating scrollbar width. Added a proper comment explaining to keep this value in sync with the width of fake floating scrollbar
Attachment #829886 - Attachment is obsolete: true
Attachment #830747 - Flags: review?(paul)
Comment on attachment 830747 [details] [diff] [review]
patch v2

Why not using: height:100% and width:100% ?

Then we don't need a static number.

Just an idea (and 10px sounds good though).
Comment on attachment 830747 [details] [diff] [review]
patch v2

100% won't work. The div is not click through…
Attachment #830747 - Flags: review?(paul) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fbea5638ad49
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Comment on attachment 830747 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 919709,919978
User impact if declined: No scrollbars in various editors like debugger, scratchpad, style editor etc. in dark theme.
Testing completed (on m-c, etc.): local, m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none
Attachment #830747 - Flags: approval-mozilla-aurora?
Keywords: regression
Comment on attachment 830747 [details] [diff] [review]
patch v2

Approving the low risk uplift to avoid this regression. :Optimzer, can you Please help cover testing in these areas ?
Attachment #830747 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to bhavana bajaj [:bajaj] from comment #17)
> Comment on attachment 830747 [details] [diff] [review]
> patch v2
> 
> Approving the low risk uplift to avoid this regression. :Optimzer, can you
> Please help cover testing in these areas ?

We don't have any tests related to floating scrollbars. We should get some. filed bug 939990
Without tests, you'll need to manually verify it works on Aurora before landing. Do you have all OS covered or do you need someone's help with that?
Seems to be working on Aurora.
verified per comment 21
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.