1206430 - Add CodeMirror scrollbar highlighting addons

Status: NEW

(DevTools :: Source Editor, enhancement, P2)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add new CodeMirror search addons

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Add new CodeMirror scroll addons

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d19a5e90f228

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8701918 [details] [diff] [review]
Part 1: Add new CodeMirror search addons

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

After a discussion, we decided to use this bug as a place to come up with a plan and work on the scrollbar and matchesonscrollbar feature instead of importing all the addons (even ones we don't have a plan to use immediately).  So the plan is to merge these two patches into one, removing jump-to-line and scrollpastend.  Then come up with a demo / screenshot of the scrollbar changes in action so we can work through the designs.  And finally, figure out how much work it's going to be to get the debugger to support this.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add new CodeMirror search addons [1.0]

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Add new CodeMirror scroll addons [1.0]

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add new CodeMirror search addons [2.0]

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Add new CodeMirror scroll addons [2.0]

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Add new CodeMirror scroll addons [3.0]

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Add annotateScrollbar method to editor [1.0]

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Annotate debugger breakpoints and debug location on the scrollbar

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Screenshot of breakpoint and debugger scrollbar annotation

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=698f11a8dff5

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Screenshot of search matches annotation

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

@helen, Hi, I attached 2 screenshots of the scrollbar annotations for search matches, breakpoints and debugger pointer. Also, you will find a try link to the build for further testing. We want to evaluate what we should do with these scrollbar annotations.

Questions:
- Should we have scrollbar annotations for search matches?
- Should we have scrollbar annotations for breakpoints and debugger pointer?
- How should we style these scrollbar annotations?

Ideas:
- Should we scroll into view when we click on these annotations? Perhaps down the line?

Comment 16

[Tim Nguyen :ntim]

Looks nice ! How does it look like with the dark theme floating scrollbars though ?

Comment 17

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #15)
> Questions:
> - Should we have scrollbar annotations for search matches?
> - Should we have scrollbar annotations for breakpoints and debugger pointer?
> - How should we style these scrollbar annotations?
I like this and I think we should have them. I would ask :jlongster/:ejpbruel/:linclark in IRC if they have any strong feelings as to why we shouldn't.

> Ideas:
> - Should we scroll into view when we click on these annotations? Perhaps
> down the line?
I don't think this is necessary. They'd be very hard to click, and frustrating if you're trying to just use the scrollbar. To be certain I'd ask in #accesibility if they know of any pitfalls when it comes to changing normal scrollbar functionality.

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #17)
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #15)
> > Questions:
> > - Should we have scrollbar annotations for search matches?
> > - Should we have scrollbar annotations for breakpoints and debugger pointer?
> > - How should we style these scrollbar annotations?
> I like this and I think we should have them. I would ask
> :jlongster/:ejpbruel/:linclark in IRC if they have any strong feelings as to
> why we shouldn't.
> 

I think I will give a demo during the webconsole and debugger meeting to solicit feedback there. I don't think there is anyone against it.

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Tim Nguyen [:ntim] from comment #16)
> Looks nice ! How does it look like with the dark theme floating scrollbars
> though ?

Doesn't work on dark theme yet because of floating scrollbars.

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add new CodeMirror search addons [3.0]

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Add new CodeMirror scroll addons [4.0]

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Patch CodeMirror's annotatescrollbar to work with floating scrollbars [1.0]

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Add annotateScrollbar and clearScrollbarAnnotations methods to editor [1.0]

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Annotate debugger breakpoints and debug location on the scrollbar [1.0]

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Patch CodeMirror's annotatescrollbar to work with floating scrollbars [2.0]

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Part 1 and 2 - only adds the necessary new addons for this feature set. 

Part 3 - Patches the annotatescrollbar.js addon by removing the cm.display.barWidth check and hardcoding the scrollbar annotation to a width of 14px. We also add a container in the CM element to container these scrollbar annotations. We want to be able to clear all these scrollbar annotations in the future for scenarios such as switching sources in the debugger.

Part 4 - The new editor annotateScrollbar method is a bit controversial. To explain further:
cm.annotationScrollar({ className }) returns an Annotation object that would take a list of CM.Pos({ line, ch }) in Annotation.update(<list of Pos>) and draw the scrollbar annotations. You can see an example of this in the matchesonscrollbar addon. 

In the new method, we would create a new Annotation object for each single annotation. The only benefit of this was that it was easier to manage the annotations because we don't have to manage a list of CM.Pos. For instance, going through the list of CM.Pos to remove the Pos of a breakpoint that was removed.

I am not quite sure if this is the best solution. 

Part 5 - Implements the breakpoint annotations and clear the annotations when we switch sources.

Comment 27

[Brian Grinstead [:bgrins]]

It looks like syntax highlighting is broken with these patches applied (haven't double checked but it's something I noticed when applying the patches)

Comment 28

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add new CodeMirror search addons [4.0]

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Add new CodeMirror scroll addons [5.0]

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Patch CodeMirror's annotatescrollbar to work with floating scrollbars [3.0]

Comment 31

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Add annotateScrollbar and clearScrollbarAnnotations methods to editor [2.0]

Comment 32

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Annotate debugger breakpoints and debug location on the scrollbar [2.0]

Comment 33

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> It looks like syntax highlighting is broken with these patches applied
> (haven't double checked but it's something I noticed when applying the
> patches)

This seems to be working for me. I did have to rebase the first 2 patches.

Comment 34

[Brian Grinstead [:bgrins]]

So, the code changes look relatively straightforward here.  I think we should have a product and UX consensus on this new feature if possible.  Especially, I'm interested in what this looks like on Windows + light theme where there are no floating scrollbars.

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #19)
> (In reply to Tim Nguyen [:ntim] from comment #16)
> > Looks nice ! How does it look like with the dark theme floating scrollbars
> > though ?
> 
> Doesn't work on dark theme yet because of floating scrollbars.

I was thinking it *would* work on the dark theme because of floating scrollbars - I might be confused though.  I'd love if we could have screenshots of what this looks like on light/dark theme on Windows and OSX.  Gabe, do you have a setup to do that?

Also including Bryan to make sure he's aware of this and has a chance to give feedback.  One thing I'd suggest is that we have this feature be globally preffable through about:config (though not through the options UI).

Comment 35

[Brian Grinstead [:bgrins]]

Comment on attachment 8720099 [details] [diff] [review]
Part 1: Add new CodeMirror search addons [4.0]

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

::: devtools/client/sourceeditor/codemirror/addon/search/matchesonscrollbar.css
@@ +1,3 @@
> +.CodeMirror-search-match {
> +  background: gold;
> +  border-top: 1px solid orange;

We should override this border-color with our own theme color in mozilla.css, right?

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8720104 [details] [diff] [review]
Part 3: Patch CodeMirror's annotatescrollbar to work with floating scrollbars [3.0]

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

Why is this needed?

::: devtools/client/sourceeditor/codemirror/addon/scroll/annotatescrollbar.js
@@ -96,5 @@
>        if (bottom == top) continue;
>        var height = Math.max(bottom - top, 3);
>  
>        var elt = frag.appendChild(document.createElement("div"));
> -      elt.style.cssText = "position: absolute; right: 0px; width: " + Math.max(cm.display.barWidth - 1, 2) + "px; top: "

Could we set barWidth instead of needing to patch the file?

::: devtools/client/sourceeditor/editor.js
@@ +402,5 @@
>        });
>  
> +      // Add a container for scrollbar annotation
> +      let doc = cm.getWrapperElement().ownerDocument;
> +      let annotations = doc.createElement("div");

Not sure I follow the reasoning for this separate container in Comment 26.  Can you elaborate?

Comment 37

[Brian Grinstead [:bgrins]]

Comment on attachment 8720106 [details] [diff] [review]
Part 4: Add annotateScrollbar and clearScrollbarAnnotations methods to editor [2.0]

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

::: devtools/client/sourceeditor/editor.js
@@ +1222,5 @@
> +  /**
> +   * Clear the scrollbar annotations.
> +   */
> +  clearScrollbarAnnotations: function() {
> +    let cm = editors.get(this);

The CM annotation plugin doesn't have any way to remove all annotations?  It looks like removing from the DOM might not end up dropping an event handler that's bound here and removed in clear: http://codemirror.net/addon/search/matchesonscrollbar.js.  If that's the case we might need to keep a Map of the current annotations and remove them all on clear()

Comment 38

[Brian Grinstead [:bgrins]]

Comment on attachment 8720107 [details] [diff] [review]
Part 5: Annotate debugger breakpoints and debug location on the scrollbar [2.0]

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

I think someone familiar with the debugger should also take a look at this.  I'd usually suggest James but I know he's busy right now, so maybe Eddy can do a look-over?

::: devtools/client/sourceeditor/codemirror/mozilla.css
@@ +82,5 @@
>    background-color: rgba(44,187,15,.1);
>  }
>  
> +.breakpoint-annotation {
> +  background: #46AFE3;

Colors here should probably use theme variables (or at least create these constants as variables at the top of the file above).

Comment 39

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #36)
> Comment on attachment 8720104 [details] [diff] [review]
> Part 3: Patch CodeMirror's annotatescrollbar to work with floating
> scrollbars [3.0]
> 
> Review of attachment 8720104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why is this needed?
> 
> ::: devtools/client/sourceeditor/codemirror/addon/scroll/annotatescrollbar.js
> @@ -96,5 @@
> >        if (bottom == top) continue;
> >        var height = Math.max(bottom - top, 3);
> >  
> >        var elt = frag.appendChild(document.createElement("div"));
> > -      elt.style.cssText = "position: absolute; right: 0px; width: " + Math.max(cm.display.barWidth - 1, 2) + "px; top: "
> 
> Could we set barWidth instead of needing to patch the file?
> 
> ::: devtools/client/sourceeditor/editor.js
> @@ +402,5 @@
> >        });
> >  
> > +      // Add a container for scrollbar annotation
> > +      let doc = cm.getWrapperElement().ownerDocument;
> > +      let annotations = doc.createElement("div");
> 
> Not sure I follow the reasoning for this separate container in Comment 26. 
> Can you elaborate?

