729480 - Link to rule in Style Editor should show the relevant line at the top of the editor's viewport

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Cedric Vivier [:cedricv]]

Attachment: screenshot - only the first line of the rule is visible (usually the selector)

Currently when clicking a rule source location in the Style Inspector, it jumps to the corresponding line in the Style Editor correctly, but that line is at the bottom of the editor's viewport - which makes it a bit less user-friendly that it could be (see screenshot).

The corresponding/highlighted line should be at the top of the editor's viewport instead, so that the user can directly see the rule properties without scrolling.

Comment 1

[Mihai Sucan [:msucan]]

This needs a fix in the Source Editor: we need to center the line that it jumps to.

Comment 2

[Cedric Vivier [:cedricv]]

As discussed on IRC, we might want a new API or extension to existing API so that a jump to a certain line can be optionally at top or bottom of the viewport.

top|bottom options should be in practice top+3 and bottom-3 to give a bit of context to the user.

For this particular bug, we might want top+3 rather than center.

Comment 3

[Girish Sharma [:Optimizer]]

I think I got the solution working locally, but it will take a while for me to create a patch as my internet connection is really slow, and the Firefox source is quite big.

Comment 4

[Girish Sharma [:Optimizer]]

IMO, there is not much change needed in the source editor part of the code, source editor already has a way to move the top visible line, thus only the style editor's code has to be changed to automatically decide the line and make it the top visible (or in this case top + 3).

I propose a fifth argument in the args list while opening styleeditor.xul named 'offset' which will have a positive numeric value to make the line have that certain offset from top of view port, or negative numeric value for offset from bottom of view port, or null for centrally aligned.

or

If we do not want so much flexibility, the 'offset' can be an enum with one of three values {top: 0, center: 1, bottom: 2}, where top and bottom here would mean top + 3 and bottom - 3 , as discussed above.

Thus a new function to sourceeditor has to be created which takes this offset and sets the viewport accordingly.

Comment 5

[Mihai Sucan [:msucan]]

Girish: Thank you for taking this bug!

I think this is sufficiently common that it should be supported by the Source Editor. setCaretPosition() should be able to center the line, or to put it at the top or bottom.

So, I suggest adding a third optional argument to setCaretPosition() in the source editor: aVerticalAlignment. Where that can be 0 (top), 1 (center) or 2 (bottom). I don't think we currently need to give more control to the alignment, to the style editor.

In the source editor you call this.setTopIndex() as needed for the given aVerticalAlignment. Default should be 0 (top). Always leave 3 visible lines at the top (or at the bottom).

The style editor code won't need an update since top will be the default.

Looking forward to your patch! Thanks!

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.0

Provides a way to set the view port of the line to top center or bottom
top and bottom are actually top+3 and bottom-3 except for the case when there are less than 5 lines visible in the view port, for that case, top and bottom are true top and bottom

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 603254 [details] [diff] [review]
Patch v1.0

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

This looks great Girish! Thanks for your patch!

General comments:

- please add a constant at the top of the file for the top/bottom vertical offset, which is by default 3 lines. This makes it easier for us to configure later how many lines we want be always visible when scrolling to top/bottom.

- for the bottom case you don't need lines visible. You can just do setBottomIndex(aLine + offset).

- you don't have to special case the code for the number of visible lines in the top/bottom cases. What you can do is to use Math.max/min to limit the offset so setTopIndex() won't make the target line invisible. If the editor is not tall enough, with a big enough offset the target line can go out of view once setTopIndex() is called. So, just limit that, with no special casing for each vAlign - and we also don't want a hard-coded limit of 5 lines.

Thanks! This is great! Looking forward for the updated patch!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +132,5 @@
> +const LINE_VIEW_ALIGN = {
> +  top: 0,
> +  center: 1,
> +  bottom: 2,
> +};

These constants are not available to other scripts when the Source Editor is imported.

Please put these constants in source-editor.jsm on the SourceEditor object, something like SourceEditor.VERTICAL_ALIGN.

@@ +1226,5 @@
>     * @param number [aColumn=0]
>     *        Optional. The new caret column location. Columns start from 0.
> +   * @param number [aAlign=0]
> +   *        Optional. Position of the line wrt to viewport
> +   *        @see LINE_VIEW_ALIGN for posible values

nits: please use periods at the end of sentences and do not use abbreviations like "wrt". ;)

@@ +1233,4 @@
>    {
>      this.setCaretOffset(this._model.getLineStart(aLine) + (aColumn || 0));
> +
> +    // See bug 729480 for more information on the code below

Not sure we need this comment. hg blame tells where the code comes from.

@@ +1238,5 @@
> +    let lineHeight = this._view.getLineHeight();
> +    let linesVisible = Math.floor(editorHeight/lineHeight);
> +
> +    if (aAlign == null)
> +      aAlign = LINE_VIEW_ALIGN.top;

nit: code style requires the curly braces even around one liners like these.

@@ +1253,5 @@
> +        }
> +        break;
> +
> +      case LINE_VIEW_ALIGN.center:
> +        topIndex = Math.max(aLine - linesVisible/2, 0);

You probably need a Math.round() here, because linesVisible/2 won't always be an integer.

@@ +1267,5 @@
> +        }
> +        break;
> +
> +      default:
> +        topIndex = aLine - 4;

This is a fourth case, actually. We want only 3 cases. So, please make the default case to be top - move the code from case LINE_VIEW_ALIGN.top to the default case here.

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #7)
> - please add a constant at the top of the file for the top/bottom vertical
> offset, which is by default 3 lines. This makes it easier for us to
> configure later how many lines we want be always visible when scrolling to
> top/bottom.

Will do.

> - for the bottom case you don't need lines visible. You can just do
> setBottomIndex(aLine + offset).

you meant setTopIndex(aLine + offset), right? as there is no such function as setBottomIndex() (at least I can't find one)
 
> - you don't have to special case the code for the number of visible lines in
> the top/bottom cases. What you can do is to use Math.max/min to limit the
> offset so setTopIndex() won't make the target line invisible. If the editor
> is not tall enough, with a big enough offset the target line can go out of
> view once setTopIndex() is called. So, just limit that, with no special
> casing for each vAlign - and we also don't want a hard-coded limit of 5
> lines.

What if there are in total 4 lines visible, then top would actually be same as bottom. We have to handle it by hardcoding , imo.

> @@ +1226,5 @@
> >     * @param number [aColumn=0]
> >     *        Optional. The new caret column location. Columns start from 0.
> > +   * @param number [aAlign=0]
> > +   *        Optional. Position of the line wrt to viewport
> > +   *        @see LINE_VIEW_ALIGN for posible values
> 
> nits: please use periods at the end of sentences and do not use
> abbreviations like "wrt". ;)

will do.

> @@ +1233,4 @@
> >    {
> >      this.setCaretOffset(this._model.getLineStart(aLine) + (aColumn || 0));
> > +
> > +    // See bug 729480 for more information on the code below
> 
> Not sure we need this comment. hg blame tells where the code comes from.

Cool. Will delete then.

> @@ +1238,5 @@
> > +    let lineHeight = this._view.getLineHeight();
> > +    let linesVisible = Math.floor(editorHeight/lineHeight);
> > +
> > +    if (aAlign == null)
> > +      aAlign = LINE_VIEW_ALIGN.top;
> 
> nit: code style requires the curly braces even around one liners like these.

Okay.
 
> @@ +1253,5 @@
> > +        }
> > +        break;
> > +
> > +      case LINE_VIEW_ALIGN.center:
> > +        topIndex = Math.max(aLine - linesVisible/2, 0);
> 
> You probably need a Math.round() here, because linesVisible/2 won't always
> be an integer.

