nytprofhtml - internal anchor links in html pages go to wrong locations #192
Comments
|
Rearranging OP to better respond ....
It should be noted that the switch of Jquery files took place between 6.09 and 6.10.
Beyond my skill set as well.
Here is a possible explanation. In 2016, the following commit was added to the master branch in the GH repository: It was at this commit that lib/Devel/NYTProf/js/jquery.floatThead.min.js was added to the repository. (Note that this was way before I myself had anything to do with Devel-NYTProf.) However, it appears that this file was not added to the MANIFEST at that time, so it would not have been distributed via a tarball uploaded to CPAN. The file was finally added to MANIFEST as follows:
The test suite, being text-based, is inherently ill equipped to spot deficiencies in the generated HTML. Is there some very simple program over which you could run the 'nytprof' utility that would enable us to spot the differences in generated HTML?
I'm a bit confused ... is there a problem with Firefox or not? (Note that if there are problems with one browser but not on another, then the problem may be out of our control.) Thank you very much. |
|
Thanks for the detailed investigations. It looks like an issue with As an experiment, I ran The rest of this post is to answer your queries, slightly reordered.
The issue impacts all three browsers under Windows 10, but Firefox has the workaround I described. As an additional point, I have reproduced the issue on Ubuntu 18 with firefox as the browser.
An example script is below. Call the usual sequence of I used the tree and diff utilities in bash to see the differences between directory contents generated by different versions of Devel::NYTProf. use 5.010;
use strict;
use warnings;
my @data = (0..100);
foreach my $i (@data) {
run_sub ($i);
}
sub run_sub {
my $x = shift;
my $y = run_sub2($x+1);
return $y;
}
# extra subs to pad out the html
# in the nyt report
# run_sub2 is in the middle
sub extra_sub1 {
my $x = shift;
my $y = run_sub2($x+1);
my $padding = << 'EOPADDING'
random random random random
random random random random
random random random random
random random random random
random random random random
random mandor random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
random random random random
EOPADDING
}
sub run_sub2 {
my $y = shift;
#say $y;
my $count = grep {$_ < $y} (0..100);
#say $count if $count;
return $count;
}
sub extra_sub6 {
my $x = shift;
my $y = run_sub2($x+1);
return $y;
}
sub extra_sub7 {
my $x = shift;
my $y = run_sub2($x+1);
return $y;
}
sub extra_sub8 {
my $x = shift;
my $y = run_sub2($x+1);
return $y;
}
sub extra_sub9 {
my $x = shift;
my $y = run_sub2($x+1);
return $y;
}
|
This is the second attempt to respond to the problem described in timbunce#192. In this attempt we take into consideration the concerns raised by Tim Bunce in timbunce#193 (comment).
|
This should be resolved with Devel-NYTProf version 6.11, just released to CPAN. |
|
I went looking to close this ticket today and installed Devel::NYTProf v6.11 from CPAN, then ran the program which the OP provided. I was not very satisfied with the results. In both cases, the location to which I expected to be taken when clicking on a link was "below the screen" (or just at the bottom of the visible window when in full screen mode). However, I know nothing about Javascript, jquery, etc., so I am at a loss as to how to improve the situation further. I'm going to close this ticket in 7 days unless someone suggests a better approach. |
|
The file causing the problems is Sorry for not spotting that when it was first implemented. |
|
On 9/24/21 9:13 PM, shawnlaffan wrote:
The file causing the problems is |jquery.tablesorter.min.js|, but it was
|jquery.floatThead.min.js| that was removed.
Sorry for not spotting that when it was first implemented.
Okay, within a few days, I'll restore |jquery.floatThead.min.js| and
remove |jquery.tablesorter.min.js|. I'll do that in a branch of my
github repository and ask you to test it out before I push to the main
repository (timbunce) and issue a new CPAN release.
Thank you very much.
Jim Keenan
|
|
That plan works for me. |
|
I'm starting to think that the "bad" output I got with Shawn Laffan's test file once I installed Devel-NYTProf-6.11 from CPAN is the result of installing this version against the same Why do I suspect this? Because when I install Devel-NYTProf-6.11 against a fresh I figured that in order to have a "baseline" against which to measure the effect of changes, I would install a fresh The steps I took to generate these results were: Devel-NYTProf-6.06o Installed Perl 5 blead (2c205b5406). Devel-NYTProf-6.11o Installed Perl 5 blead (2c205b5406). I then eyeballed the generated HTML files. I couldn't detect any formatting differences between the 6.06 and 6.11 versions. Upon reflection, I recalled that this was the result I was getting when I began working on this issue after @shawnlaffan reported it earlier this month. It was only after I installed 6.11 from CPAN against my "everyday" To add to the confusion ... When I look at the Javascript files that are found in each installation, I observe the following: I get "good" results when I install either 6.06 or 6.11 against a freshly built But I get "bad" results when I install 6.11 against my previously built This is confusing, to say the least. @shawnlaffan, @timbunce: Could you try reproducing the process I described here and report: Thank you very much. |
|
I'll have a look and report back, but one point I've noticed is that the nytprof directory needs to be deleted between runs so the js subdir does not contain files from different nytprof releases. If you are running the test script and then calling nytprofhtml with default arguments then it adds to the directory contents (something I've just noticed). nytprofhtml needs to be called with either the --out arg so the output goes into a new directory, or with --delete. |
|
WRT the checks, I installed two clean copies of strawberry perl 5.31.1 portable on my machine. I then installed Devel::NYTProf 6.11 in one of them, and 6.06 in the other. Both produced expected results. I then installed 6.11 over the top of 6.06 and got the expected results. Then I installed 6.10 over the top of 6.11. Calling Then I reinstalled 6.11 over 6.10. This still results in offset anchor issue, even when deleting the report output directory. So it would seem that a file or files from 6.10 are having an effect on 6.11. Looking at the js directories, these are the Is there a simple way of cleaning up the js dir on module installation? |
|
Before proceeding further I want to make a correction to the data I provided yesterday in #192 (comment). The listing of With this set of files in position, I re-ran @shawnlaffan's test program and then took screenshots. The screenshots can be found here. As you can see, the |
The implication I'm drawing from the above is that I'm having problems even when |
|
Author of floatthead here, I have no idea what your thing does, but I can help you figure out why my thing isn't working.. at least that's what I understand after reading this thread If you want my help, I would need to generated html/css/js that this tool produces when you run it so I can figure out why floatthead isn't doing what is supposed to do - that is not breaking your stuff. I have no perl and would rather avoid it. I had a bad experience with it 15 years ago ;) |
Removed all code that initializes the plugin (doing this under the assumption that timbunce#194 removes the js file from this project). Also removed `show_fragment_target()` because it would no longer work without floatthead present. This fixes timbunce#192 , or at least fixes the fix that was made in it. The proper way to fix timbunce#192 would be to take a look at the `show_fragment_target()` function and figure out why its setting the incorrect scrollTop there when you click on links. I cannot do this without seeing the generated code. Either way, someone needs to test this change before merging it, because I have not done so.
|
I made a PR to cleanup the issue created in #194 where you removed the script but not its usages. Merging my PR will fix the immediate problem of having broken javascript code on master. After you merge that PR, I suggest that you create a branch where it is rolled back (along with #194), run your tool and send me all the stuff it generates so i can take a look. I know that the problem you are trying to solve here is actually caused by It probably broke because someone changed the height of something it uses to decide how to show the thing you want and not have it be under the floating headers. |
Internal links to anchors in pages generated using recent versions of nytprofhtml go to an offset position in the page. The offset varies, and can be above or below. It is, however, consistent for each link.
First occurs with 6.07. Does not occur with 6.06.
To reproduce, run
nytprofhtmlon the results of a profiling run. Open index.html and click on any link to a subroutine, in the flamegraph or otherwise. Then click on any of the other links in the next page, and so on.Refreshing the page stays at the link, but selecting the URL address and hitting enter goes to the correct location on Firefox (but not on Chrome or MS Edge).
The only meaningful difference I can see in the files produced between these versions is that
jquery-tablesorter-min.jshas been replaced byjquery.tablesorter.min.js. However, working with js files is outside my skill set so I cannot delve further.My operating system is Windows. Occurs across browsers (tested with Firefox, Chrome and MS Edge).
The text was updated successfully, but these errors were encountered: