931041 - Floating Scrollbars do not work with Code Mirror

Status: VERIFIED FIXED

(DevTools :: Source Editor, defect, P3)

Description

[Girish Sharma [:Optimizer]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

(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?

Comment 2

[Girish Sharma [:Optimizer]]

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.

Comment 3

[Paul Rouget [: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?

Comment 4

[Anton Kovalyov (:anton)]

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.

Comment 5

[Trevor Rowbotham [:rowbot]]

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.

Comment 6

[Girish Sharma [:Optimizer]]

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.

Comment 7

[Girish Sharma [:Optimizer]]

Attachment: patch

This brings back the floating scrollbars in dark theme.

Comment 8

[Anton Kovalyov (: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?

Comment 9

[Girish Sharma [:Optimizer]]

(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.

Comment 10

[Paul Rouget [:paul]]

Comment on attachment 829886 [details] [diff] [review]
patch

You need to convince me first that we actually need 17px. I doubt that.

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: patch v2

Uses max of floating scrollbar width. Added a proper comment explaining to keep this value in sync with the width of fake floating scrollbar

Comment 12

[Paul Rouget [: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 13

[Paul Rouget [:paul]]

Comment on attachment 830747 [details] [diff] [review]
patch v2

100% won't work. The div is not click through…

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/fbea5638ad49

Comment 16

[Girish Sharma [:Optimizer]]

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

Comment 17

[bhavana bajaj [:bajaj]]

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 ?

Comment 18

[Girish Sharma [:Optimizer]]

(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

Comment 19

[Anton Kovalyov (:anton)]

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?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Anton Kovalyov (:anton)]

Seems to be working on Aurora.

Comment 22

[Tracy Walker [:tracy]]

verified per comment 21