Hmm.

> @@ +1267,5 @@
> > +        }
> > +        break;
> > +
> > +      default:
> > +        topIndex = aLine - 4;
> 
> This is a fourth case, actually. We want only 3 cases. So, please make the
> default case to be top - move the code from case LINE_VIEW_ALIGN.top to the
> default case here.

This case should not be accesible , but I kept it just for the sake of switch.

Will address comments in few minutes and update the patch with the test.

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: Patch v2.0

Addressed previous review comments.

Changing the threshold for top so that top is in upper half of visible lines (as discussed on irc)

Comment 10

[Girish Sharma [:Optimizer]]

(In reply to Girish Sharma from comment #9)
> Created attachment 603437 [details] [diff] [review]
> Patch v2.0
> 
> Addressed previous review comments.
> 
> Changing the threshold for top so that top is in upper half of visible lines
> (as discussed on irc)

Also added a testcase for the bug.

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: Patch v2.1

Forgot to add myself as a contributor, as told by msucan

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: Patch v2.2

minor comment fix.

Comment 13

[Girish Sharma [:Optimizer]]

Attachment: Patch v2.3

Forgot to declare the top offset const

Comment 14

[Girish Sharma [:Optimizer]]

Attachment: Patch v3.0

Updated the patch with successful test.

Comment 15

[Cedric Vivier [:cedricv]]

(In reply to Girish Sharma from comment #13)
> Created attachment 603450 [details] [diff] [review]
> Patch v2.3
> Forgot to declare the top offset const

IMHO the VERTICAL_ALIGN enum should go for strings instead of integers, as this is the general way forwards in web APIs.

If we prefer to keep integers, then we should have top=1 instead of top=0 so that the API is flexible to use combinations later (which is the 'only' benefit of using integers here).

Comment 16

[Girish Sharma [:Optimizer]]

(In reply to Cedric Vivier [:cedricv] from comment #15)
> (In reply to Girish Sharma from comment #13)
> > Created attachment 603450 [details] [diff] [review]
> > Patch v2.3
> > Forgot to declare the top offset const
> 
> IMHO the VERTICAL_ALIGN enum should go for strings instead of integers, as
> this is the general way forwards in web APIs.


The integers should never be in direct use as such , as I have provided a way to use editor.VERTICAL_ALIGN.TOP/BOTTOM/CENTER. People should use only this , and not directly 0,1 or 2.

> If we prefer to keep integers, then we should have top=1 instead of top=0 so
> that the API is flexible to use combinations later (which is the 'only'
> benefit of using integers here).

What type of combinations ? Sorry, I didn't get you there.

Comment 17

[Cedric Vivier [:cedricv]]

(In reply to Girish Sharma from comment #16)
> The integers should never be in direct use as such , as I have provided a
> way to use editor.VERTICAL_ALIGN.TOP/BOTTOM/CENTER. People should use only
> this , and not directly 0,1 or 2.

Yes.. still this is the values that are passed to the function, which has readability/debuggability issues (ie. that's why Web APIs deprecates using integer enums).

Also for consistency, the existing enums in SourceEditor are string-based not integer-based (ie. ORION_EVENTS and ORION_ANNOTATION_TYPES).


> > If we prefer to keep integers, then we should have top=1 instead of top=0 so
> > that the API is flexible to use combinations later (which is the 'only'
> > benefit of using integers here).
> 
> What type of combinations ? Sorry, I didn't get you there.

eg.
VERTICAL_ALIGN.TOP | HORIZONTAL_ALIGN.END

if we ever need that (if we stay with integers we should at least use them in a way that can use the 'only' advantage of integer-enum)

Comment 18

[Girish Sharma [:Optimizer]]

I have no problem in shifting to string literals if msucan says so in his review :)

Comment 19

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma from comment #8)
> (In reply to Mihai Sucan [:msucan] from comment #7)
> > - please add a constant at the top of the file for the top/bottom vertical
> > offset, which is by default 3 lines. This makes it easier for us to
> > configure later how many lines we want be always visible when scrolling to
> > top/bottom.
> 
> Will do.
> 
> > - for the bottom case you don't need lines visible. You can just do
> > setBottomIndex(aLine + offset).
> 
> you meant setTopIndex(aLine + offset), right? as there is no such function
> as setBottomIndex() (at least I can't find one)

I knew Orion had getBottomIndex() and expected it also has setBottomIndex() - never really needed these methods. My bad.

So, then ignore my request here. For vertical align bottom you need to do setTopIndex(aLine - lineVisible + offset).

> > - you don't have to special case the code for the number of visible lines in
> > the top/bottom cases. What you can do is to use Math.max/min to limit the
> > offset so setTopIndex() won't make the target line invisible. If the editor
> > is not tall enough, with a big enough offset the target line can go out of
> > view once setTopIndex() is called. So, just limit that, with no special
> > casing for each vAlign - and we also don't want a hard-coded limit of 5
> > lines.
> 
> What if there are in total 4 lines visible, then top would actually be same
> as bottom. We have to handle it by hardcoding , imo.

You can do:

offset = Math.min(linesVisible/2, offset);
topIndex = aLine - offset

This ensures that topIndex won't be in the lower half of the editor when you say TOP. Do something similar for bottom:

offset = Math.min(linesVisible/2, offset);
topIndex = aLine - linesVisible + offset

(This is just example code, I didn't test it, might need adjustments, and so on. One thing I want is Math.round() for linesVisible/2.)

This avoids any hard-coded special-casing.

Thanks for your work!

Comment 20

[Girish Sharma [:Optimizer]]

I am using Math.round for linesVisible/2 in the latest patch.
Also, I removed the scope of hardcoding, when linesVisible/2 < offset, then I drop the offset and make it absolute top.

Also, I addresed you comment and made bottom as absolute bottom.

Comment 21

[Mihai Sucan [:msucan]]

Comment on attachment 603640 [details] [diff] [review]
Patch v3.0

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

This looks good. Thanks for your patch!

Please address the comments below. It would be awesome if we can get this in Firefox 13!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +129,5 @@
>  
> +/**
> + * Default top offset used by setCaretPosition.
> + */
> +const TOP_OFFSET = 3;

Please put this constant before ORION_THEMES where other settings are found (like PRIMARY_SELECTION_DELAY).

@@ +1215,5 @@
>    },
>  
>    /**
>     * Set the caret position: line and column.
> +   * Also aligns the line with respect to the view port as per a third argument.

This is not needed.

@@ +1223,5 @@
>     * @param number [aColumn=0]
>     *        Optional. The new caret column location. Columns start from 0.
> +   * @param number [aAlign=0]
> +   *        Optional. Position of the line with respect to viewport.
> +   *        @see SourceEditor.VERTICAL_ALIGN in source-editor.jsm for posible values.

Do not use "@" for see. Please list the allowed vertical alignment options.

Also you have a typo: s/posible/possible/

@@ +1247,5 @@
> +          topIndex = aLine;
> +        } else {
> +          // otherwise top - TOP_OFFSET should be the first visible line
> +          topIndex = Math.max(aLine - TOP_OFFSET, 0);
> +        }

You can simplify this with the approach suggested in 19.

@@ +1255,5 @@
> +        topIndex = Math.round(Math.max(aLine - linesVisible/2, 0));
> +        break;
> +
> +      case this.VERTICAL_ALIGN.BOTTOM:
> +        topIndex = Math.max(aLine - linesVisible, 0);

This needs to allow an offset at the bottom - just like TOP_OFFSET.

Maybe rename TOP_OFFSET to VERTICAL_OFFSET (not sure what better name to use). Reuse this constant for both top and bottom vertical alignments. See comment 19.

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +311,5 @@
>    DIRTY_CHANGED: "DirtyChanged",
>  };
>  
>  /**
> + * Allowable positions for a line's offset while using setCaretPosition

Allowed vertical alignment options for the line index when you call SourceEditor.setCaretPosition().

::: browser/devtools/sourceeditor/test/browser_bug729480_line_vertical_align.js
@@ +15,5 @@
> +{
> +  let component = Services.prefs.getCharPref(SourceEditor.PREFS.COMPONENT);
> +  if (component == "textarea") {
> +    ok(true, "skip test for bug 729480: not applicable for TEXTAREAs");
> +    return; // TEXTAREAs have different behavior

Comment not needed.

@@ +40,5 @@
> +
> +  let text = "line 0\n" +
> +             "\n" +
> +             "line 2\n" +
> +             "line 3\n" +

This is not the ideal approach here. Please use no initialText.

Look into browser_bug687568_pagescroll.js how you can add the number of lines needed to fill how many scroll pages you want.

@@ +97,5 @@
> +
> +  // setting to line 0 will set line 0 at top of view
> +  editor.setCaretPosition(0, 0, editor.VERTICAL_ALIGN.TOP);
> +  executeSoon(function () {
> +    is(editor.getTopIndex(), 0, "caret location at start of line 0 top aligned with line 0 at absolute top of view");

This test code is too deeply nested. What I propose is you make an array of things to test: in each array element note the target line, the expected top index and the vertical alignment. Then loop through the array. Make a generic function you call to test the source editor, for each element in the array - an iterator.

Comment 22

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma from comment #20)
> I am using Math.round for linesVisible/2 in the latest patch.
> Also, I removed the scope of hardcoding, when linesVisible/2 < offset, then
> I drop the offset and make it absolute top.
>
> Also, I addresed you comment and made bottom as absolute bottom.

Might have been a confusion wrt. absolute bottom. Bottom also needs the vertical offset, like top.

Comment 23

[Girish Sharma [:Optimizer]]

Attachment: Patch v4.0

Patch on latest m-c code.
Addresses comments from review of patch v3.0

Comment 24

[Mihai Sucan [:msucan]]

Comment on attachment 604194 [details] [diff] [review]
Patch v4.0

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

Thanks for the updated patch Girish! This is cool!

Found one bug: click on the lines ruler. It always jumps to top. This is because the current code always does the vertical alignment. To fix this we need to not scroll the editor when the target line is already visible.

How to implement this elegantly: do not call this.setCaretOffset() because that scrolls the editor with its default logic. Store the target offset in a variable, then call this._view.setSelection(offset, offset, false) - this is a direct call to the Orion API to move the cursor to the offset we want, but the third argument tells it to not show/scroll to the selection. We handle scrolling.

To handle scrolling add a bit more logic: get the current top index and the current bottom index (getBottomIndex). See if the target line index is out of the visible range of lines. If yes, apply the current logic of scrolling to the target line index.

Please also test this case.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +65,5 @@
>  const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>  
>  /**
> + * Maximum allowed vertical offset for the line index when you call
> + * SourceEditor.setCaretPosition()

nit: end comment with a period.

@@ +1350,5 @@
> +        topIndex = Math.round(Math.max(aLine - linesVisible/2, 0));
> +        break;
> +
> +      case this.VERTICAL_ALIGN.BOTTOM:
> +        offset = Math.min(Math.round(linesVisible/2), VERTICAL_OFFSET);

The |offset| variable is the same for both TOP and BOTTOM. Please put this outside of the switch.

The |linesVisible| value is halved in all three cases. You can do this just once, before the switch.

You do |Math.max(foo, 0)| for all three cases. You can do this after the switch. Also, you should do a similar check to not go beyond |getLineCount()|.

::: browser/devtools/sourceeditor/test/browser_bug729480_line_vertical_align.js
@@ +15,5 @@
> +function test()
> +{
> +  let component = Services.prefs.getCharPref(SourceEditor.PREFS.COMPONENT);
> +  if (component == "textarea") {
> +    ok(true, "skip test for bug 729480: not applicable for TEXTAREAs");

This check is no longer needed. The textarea component has been removed.

@@ +43,5 @@
> +  let config = {
> +    showLineNumbers: true,
> +  };
> +
> +  editor = new SourceEditor();

Two empty lines before |config| and you can inline the object when you call |new SourceEditor()|.

@@ +79,5 @@
> +    [5, editor.VERTICAL_ALIGN.BOTTOM, 0],
> +    [38, editor.VERTICAL_ALIGN.BOTTOM, 38 - linesPerPage + offset],
> +    [0, editor.VERTICAL_ALIGN.CENTER, 0],
> +    [4, editor.VERTICAL_ALIGN.CENTER, 0],
> +    [24, editor.VERTICAL_ALIGN.CENTER, 24 - Math.round(linesPerPage/2)]

You can make this look simpler by just using "TOP"/"CENTER"/"BOTTOM". In you iterator you can do editor.VERTICAL_ALIGN[foo].

@@ +101,5 @@
> +                                                  + " as first visible line");
> +        iterator(++i);
> +      });
> +    }
> +  }

You can simplify the iterator() by moving the code from i == array.length into a separate function, like testEnd() that you call. Also, the executeSoon() function have can be in a separate function. You can do executeSoon(testPosition.bind(null, i)) where testPosition() takes one argument - the index in the array to test.

Anyhow, this is fancy. Great work!

Comment 25

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #24)
> Found one bug: click on the lines ruler. It always jumps to top. This is
> because the current code always does the vertical alignment. To fix this we
> need to not scroll the editor when the target line is already visible.
> 
> How to implement this elegantly: do not call this.setCaretOffset() because
> that scrolls the editor with its default logic. Store the target offset in a
> variable, then call this._view.setSelection(offset, offset, false) - this is
> a direct call to the Orion API to move the cursor to the offset we want, but
> the third argument tells it to not show/scroll to the selection. We handle
> scrolling.
> 
> To handle scrolling add a bit more logic: get the current top index and the
> current bottom index (getBottomIndex). See if the target line index is out
> of the visible range of lines. If yes, apply the current logic of scrolling
> to the target line index.
 
Yes, right, will do it.

> The |offset| variable is the same for both TOP and BOTTOM. Please put this
> outside of the switch.
> 
> The |linesVisible| value is halved in all three cases. You can do this just
> once, before the switch.
> 
> You do |Math.max(foo, 0)| for all three cases. You can do this after the
> switch. Also, you should do a similar check to not go beyond
> |getLineCount()|.
 
Will combine as much as possible.

> This check is no longer needed. The textarea component has been removed.

I had no idea what this does :|. Will remove.

> Two empty lines before |config| and you can inline the object when you call
> |new SourceEditor()|.

sure.

> You can make this look simpler by just using "TOP"/"CENTER"/"BOTTOM". In you
> iterator you can do editor.VERTICAL_ALIGN[foo].

hmm, true.

> > +        iterator(++i);
> > +      });
> > +    }
> > +  }
> 
> You can simplify the iterator() by moving the code from i == array.length
> into a separate function, like testEnd() that you call. Also, the
> executeSoon() function have can be in a separate function. You can do
> executeSoon(testPosition.bind(null, i)) where testPosition() takes one
> argument - the index in the array to test.
> 
> Anyhow, this is fancy. Great work!

Even though executeSoon is async, I need to call the next setCaretPosition after the is() check in th eprevious executeSoon, thats why things need to be nested and in synchronous manner. If this method is ok, can I carry on with this only ?

Comment 26

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma from comment #25)
> (In reply to Mihai Sucan [:msucan] from comment #24)
> > 
> > You can simplify the iterator() by moving the code from i == array.length
> > into a separate function, like testEnd() that you call. Also, the
> > executeSoon() function have can be in a separate function. You can do
> > executeSoon(testPosition.bind(null, i)) where testPosition() takes one
> > argument - the index in the array to test.
> > 
> > Anyhow, this is fancy. Great work!
> 
> Even though executeSoon is async, I need to call the next setCaretPosition
> after the is() check in th eprevious executeSoon, thats why things need to
> be nested and in synchronous manner. If this method is ok, can I carry on
> with this only ?

In iterator() you keep the editor.setCaretPosition(). You just move the executeSoon() function into a separate one. That one will call the next iterator() step - like it does now. Nothing should break.


Something like this should work (quick copy/paste code, most-likely broken):

function testPosition (pos) {
+        is(editor.getTopIndex(), iterateOn[pos][2], "Caret at line " + iterateOn[pos][0]
+                                                  + " aligned " + (iterateOn[pos][1] == 0
+                                                  ? "to top": iterateOn[pos][1] == 1? "centrally"
+                                                  : "to bottom") + " with line "
+                                                  + iterateOn[pos][2]
+                                                  + " as first visible line");
+        iterator(++pos);
+      }

+  function iterator(i) {
+    if (i == iterateOn.length) {
+      testEnd();
+    }
+    else {
+      editor.setCaretPosition(iterateOn[i][0], 0, iterateOn[i][1]);
+      executeSoon(testPosition.bind(this. i));
+    }
+  }
+  iterator(0);

function testEnd() {
+  editor.destroy();
+      testWin.close();
+      testWin = editor = null;
+      waitForFocus(finish, window);
}

Comment 27

[Girish Sharma [:Optimizer]]

Attachment: Patch v4.5

Not scrolling when the target line already in view.

Comment 28

[Girish Sharma [:Optimizer]]

Attachment: Patch v4.6

Minor comment understanding issue

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 604487 [details] [diff] [review]
Patch v4.6

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

This is awesome work Girish! Thank you!

One global nit: there are some lines in the patch that need to wrap at 80 chars. Please make sure the patch doesn't have lines that are too long.

Giving r+ with the comments below addressed (horizontal scrolling and handling of unknown aAlign values).

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1337,5 @@
> +    this._view.setSelection(caretOffset, caretOffset, false);
> +
> +    // If the target line is in view, skip the vertical alignment part.
> +    if (aLine < firstVisible + linesVisible && aLine >= firstVisible) {
> +      return;

We need to do horizontal scrolling, if the selection is not visible horizontally.

Before the return please call this._view.showSelection().

@@ +1361,5 @@
> +        break;
> +
> +      case this.VERTICAL_ALIGN.BOTTOM:
> +        topIndex = Math.max(aLine - linesVisible + offset, 0);
> +        break;

What happens when aAlign has an unknown value?

I suggest:

switch (aAlign) {
  case center:
    ...
  case bottom:
  default: // top
    ...
}

So top would always be the default, if aAlign is an unknown value. With this you don't even need the if (aAlign == null) { aAlign = TOP } that you have before the switch block.

@@ +1365,5 @@
> +        break;
> +    }
> +    // Bringing down the topIndex to total lines in the editor if exceeding.
> +    topIndex = Math.min(topIndex, this.getLineCount());
> +    this.setTopIndex(topIndex);

You also need a this._view.showSelection() call after setTopIndex() that will deal with horizontal scrolling to show the selection.

::: browser/devtools/sourceeditor/source-editor.jsm
@@ +313,5 @@
>  };
>  
>  /**
> + * Allowed vertical alignment options for the line index
> + * when you call SourceEditor.setCaretPosition()

Nit: period at the end.

::: browser/devtools/sourceeditor/test/browser_bug729480_line_vertical_align.js
@@ +86,5 @@
> +      + " aligned " + (iterateOn[pos][1] == 0
> +      ? "to top": iterateOn[pos][1] == 1? "centrally"
> +      : "to bottom") + " with line "
> +      + iterateOn[pos][2]
> +      + " as first visible line");

Nit: this message should be simplified to "scroll is correct for test #" + pos.

Comment 30

[Girish Sharma [:Optimizer]]

Attachment: Patch v4.7

Fixed Horizontal scrolling also.
Setting r+ and ready to land.

Comment 31

[Girish Sharma [:Optimizer]]

Please feel free to push it to fx-team.

Comment 32

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma from comment #30)
> Created attachment 604620 [details] [diff] [review]
> Patch v4.7
> 
> Fixed Horizontal scrolling also.
> Setting r+ and ready to land.

Thanks for your quick patch update. Please always test your changes. I just tested it and this is probably a bug in Orion or something: showSelection() causes vertical scrolling to happen - so the vertical offset is canceled. Meh. Tests fail.

Since this is unexpected behavior I looked into Orion API and made it work. At the end of setCaretPosition() instead of calling _view.showSelection() do this:

-    this._view.showSelection();
+
+    let location = this._view.getLocationAtOffset(caretOffset);
+    this._view.setHorizontalPixel(location.x);

This is what I did and it works for me. The getLocationAtOffset() tells us the x and y coordinates of the character offset where we want to scroll to (relative to the document). With setHorizontalPixel() does the scroll.

I also found a bug when testing: if you click on the last line in the ruler, it scrolls to top when setCaretPosition() is called. This is because the last line is not fully visible. To fix that I did:

     let firstVisible = this.getTopIndex();
+    let lastVisible = this._view.getBottomIndex();

-    if (aLine < firstVisible + linesVisible && aLine >= firstVisible) {
+    if (aLine <= lastVisible && aLine >= firstVisible) {

So that work fixes the bug because getBottomIndex() tells us the last visible index, even if it's only partially visible.

Thanks for your patience with fixing this bug. Great work!

Comment 33

[Girish Sharma [:Optimizer]]

Attachment: Patch v4.8

Sorry for the last patch, I thought that its a small change and it souded that it should not break , so I did not test it.

But this patch has been tested and it passed the test.

I leave you for any final check.

Comment 34

[Mihai Sucan [:msucan]]

Comment on attachment 604642 [details] [diff] [review]
Patch v4.8

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

This works. Thanks!

I pushed your patch along with other related patches to the try servers.

https://tbpl.mozilla.org/?tree=Try&rev=2451a0e62cc0

Once we get greens we can land.

Comment 35

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] Patch v4.8b

Minor change: if (aLine < lastVisible) becomes aLine <= lastVisible (otherwise clicking on the last line still jumps to the top).  Another minor change: for when aLine is within the visible lines we call showSelection() which does more than setHorizontalPixel(). In my previous comment I suggested using setHorizontalPixel() only for the case when we call setTopIndex().

Thanks for your contribution Girish!

Landed the patch:

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

Try results are green.

Comment 36

[Girish Sharma [:Optimizer]]

Great! .
I was thinking while traveling. Doesn't this feature needs to be imported to setSelection, setCaretOffset and other similar functions ?
Like when you find something in sourceeditor, it Highlights the found part of text and the highlighted part is shown in the last visible line.

Comment 37

[Girish Sharma [:Optimizer]]

http://hg.mozilla.org/mozilla-central/rev/28cf4b50717c

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/28cf4b50717c

Comment 39

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma from comment #36)
> Great! .
> I was thinking while traveling. Doesn't this feature needs to be imported to
> setSelection, setCaretOffset and other similar functions ?
> Like when you find something in sourceeditor, it Highlights the found part
> of text and the highlighted part is shown in the last visible line.

That's a cool idea! However I believe that's not needed for now. setCaretPosition() is the only method that deals with "go to line" - hence vertical alignment makes sense. The others work as expected.