cnumr / ecoindex_scrap_python

Ecoindex_scraper module provides a way to scrape data from given website while simulating a real web browser
Other
2 stars 4 forks source link

[Bug]: Le décompte des SVGs semble incohérent #99

Closed tsecher closed 1 year ago

tsecher commented 1 year ago

Dans la fonction de décompte des tags liés au svgs, il semble y avoir une incohérence comparée à la spec. En effet, pour déterminer le nombre d'éléments dans le DOM, on comtpe l'ensemble des tags ("//"). On comprend donc l'ensemble des descendants des balises SVGs. En revanche dans le compte des éléments SVG on ne compte que les enfants directs ("//[local-name()='svg']/"). Ne faudrait-il pas mieux soustraire l'ensemble des enfants du SVGs, comme sous-entendu dans la spec ("//[local-name()='svg']//*") (et comme le fait l'extension GreentIT-Analysis)?

https://github.com/cnumr/ecoindex_scrap_python/blob/1231e086dac1d46ee3fad2f1238038c345399823/ecoindex_scraper/scrap.py#L213

vvatelot commented 1 year ago

Salut @tsecher dans le calcul du DOM, on décompte justement le nombre d'éléments enfants du svg:

https://github.com/cnumr/ecoindex_scrap_python/blob/1231e086dac1d46ee3fad2f1238038c345399823/ecoindex_scraper/scrap.py#L144

Est ce que c'est ça la question ?

tsecher commented 1 year ago

Oui, justement ma question c'est qu'on ne décompte que les enfants directs. Pas l'ensemble des enfants du svg. Par exemple si un SVG a cette structure : <svg><g><path id="path-1" .../><path id="path-2" .../></g></svg>. On ne va décompter que le tag <g>, et pas les deux <path>.

Donc par exemple sur un DOM de 10 tags, qui contient ce SVG en exemple, on fait 10 - 1 (DOM - SVG_DIRECT_CHILDREN), au lieu de 10 - 3 (DOM - SVG_NESTED_CHILDREN)

En comparaison, greenIT-Analysis supprime tous les descendant des SVGs image

tsecher commented 1 year ago

A mon avis, tout se joue sur l'expression du xpath ici : https://github.com/cnumr/ecoindex_scrap_python/blob/1231e086dac1d46ee3fad2f1238038c345399823/ecoindex_scraper/scrap.py#L215 Je pense qu'il faut un double slash pour intégrer tous les descendants des svgs : ("//[local-name()='svg']//*")

La spec n'est pas très claire et parle uniquement d'enfant. Mais il me semble plus logique de ne pas compter tous les enfants d'un svg. Sur certains sites, la stratégie va être d'utiliser en priorité des svgs pour éviter justement de faire des requête et d'intégrer des images lourdes. La différence est très vite visible sur le résultat de l'ecoindex vue que le nombre d'élément du DOM peut rapidement augmenter à cause de ces descendants de svgs.

vvatelot commented 1 year ago

OK ! Tu as raison, je ne m'étais pas rendu compte de ce problème... Je te laisse faire une PR ou tu préfères que je m'en charge ?

tsecher commented 1 year ago

Je peux la faire si tu veux.