Specifically, if we need to take a different approach like in Comment 37 then maybe we don't need to patch this file anymore?

Comment 40

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #34)
> So, the code changes look relatively straightforward here.  I think we
> should have a product and UX consensus on this new feature if possible. 
> Especially, I'm interested in what this looks like on Windows + light theme
> where there are no floating scrollbars.
> 
> (In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #19)
> > (In reply to Tim Nguyen [:ntim] from comment #16)
> > > Looks nice ! How does it look like with the dark theme floating scrollbars
> > > though ?
> > 
> > Doesn't work on dark theme yet because of floating scrollbars.
> 
> I was thinking it *would* work on the dark theme because of floating
> scrollbars - I might be confused though.  I'd love if we could have
> screenshots of what this looks like on light/dark theme on Windows and OSX. 
> Gabe, do you have a setup to do that?

Another option instead of pulling down a Windows build would be to enhance the devtools mozscreenshot set to include setting a couple of breakpoints on the test page then we could have a full UI comparison across platforms.  If you want to take that on it can be run locally with `./mach mochitest browser/tools/mozscreenshots/devtools/browser_devtools.js` and changed by modifying https://dxr.mozilla.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/DevTools.jsm to set some breakpoints in the debugger before taking the screenshot.  Then the set could be run in automation with `-u mochitest-browser-screenshots[Ubuntu,10.6,10.10,Windows XP,Windows 7,Windows 8] --setenv MOZSCREENSHOTS_SETS=DevTools`

Comment 41

[James Long (:jlongster)]

I think Eddy asked me to take this review, but I don't see any pending reviews? Feel free to r? me

Comment 42

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to Brian Grinstead [:bgrins] from comment #34)
> Also including Bryan to make sure he's aware of this and has a chance to
> give feedback.  One thing I'd suggest is that we have this feature be
> globally preffable through about:config (though not through the options UI).

This looks good to me.  The debugger breakpoint listing change is great.  And I think the filter option will improve our search input.

If we're unsure how this will perform or if its ready for prime time lets hide it behind a pref at first or limit it to Nightly.