The View Source window needs the capability to go to a specific line number in the source. Here is how such a feature might work: 1. The user selects Edit | Go to Line #... 2. The user types a line number in a field and clicks the Go button or presses the Enter key 3. The line is displayed and highlighted, with the window scrolled all the way to the left side so the beginning of the line can be seen, and perhaps the dialog is dismissed

Great idea. I couldn't find another request for this, so confirming. Also see bug 15364.

doron, I'll love you if you do this.

ui wise we can have a input box in the menubar. There is a window.scrollByLines() function (http://developer.netscape.com/evangelism/docs/technotes/xref/window-object/) which might work. bz even suggest if we have a view-source:url#45 to jump to line 45. Will look at it after sleep

I have this working, we need to decide for how to do the UI (I currently have it in the menubar).

Notepad [w2k] uses Edit>Goto which is fine by me, assuming we have no Search menu.

i need to bring back the search menu for viewsource since the naviagor search remocal was futured. can you take a screenshot of the noteapd goto dialog? win98 goto has none

In the windows 2k notepad the shortcut for goto line is ctrl-G and when the dialog opens up, the line number displayed in the textbox is the line number that you are currently on.

0.9.7 it is

future. I have this working, but it depends on 110506 getting fully fixed by me.

mass moving open bugs pertaining to view source to pmac@netscape.com as qa contact. to find all bugspam pertaining to this, set your search string to "ItsSharkeysNight".

do we want to call this "jump to line" or goto? Have this working, attaching what I have in a sec

How do we want to call it? Go to or jump to line? Patch hardcodes some text still.

> + * The Original Code is Jumo To Line Code. Who's "Jumo" ? :) Why is the little jump-to-line dialog a dialog=no? Why is it even a separate dialog, instead of being a textfield in the viewsource window (like the typein field in the JS console)? Also, I thought the idea was to pass a default line number to the view-source window and have it scroll to that line number onload... are we still planning to do that?

I've always seen this called Go to. I believe Win2K's Edit | Go To... menu makes sense. HomeSite 5 uses Edit | Go to Position... and gives you the option to indicate both the line number and the character position on the line. Of course it's an editor and not a viewer. One nice thing about the HomeSite dialog is that it gives you the valid range: Choose a line number (1-789) [ ]

I copied the find code :) For the default line number, that isn't yet done as I need to modify the js console, don't worry. If I could figure form js the number of lines, that would be cool and solve some issues. I think I have a rough idea of a possible way, but need to do more research on that.

I would say that if there was a way to display line numbers (see bug 15364), this functionality would not necessarily be needed. I would say that line numbers should be added to the source viewer before the "go to line" is added.

Doron, how does this patch deal with the "wrap long lines" mode? Mike, getting this to work is a _lot_ easier than displaying line numbers. We have two proposed fixes for this bug, none for that one yet...

I don't do any development on Mozilla, so I certainly can't argue what would be the easiest solution. If "jump to line" is added (without a display of line numbers on the left), how does the user identify the line once the window has scrolled to it. Is it supposed to be obvious that it is the line at the top of the source window (assuming that the window scrolls to the top of the indicated line)? Does the line become highlighted? There's no cursor to indicate the line in the manner that most editors might. What happens if you "jump to" the last line of the source, or a line near the end? The scrollbar for the viewer only scrolls down enough for the last line to appear at the bottom of the window. On my screen, that leaves close to 40 lines. If I were to "jump to" line 396 of 400 in a particular file, will I only see the last 5 lines in the window (with blank space below)? Or will there be some other way to indicate the line that I was searching for? I'm not trying to argue, I am just pointing out why I was thinking that displaying the line numbers on the left might help make the "jump to" feature a little more usable.

I haven't had time to log into linux and check, and won't till next week probably. As for wrapping, it doesn't know that lines were wrapped, so it will scroll to the nth line displayed. As for line numbers, venkman does it, need to look at that code

*** Bug 160209 has been marked as a duplicate of this bug. ***

setting to 1.2alpha btw, I am very inclined to replace the new->brower/mail/etc with just a "New Browser" menuitem, or even removing that totally As for wrapping, I'm using window.scrollto, so it has no idea about wrapping, so this might cause some issues when I get a jsconsole -> viewsource to linenumber patch (which I had, but lost).

> As for wrapping, I'm using window.scrollto, so it has no idea about wrapping Then maybe it's the wrong thing to use? If the line numbers in view source are going to have no resemblance to the line numbers in the JS console, what's the point of this patch?

I could always turn off wrapping :) reseting, as this is a valid concern.

so, would turning off wrapping if the caller is js console be evil?

Somewhat, yes. There's also the approach taken in bug 79612. We _could_ try to minimize the speed hit there...

it's not worth it for this feature to hack c++

That's a matter of your opinion, not fact.... I think it's perfectly worth it.

current patch doesn't select that line, but it's a start

Now using Mozilla 1.2 Javascript Console gives me this (just an example): Error: xxx is not defined Source File: http://localhost/file.asp Line: 86 I can click the link and the "Source of" window opens the file at the top. IMHO it would be a great start if the "Source of" window had the ability to move to line x and then highlight it (is there a bug for that). Leave the Goto Menu/Dialog stuff for later. (Ctrl-A, Ctrl-C, "open editor", Ctrl-V, Ctrl-G, is not fun)

> is there a bug for that Yes. This is the bug for that.

*** Bug 185572 has been marked as a duplicate of this bug. ***

... lot of bugs were marked as duplicate of this one, but there is NO RESULT. There is still no "GoToLine" or "JumToLine" in ViewSource ... so is there finally a possibility to add thes SIMPLY feature?

If it's so simple, add it. Maybe it's not as simple as you think, eh?

It was totally useless comentary because Mozilla-team solve this problem since Mozilla 0.9 and now we have 1.3a ...

Where exactly have we solved this problem?

future. to make this really work, we need line numbers, which would mean slowing down viewsource. Might have an idea how to speed viewsource a bit to hide the perf hit of line numbers, but I don't have time now to do that.

This patch finds the specified line the hard way by counting newlines. When the line (e.g. line 34) is found, it is enclosed in a <span class="selected-line" id="line-34"> element. If necessary, span elements that span several lines including the specified are split into two. The line can be styled by inserting some suitable CSS in /res/viewsource.css, e.g. .selected-line { border: 1px dotted black; } Currently it only searches for \n. I am not sure if \r should be handled as a newline as well. When syntax highlighting is on, the performance isn't impressing. With syntax highlighting turned on, however, there is no noticeable delay. The UI is still pretty rudimentary. It still needs localization and some kind of feedback when the line number entered is out of range. Currently the line number is entered in a regular JS prompt(). Personally I prefer a popup dialog, but another possibility is a fixed textfield in the viewsource window as suggested in comment 14. Comments?

> I am not sure if \r should be handled as a newline as well. \r is not a valid character in the DOM; there will never be \r in #text nodes unless someone sticks it in there via DOM methods (as in, the parser will never produce such text nodes). Would it possibly be faster to select all the #text nodes in the document with a treewalker and then stop iterating when you hit the n-th newline? Also, what sort of speed are we talking about here in the highlighted case? Would we be better off just using the patch in bug 79612 (perf hit opening view source every time) or the patch in this bug (per hit on every traversal to a line)? (This is a question about what we think the primary usage patter of such a function would be, really...) Thanks for picking this one up, Christian!

>Would it possibly be faster to select all the #text nodes in the document with >a treewalker and then stop iterating when you hit the n-th newline? I doesn't seem so. Infact is seems considerably (> +50%) slower. And it doesn't make the code easier to read either, because we still need to know which pre and span (if any) each text node is in (disclaimer: I have never worked with the treewalker before so I might have used it in a less-than-optimal way). >Also, what sort of speed are we talking about here in the highlighted case? I have changed the code so it now uses offsetHeight-calculations to count the number of lines when word wrap is off. Also the number of lines in each pre element is now cached, so when the user jumps to a line for the second time it is usually much faster. The following results show approximately how long time it takes to jump to line 2000 of a document on my old Pentium Celeron 300 MHz: word wrap OFF, syntax highlighting OFF < 1 sec word wrap OFF, syntax highlighting ON < 1 sec word wrap ON, syntax highlighting OFF ~ 3 sec word wrap ON, syntax highlighting ON ~15 sec >Would we be better off just using the patch in bug 79612 (perf hit opening view >source every time) or the patch in this bug (per hit on every traversal to a >line)? (This is a question about what we think the primary usage patter of >such a function would be, really...) Personally I'd use the go-to-line feature a lot, but I usually have word wrapping turned off, so I would probably prefer the patch in this bug. Judging from the number of votes and duplicates, this feature (surprisingly) doesn't seem that popular. However, I think the best solution would be a combination of the two patches. If the patch in bug 79612 is changed, so it doesn't insert nodes for every line but just one for every, say, 50 lines (perhaps it could just add an id attribute on each pre element indicating the line number of the last line in the pre), then the perf hit on window open probably wouldn't be that big. The patch in this bug can then find and highlight the exact line by counting newlines from the nearest of these target nodes. Unfortunately I don't really hack C++, so I can't change the patch to bug 79612 myself.

> (perhaps it could just add an id attribute on each pre element indicating the > line number of the last line in the pre) That is an interesting thought, now... Do you build Mozilla, by any chance? If I changed the patch in bug 79612 to do that, would you be able to test?

Yes, I build (on Linux). I tried applying the existing patch, but it seems to have bitrotted. But if you update and change it, I will try again and work from there.

This adds an id to all but the first "pre" element (I could add it to the first one too, if you really want, of course). The id has the form "lineXXX" where XXX is the line number of the first line in that pre. So: <pre> Text </pre> <pre id="line2"> text2 text3 </pre> <pre id="line4"> ... I've not done any perf testing on how long it takes to open viewsource with or without this patch; if someone has the time to do that, it would be much appreciated. I _think_ this one shouldn't really affect perf much...

Oh, and I did first line instead of last, because it's a _lot_ easier to do in the current setup... if having the last line number would be much much better, I'll try to look into doing that instead...

It now highlights the line in < 1 sec, independently of word wrapping and syntax highlighting. >Oh, and I did first line instead of last, because it's a _lot_ easier to do That's fine with me. Now, I am not sure how to move this further (this is my first real Mozilla hacking): - is regular JS prompt() and alert() calls the way to go, should I create a XUL dialog for entering the line number, or should the UI be totally different? - element.scrollIntoView() doesn't do what I expect (I can't find any authorative information about how it is supposed to react). When called on an span element containing a lot of lines down in a pre element, only the top of the pre element is scrolled into view, leaving the span element way below the viewport. This is particularly a problem when viewing source of documents containing a lot of inline Javascript, because a <script> block appears not to be split into seperate pre elements. Is this a bug in scrollIntoView()?

> is regular JS prompt() and alert() calls the way to go Not really, you should use nsIPromptService. see http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/public/nsIPromptService.idl

> element.scrollIntoView() doesn't do what I expect One possibility here is to use <a id="line-xxx" name="line-xxx"> </a> to enclose the line, to scroll to it by calling loadURI("#line-xxx") on the content webnavigation, and to style it by using the :target selector.... That said, is the problem that scrollIntoView is just scrolling to the top of the parent span that the span you added is in or something? Regular prompt() or alert() should not be called from chrome. You could use the ones on nsIWindowWatcher instead.... I think just having a line number toolbar thingy would be best, myself.... but I'm not a good person to ask about UI. In any case, it's be good to get the code itself working and landed; that can happen even without real UI for it (note that it would be trivial to at least make links from the JS console scroll to the right line, per patch in bug 79612). Two more questions: 1) Why the relative positioning instead of just using a negative margin? 2) What page are you using as your testcase for how long the "go to line" takes? Could you possibly test it on http://www.w3.org/TR/DOM-Level-2-HTML/html.html just so we have a baseline for a fairly large heterogeneous page?

Er, biesi is right. nsIPromptService, not nsIWindowWatcher. I just thought of something else. Sicking, could the splitting and such done in this patch be more easily done using surroundContents on a range or something?

it looks like you should be able to just set the start and endpoint of a range and then use the range to do all the splitting and cloning. However you will not be able to call .surroundContents because that doesn't like splitting non-textnodes (i.e splitting a parent <span>). This is a very stupid limitation in .surroundContents and I poked them about it before the spec went into recommendation, but aparently it is by design (don't ask me why though, it doesn't make sense to me) http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Surrounding However what you still can do is to use the range and call selectedLine.appendChild(range.extractContents()); range.insertNode(selectedLine); That should split any spans as necessary.

>> element.scrollIntoView() doesn't do what I expect > >That said, is the problem that scrollIntoView is just scrolling to the top of >the parent span that the span you added is in or something? It scrolls to the top of the parent pre. This is a problem, because a pre containing Javascript can easily contain more than a screenful of lines. >One possibility here is to use <a id="line-xxx" name="line-xxx"> This appears to behave exactly like scrollIntoView(). I found out that scrollIntoView() is actually broken for dynamically created elements (bug 186149), and this is probably also affecting <a id="line-xxx" name="line-xxx">. However, using setTimeout('element.scrollIntoView()', 1) appears to be working for now. >1) Why the relative positioning instead of just using a negative margin? I was tired :-) Now fixed. >2) What page are you using as your testcase for how long the "go to line" >takes? Could you possibly test it on >http://www.w3.org/TR/DOM-Level-2-HTML/html.html It works in < 1 sec even on a long page like this. However, you have to wait for the line to display before searching (not the whole document, just the line your are looking for). Especially with syntax highlighting turned on, this may take quite a while. But once it is loaded, "go to line" works "instantly". >However what you still can do is to use the range and call >selectedLine.appendChild(range.extractContents()); >range.insertNode(selectedLine); Wow, that definately made the code a lot simpler. Also, it now makes sense to use a treewalker to find the text nodes, making the code even more simple. However, due to bug 135922 I cannot use range.insertNode(), so instead I use a slightly more complicated workaround. >Not really, you should use nsIPromptService. Done. Also, the strings are now localizable. >I think just having a line number toolbar thingy would be best, myself.... but >I'm not a good person to ask about UI. In any case, it's be good to get the >code itself working and landed; that can happen even without real UI for it This patch uses the dialog approach. I can easily change it to a toolbar thingy or completely remove the UI (who decides this?), but at least for testing some kind of UI is probably necessary. >(note that it would be trivial to at least make links from the JS console >scroll to the right line, per patch in bug 79612). It's not completely trivial (for me, at least :-), because the "go to line" function has to wait until the line has been displayed (loaded?). That could probably be done using window.setInterval(), but I don't know how to detect when the page has finished displaying (I cannot find an appropriate onload event).

