mp4ra / mp4ra.github.io

MPEG-4 Registration Authority Web Site
https://mp4ra.github.io/
38 stars 27 forks source link

Corrections #134

Closed podborski closed 2 years ago

podborski commented 2 years ago

while working on the conformance suite for FileFormat and building up the set of standard_features. I checked the alignment of registered codes between MP4RA, GPAC and the syntax from the actual specs (extracted with a python script). Here is a short summary of findings:

This PR also removes redundant column in specifications.csv

podborski commented 2 years ago

@cconcolato or @edrthomas, could you review https://github.com/mp4ra/mp4ra.github.io/pull/134/commits/025408474b83c525fc52c5c02c21e068c0268d4e? Just to make sure I din't break anything :)

The reason for that is that entire column in the specifications.csv is not used, and there have been confusions in the previous PRs on how to name elements in that row, etc. I guess it's just better to get rid of it completely.

edrthomas commented 2 years ago

@podborski indeed seeing the specifications.csv the first and second columns became very much alike over time. We may try to combine them.

I looked at your PR and I think this will break the scripted scrolling. Whenever you click on a specification in any table this calls the specifications page and then a piece of JavaScript scrolls to the line of that specification. To know where to scroll to, the element plays the role of a invisible tag. The scrollFix function indeed looks for any html element with the attribute "name" with the value of the linkname, this matches on the corresponding element. So we could remove the linkname and use the specification name field but removing the will break the scrollFix function, I am afraid.

This is just my reading of the code. Did you try to compile it locally to confirm it?

As for solution, we may just for now keep the like this:

I am adding a if because item.specificationAnchor may not always been defined as opposed to the current item.linkname.

podborski commented 2 years ago

I looked at your PR and I think this will break the scripted scrolling. Whenever you click on a specification in any table this calls the specifications page and then a piece of JavaScript scrolls to the line of that specification. ... ... This is just my reading of the code. Did you try to compile it locally to confirm it? ...

@edrthomas Yes, I tried to run it locally and it all seems to work as before (on Safari). Therefore I'm a bit surprised.

edrthomas commented 2 years ago

@podborski , I pulled this branch and tested locally. From what I see, this PR breaks the scrolling.

Steps to reproduce:

  1. Go to search
  2. Type iso
  3. Click on any ISO link in the specification column
  4. The page specification is displayed but stays at the top
  5. The console log shows: Uncaught TypeError: Cannot read properties of undefined (reading 'getBoundingClientRect')

The error is raised because the invisible tag is not found and thus the scrolling cannot be performed. If you try it locally, don't forget to recompile the js code by running the build.sh script. Otherwise the changes in the /src won't be used in your browser.

If I try those steps on the public website, the scrolling does happen.

BUT, what I did notice also is that the scrolling for all the other pages seems to be already broken while it shouldn't be.

For instance, search vvcN and click on the Codecs link in the column. It should scroll down to vvcN but it currently does not. I also found other bugs here and there, e.g. type vopi and click on Sample groups link. The link is broken because it does not use the name of the page for the routing (with a _) but the display name with a whitespace.

Since this PR contains more useful things, I would suggest to remove the commits related to the removal of linkname. I have the feeling I created it for a good reason in the end :) The rest could be pushed.

Also, I will be working soon on a PR about the other bugs I noticed,

podborski commented 2 years ago

@edrthomas nice catch with the scrolling, this should be now fixed. I should not have removed v-bind in table.vue, now it's binding to specification instead of not present linkname (which was removed from the specification.csv). I searched through all the code and I don't see any other locations where linkname is used.