Assigning to Christian, since he's doing all the work... ;) > I found out that scrollIntoView() is actually broken for dynamically created > elements (bug 186149), and this is probably also affecting <a id="line-xxx" > name="line-xxx"> Yep. Try the patch in that bug? If that makes the <a name="xxx"> approach work, I'd sorta like to go with it, since if/when we implement smart scrolling behavior on resize we'd be more likely to keep the right place... That, and it makes the code simpler again -- no need for keeping track of the class. If one approach works and the other does not, of course, we use the one that works. ;) > However, due to bug 135922 I cannot use range.insertNode() Try the patch in that bug too? I've always loved how the use of DOM in our UI forces our DOM impl to suck less.... ;) I hope that all the methods involved keep the selected line properly highlighted, btw.... (I'm not too familiar with ranges, as you can tell.... ;)). In particular, using the "pre" as the thing to append to instead of using the range start container in the case when the container is not a #text node looks odd... (but I've not really put much thought about what the range looks like at that point yet). > but I don't know how to detect when the page has finished displaying Hm... You _may_ be able to attach an onload event to _content. That has to be done after you start the load, however... Perhaps a better way is to, before you start the load, getBrowser().webNavigation.GetInterface(Components.interfaces.nsIWebProgress), then use a small nsIWebProgressListener impl to watch for the STATE_STOP state change? The set an interval and keep trying to scroll and cancel when you manage to scroll or the stop event comes in.... I haven't really done a thorough nitpicking review yet, since this is sort of in-progress so far.. but there are some places where things don't really line up; dunno if those are tabs or just bad whitespacing. This is looking really good, btw. Thanks a ton for working on it.

>> I found out that scrollIntoView() is actually broken for dynamically created >> elements (bug 186149), [...] > >Yep. Try the patch in that bug? >> However, due to bug 135922 I cannot use range.insertNode() > >Try the patch in that bug too? I've always loved how the use of DOM in our UI >forces our DOM impl to suck less.... ;) Boris, you rock! Both your patches work perfectly and made the patch for this bug even cleaner. >Hm... You _may_ be able to attach an onload event to _content. That has to be >done after you start the load, however... Perhaps a better way is to, before >you start the load, >getBrowser().webNavigation.GetInterface(Components.interfaces.nsIWebProgress), >then use a small nsIWebProgressListener impl to watch for the STATE_STOP state >change? The set an interval and keep trying to scroll and cancel when you >manage to scroll or the stop event comes in.... Appearently STATE_STOP is reached way before the whole document is displayed and accessible via the DOM API. If I understand nsViewSourceHTML.cpp correctly it just adds elements to an existing document, so waiting for onload/STATE_STOP wont work. Is it possible for nsViewSourceHTML.cpp to trigger an event, or perhaps close the element with a magic element, e.g. <span id="the-end">? Or is there another way to detect when nsViewSourceHTML.cpp is done? Or did I miss something about nsIWebProgressListener?

OK, maybe we should just do this the stupid way. ;) Sometime before you actually start the load (say at the top of onLoadViewSource() or viewSource(), do: getBrowser().addEventListener("load", foo, true); and add function foo (event) { // we are loaded } (with a better function name and all). Just make sure to use a capturing listener, since load events don't bubble... (and yes, I should have thought of this before suggesting the other stuff; I've been working on necko/docshell too much lately.... ;) )

Now I managed to get hold of the onload event (using getBrowser().addEventListener). Included in the patch is now also a change to the JS console that makes the links to the view source window also go to the line in question. The line number is supplied via window.arguments as recommended in bug 79612. The argument must be of type "number" to work. I changed window._content.document.getElementById('viewsource') to window._content.document.body, because when view source is called without a page descripter (e.g. from the JS console) the body element for some reason does not have id="viewsource". I don't know why this is the case, but using document.body is probably just as good as using document.getElementById("viewsource"). I jump to the line using webNavigation.loadURI(), but I looks as if I have to supply an absolute URL instead of just |#line-1234|. I don't know a way to find this absolute URL except by doing simple string searches and concatenations (I tried getBrowser().webNavigation.currentURI.resolve(...) and ioService.newURI(...) without luck).

Gack. You're right; loadURI does not take an anchor name.... I should consider fixing that or adding a scrollToAnchor on that API.... Anyway, that works for now. I'm not sure why there is no "viewsource" id -- nsViewSourceHTML.cpp always adds it to the <body>... Please give global variables names like gLineFound? If you use the :target pseudo-class instead of a class, you shouldn't need to keep track of the selected line globally, right? This is looking really excellent. ;)

I now use the :target selector, so selectedLine no longer have to be global. The global variables have been prefixed with |g|.

Comment on attachment 120593 [details] [diff] [review] Updated patch.... rbs, would you sr the back end part of this?

Comment on attachment 121923 [details] [diff] [review] Patch v. 1.5 + textNodes: for (var textNode = treewalker.firstChild(); + textNode != null; + textNode = treewalker.nextNode()) { Indent the sub-clauses of the if to line up? No need for a new patch with that change till after Neil's review. sr=me, and looking forward to having this in the tree. ;)

Comment on attachment 121923 [details] [diff] [review] Patch v. 1.5 + var selectedLine = window._content.document.getElementById('line-' + line); always null here due to 'line-'

rbs, that's not looking for the pre blocks. See + selectedLine.id = 'line-' + line;

Might be best to settle on a uniform nomenclature then. It is confusion otherwise. Other comments comming up...

Comment on attachment 121923 [details] [diff] [review] Patch v. 1.5 + if (typeof(arg) == "number" && !isNaN(arg)) { + window.setTimeout(waitForLine, 200, arg); + getBrowser().addEventListener("load", function() { contentLoaded = true; }, true); + } + } [...] +function waitForLine(line) { + var found = goToLine(line); + // + // If the line was not found and the page has not finished + // loading, then try again after a short while. + // + if (!found && !contentLoaded) { + window.setTimeout(waitForLine, 200, line); + } +} Do you need all these genuflections? BTW, I experienced similar troubles with View Selection Source where I had to wait until the DOM is built before highligthing the selected area. (It is until you set out to do these stuff that difficulties show up :-) It would have save you some hassle to look over there. Your code can be simplified in these ways: 1) to wait until the view-source DOM is there, you can for example do the following (i.e., no need for setTimeout): disable_the_goto_line_menu_item_by_default(); window.document.getElementById("appcontent") .addEventListener("load", waitForLine, true); function waitForLine() { removeListener(waitForLine) ... enable_the_goto_line_menu_item(); } 2) no much point for CSS rules with :target. Just re-use the selection infrastructure to select the line (like in View Selection Source). function gotoLine() { // get the selection controller var contentWindow = getBrowser().contentDocument.defaultView; var selection = contentWindow.getSelection(); // figure out the range of interest, say |myRange| // (it is possible to avoid modifying the view-source DOM by // defining the parameters of range appropriately) myRange = ... if (invalid) return, of course selection.removeAllRanges(); selection.addRange(myRange); // this will automatically scroll // the selection/line into view... // (i.e. no need to re-load) // ======= // FYI, the default behavior of the selection is to scroll at the // end of the selection. For wrapped lines, to ensure that the // selection is scrolled at the beginning, you might want to override // the default behavior with this little voodoo: (this is what // View Selection Source does, BTW) try { getBrowser().docShell .QueryInterface(Components.interfaces.nsIInterfaceRequestor) .getInterface(Components.interfaces.nsISelectionDisplay) .QueryInterface(Components.interfaces.nsISelectionController) .scrollSelectionIntoView( Components.interfaces.nsISelectionController.SELECTION_NORMAL, Components.interfaces.nsISelectionController.SELECTION_ANCHOR_REGION, true); } catch(e) { } }

Comment on attachment 120593 [details] [diff] [review] Updated patch.... r+sr=rbs. (With the freeze, I wouldn't have bothered doing the cleanup since it makes the patch unnecessarily big when requesting the a=)

>1) to wait until the view-source DOM is there, you can for >example do the following (i.e., no need for setTimeout): The thing is that the line may be accessible before the whole document has loaded and the load event occurs. setTimeout allows us to highlight the line almost as soon as it has been loaded. Thanks for the review. An updated patch coming up later today.

Comment on attachment 121923 [details] [diff] [review] Patch v. 1.5 >+ getBrowser().addEventListener("load", function() { gContentLoaded = true; }, true); I agree that you should be trying to find the line here. >+ var viewSourceBundle = document.getElementById('viewSourceBundle'); Pity you forgot to include the bundle in the XUL... >+ for (;;) { I'm doubtful you want this in a loop... >+ if (isNaN(line)) { >+ promptService.alert(window, >+ viewSourceBundle.getString("invalidInputTitle"), >+ viewSourceBundle.getString("invalidInputText")); >+ } else { >+ break; >+ } if (!isNan(line)) break; promptService.alert(...); IMHO looks neater. Actually, you might want to check for if (line > 0) break; >+ if (selectedLine == null) { Do we do this or !selectedLine? (Can't remember "house" style) >+ for (var lbound = 0, ubound = viewsource.childNodes.length - 1; ; ) { >+ var middle = lbound + Math.ceil((ubound - lbound) / 2); var middle = Math.ceil((ubound + lbound) / 2); Aside: I prefer to have ubound start at .length and adjust the code accordingly e.g. middle = (ubound + lbound) >> 1 >+ textNodes: A label! Is this to be the first JS label in Mozilla? (OK I haven't searched the rest of the code). >+ // Count newlines; Try splitting the data into lines, that gives you an instant count. i.e. var lineArray = textNode.data.split(/\n/); (you might need to adjust for a trailing newline though) >+ selectedLine = window._content.document.createElement('SPAN'); Interesting this. When word wrap is on the border looks awful. I tried using -moz-outline which outlines each row of the wrapped line. I also tried using a DIV instead of a SPAN, which forces a 100% width block which looks good for short and wrapped lines, but not for long unwrapped lines (i.e. more than 100% width). You could of course try pre[wrap] :target { display: block; } (or whatever). Or you could forget the border and put both a DIV and a SPAN in... >+ if (selectedLine != null) { >+ selectedLine.className = 'selected-line'; A leftover from some code that you might need to resurrect? >+ getBrowser().webNavigation.loadURI(viewSrcUrl, loadFlags, null, null, null); Note that getBrowser() also has a .loadURI method which passes some default values to .webNavigation.loadURI >+<!ENTITY goToLineCmd.label "Go to line ..."> No space before the ...

neil, thanks for the review. I have a few clarifying questions: >>+ getBrowser().addEventListener("load", function() { gContentLoaded = true; }, true); >I agree that you should be trying to find the line here. I am not sure I understand you correctly. Do you suggest that I should wait for the whole DOM to load before looking for the line? This will make looking for a line in the beginning of the document take significantly longer - especially when syntax highlighting is on. >>+ for (;;) { >I'm doubtful you want this in a loop... If I get you right, you think that I should just show the alert(invalidInput) and then terminate without prompting for a line again? Notepad, UltraEdit and others keep prompting for a line until a valid integer has been entered. >Interesting this. When word wrap is on the border looks awful. I will probably just use a selection as suggested by rbs to be in sync with View Selection Source (if people agreee).

Yeah, using the selection sounds fine.

re: comment #64 > >1) to wait until the view-source DOM is there, you can for > >example do the following (i.e., no need for setTimeout): > > The thing is that the line may be accessible before the whole document has > loaded and the load event occurs. setTimeout allows us to highlight the line > almost as soon as it has been loaded. With the selection approach, you wouldn't be using a view-source(url#line) anymore. Hence there is no (re)load. Rather, the gotoLine kicks in after the user mouses clumsily to Edit -> Goto Line -> input. By then, a rather long time would have elapsed and the single load (first time) would have completed in most cases. [In an earlier implementation of View Selection Source, it used a setTimeout. It was not worth the trouble. Plus the sr didn't like it. Also, optionally disabling the menu item (if you deem necessary) provides a safeguard until that first load is done. As you experiment with the selection, you will get a clearer picture of where things stand.]

> Rather, the gotoLine kicks in after the user mouses clumsily to Edit -> Goto > Line -> input. Except in the common case of source being opened from the JS console. Then the gotoLine() is called programmatically. We could indeed delay that till the onload, but if there is a JS error on line 143 of a 5000-line HTML file, it'd be good to scroll to it asap after the user clicks the link in the JS console.

Adding dependency to bug 79612 which I hadn't read before and which is the scenario that you describe. So, the fix is intended to subsume two cases: Normal: view-source(url) + Edit -> Goto line -> input JS Console: view-source(url#line), jump to line. ==== From my recollection, I couldn't get a meaningful document pointer from the script until the document onload was fired (paint supression/zombie, perhaps). There is a difference between: /* this one tells that the chrome is ready */ + getBrowser().addEventListener("load", function() { gContentLoaded = true; }, true); /* this one tells that the content document is ready, same as <body onload> */ window.document.getElementById("appcontent") .addEventListener("load", waitForLine, true); Are you really observing a jump to the line asap with the other one, while the document is still sluggishly loading?

> There is a difference between: There is? The first sets a capturing load listener on the XUL <browser> node. The second sets a load listener on the parent of said node. Both will fire at the same point -- when the onload() of the document loading withing the browser fires. I'm assuming that Christian tested the scrolling to line while the document is loading, of course.

> There is? Indeed. It took me a while to discover & get what I wanted because of that difference. In fact, come to think of it, if there wasn't a difference the setTimeout would be meaningless. Since <body onload> would mean that the DOM is there, and so gotoLine will succeed. Whereas <chrome onload> means that the chrome is done and is about to load the document, and so attaching the waitLine for the document means something. This brings to what I was most wondering about: whether what is exposed to the script is indeed a meaningful at that stage, which is why I am curious to know how the testing goes.

> This brings to what I was most wondering about: whether what is exposed to the > script is indeed a meaningful at that stage, which is why I am curious to know > how the testing goes. I do not know whether the DOM exposed to the script is meaningful during load, but if the text nodes are added sequentially in the order they will appear in the tree, then I assume that if the script thinks it has found the line then it in fact is the right line (because text nodes cannot be inserted before it in the tree). But I am just guessing. Judging by my testing it looks as if the setTimeout version always (?) succeeds in finding the line, but sometimes the scrollSelectionIntoView fails, i.e. the line is highlighted but the window does not scroll at all. Perhaps the text is somehow present in the DOM a short while before it is displayed on the screen. If people prefer the conservative approach that waits for the whole document to load before looking for the line, I will use that.

> the setTimeout version always (?) succeeds in finding the line Indeed, since you are looping. But it could be that the loop is succeeding at the very end, i.e., when the document has already loaded anyway. Another thing to double-check is that the line that is seemingly found may actually belong to the old document (the so-called "zombie" document). I am a bit cautious in this muddy area because it took me a while to get out of it. Once you are happy with a reliable version, feel free to show us.

This patch waits for the load event before looking for the line. This appears to work in all cases contrary to the setTimeout solution that sometimes failed to scroll the line into view. The specified line is highlighted using a selection. If the line happens to be blank, the selection is invisible. This might change if the linefeed characters are made visible in a selection as proposed in bug 156175. I don't know if we need a workaround for this, e.g. reverting to highlighting via CSS, but since empty lines in general aren't of much interest this probably isn't a big issue. It looks like there is a problem with scrollSelectionIntoView being off by one line, so I filed bug 205006.

Comment on attachment 122940 [details] [diff] [review] Patch v. 1.6 Looking good to me, just a few sanity check comments: + // arg[3] - Line number to go to. // if ("arguments" in window) { var arg; @@ -78,6 +104,16 @@ } } // + // Wait for the specified line to load. + // + if (window.arguments.length >= 4) { + arg = window.arguments[3]; + + if (typeof(arg) == "number" && arg > 0) { + gGoToLine = arg; + } + } + // This is for bug 79612, view-source(url#line), right? I don't see the parsing of #line. Needs to hook properly now? + getBrowser().addEventListener( + "load", + function() { + if (gGoToLine) { + goToLine(gGoToLine); + gGoToLine = false; + } You mean |if (gGoToLine > 0)| and |gGoToLine = -1| since it isn't a boolean.

> This is for bug 79612, view-source(url#line), right? I don't see the parsing > of #line. It is indeed a hook for the JS console, but the line number is sent from the JS console (consoleBindings.xml) via window.arguments instead of url#line (as suggested in bug 79612), i.e. no parsing needs to be done: window.openDialog( "chrome://navigator/content/viewSource.xul", "_blank", "scrollbars,resizable,chrome,dialog=no", url, null, null, line);

I applied the patch for experimentation and could fix/work-around the remaining issues: 1) If the selected line happens to be blank, the selection is invisible. -> show a blinking caret on the line... 2) scrollSelectionIntoView off by one line -> use a "hint" to orientate the selection. This one was subtle because range indices are delicate. With your code, the selection start's offset is after the "\n" at the end of the previous line. By using an interline hint ("hintright"), I forced the needed line to come into view. Also, -> moved the bundle to the viewsource overlay -> placed the Goto line menu item before the Finds. It looked akward to be in-between them. (I guess some UI people might comment later on the wordings anyway.) Back to you. Good job. It works great and the ice on the cake is that it kills bug 79612, as well as CSS errors (when the line number is given properly). Re: bz, you might want to sync the line's id in DUMP_TO_FILE. Also, it looked like you could have set id="line1" on the first <pre> to save the special-case in the JS.

Yeah, I should sync the DUMP_TO_FILE stuff, and it looks like I need to add some IF_FREE() calls as well (the body token is the only one we do it for now)... those are not real leaks, since it's an arena, but still.... Oh, and yes; I'll add an id to the first <pre>.

Want to drive things in, then? It is low risk with high reward. Click on a warning/error in the JavaScript console and it jumps to the precise line in the source and highlights it... JavaScript'ers in particular will like this a lot.

Comment on attachment 123360 [details] [diff] [review] Patch for back end updated to comment 78 I was wrong about IF_FREE. Parser nodes free their tokens, so we should NOT, in fact, be freeing them. This is the same as the previous patch except I added id="line1" to the first <pre> and fixed the DUMP_TO_FILE sections. I agree that it would be great to take this for 1.4, drivers willing (I agree that this is quite low-risk). What we need is r= on the front end section from a front-end person (eg Neil).

Comment on attachment 123251 [details] [diff] [review] v1.7 - correct remaining issues from using the selection sr=me on this (modulo whatever you guys decide to do with that first pre). Neil, would you review?

Comment on attachment 123360 [details] [diff] [review] Patch for back end updated to comment 78 r+sr=rbs on this updated back-end.

Comment on attachment 123251 [details] [diff] [review] v1.7 - correct remaining issues from using the selection >+ locale/en-US/navigator/viewSource.properties (resources/locale/en-US/viewSource.properties) Sadly the file itself is missing from the patch :-/ >+ // >+ // Disable "go to line" while reloading due to e.g. change of charset >+ // or toggling of syntax highlighting. >+ // >+ getBrowser().addEventListener( >+ "unload", >+ function() { >+ document.getElementById('cmd_goToLine').setAttribute('disabled', 'true'); >+ }, >+ true); >+ >+ getBrowser().addEventListener( >+ "load", >+ function() { >+ if (gGoToLine) { >+ goToLine(gGoToLine); >+ gGoToLine = 0; >+ } >+ document.getElementById('cmd_goToLine').removeAttribute('disabled'); >+ }, >+ true); I'm not a fan of big inline functions... >+ arg = window.arguments[3]; >+ >+ if (typeof(arg) == "number" && arg > 0) { >+ gGoToLine = arg; >+ } Probably neater to do the parseInt here instead of in consoleBindings.xml i.e. arg = parseInt(window.arguments[3]); if (arg > 0) { gGoToLine = arg; } Actaully, gGoToLine = parseInt(window.arguments[3]); would work becuause your load handler already checks for > 0. >+ promptService.alert(window, >+ viewSourceBundle.getString("invalidInputTitle"), >+ viewSourceBundle.getString("invalidInputText")); The subtle difference is that Notepad et al don't close the prompt for the input while displaying the invalid input alert... in fact, Notepad doesn't close it for the out of range alert either, so I think you should be consistent too, whichever way around, either always prompt after an error or never prompt after an error. >+ gLastLineFound = line; >+ var found = goToLine(line); You haven't found the line yet... please set gLastLineFound in goToLine() instead. >+ if (curLine == line) {// && !range) { Still setting the end of the range every time? >+ if (!range) { >+ range = document.createRange(); >+ range.setStart(textNode, curPos); >+ } >+ >+ // >+ // This will always be overridden later, except when we look for >+ // the very last line in the file (this is the only line that does >+ // not end with \n). >+ // >+ range.setEndAfter(pre.lastChild); >+ var selection = window._content.getSelection(); >+ selection.removeAllRanges(); I think you should use if (!range) return false; before here (also change the last return to return true;). >+ var url = this.getAttribute("url"); >+ var line = this.hasAttribute("line") ? parseInt(this.getAttribute("line")) : null; > window.openDialog( > "chrome://navigator/content/viewSource.xul", "_blank", >- "scrollbars,resizable,chrome,dialog=no", this.getAttribute("url")); >+ "scrollbars,resizable,chrome,dialog=no", url, null, null, line); With the parseInt in viewsource.js you can just use two inline getAttribute calls instead now.

Comment on attachment 123360 [details] [diff] [review] Patch for back end updated to comment 78 Requesting 1.4 approval for the back end part. This is a very safe change that just adds line number information to the markup view-source generates; this should enable front-end code to be added (in the other patch, or via an XPI) that will enable users to "go to line" in view-source.

> The subtle difference is that Notepad et al don't close the prompt for the > input while displaying the invalid input alert... [...] so I think you should > be consistent too, whichever way around, either always prompt after an error > or never prompt after an error. I decided to always prompt after an error. This allows the user to see and possible change what he typed.

+ // the first line in the pre element is number 123 (except the first + // pre that does not have an id attribute). Now that the first pre isn't a special-case, the "except" bit is not necessary anymore, you might want update the comment at check-in.

Comment on attachment 123452 [details] [diff] [review] v1.8 - updated to comment 85 >+ if (gGoToLine) { I think gGoToLine > 0 would be better here. >+ goToLine(gGoToLine); >+ gGoToLine = 0; >+ } >+ var selection = window._content.getSelection(); >+ selection.removeAllRanges(); >+ >+ if (!range) { >+ return false; >+ } I think you should swap these around, otherwise the previous selection will be lost if the line was not found.

I am not sure whether this update needs new reviews. I do not have CVS access so I need help getting this checked in when it's time - thanks.

Comment on attachment 123502 [details] [diff] [review] v1.9 that addresses comment 88 and 89 Neil, one last look, please? ;) sr=me

Comment on attachment 123360 [details] [diff] [review] Patch for back end updated to comment 78 a=asa (on behalf of drivers) for checkin to 1.4

Comment on attachment 123360 [details] [diff] [review] Patch for back end updated to comment 78 Back end patch checked in.

Comment on attachment 123502 [details] [diff] [review] v1.9 that addresses comment 88 and 89 a=asa (on behalf of drivers) for checkin to 1.4

Patch checked in. Christian, thank you once again (I just noticed that this plays perfectly with the debug-only CSS error reporting, so life got that much happier).

*** Bug 239211 has been marked as a duplicate of this bug. ***