From e71caa719c3a61cda1e30aa4b2deb232992ca192 Mon Sep 17 00:00:00 2001
From: benoit74 <benoit74@users.noreply.github.com>
Date: Tue, 5 Nov 2024 08:07:53 +0000
Subject: [PATCH 1/4] Remove dead code

---
 scraper/src/mindtouch2zim/processor.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/scraper/src/mindtouch2zim/processor.py b/scraper/src/mindtouch2zim/processor.py
index fc02bf6..65ec21a 100644
--- a/scraper/src/mindtouch2zim/processor.py
+++ b/scraper/src/mindtouch2zim/processor.py
@@ -768,8 +768,6 @@ def rewrite_iframe_tags(
             '<iframe style="display: none;">'  # fake opening tag just to remove iframe
         )
 
-    raise UnsupportedTagError(f"Unsupported src {src} in iframe")
-
 
 class HtmlUrlsRewriter(ArticleUrlRewriter):
     """A rewriter for HTML processing

From 95bafa997c812ffb38649e2e886f51f12e038094 Mon Sep 17 00:00:00 2001
From: benoit74 <benoit74@users.noreply.github.com>
Date: Tue, 5 Nov 2024 08:08:13 +0000
Subject: [PATCH 2/4] Add support for images with only srcset

---
 scraper/src/mindtouch2zim/html_rewriting.py | 67 ++++++++++++++
 scraper/src/mindtouch2zim/processor.py      | 13 ---
 scraper/src/mindtouch2zim/utils.py          | 24 +++++
 scraper/tests/test_html_rewriting.py        | 99 +++++++++++++++++++++
 scraper/tests/test_utils.py                 | 36 +++++++-
 5 files changed, 225 insertions(+), 14 deletions(-)
 create mode 100644 scraper/src/mindtouch2zim/html_rewriting.py
 create mode 100644 scraper/tests/test_html_rewriting.py

diff --git a/scraper/src/mindtouch2zim/html_rewriting.py b/scraper/src/mindtouch2zim/html_rewriting.py
new file mode 100644
index 0000000..120fabd
--- /dev/null
+++ b/scraper/src/mindtouch2zim/html_rewriting.py
@@ -0,0 +1,67 @@
+from zimscraperlib.rewriting.html import (
+    AttrsList,
+    format_attr,
+    get_attr_value_from,
+)
+from zimscraperlib.rewriting.html import rules as html_rules
+from zimscraperlib.rewriting.url_rewriting import ArticleUrlRewriter
+
+from mindtouch2zim.processor import HtmlUrlsRewriter
+from mindtouch2zim.utils import is_better_srcset_descriptor
+
+
+@html_rules.rewrite_tag()
+def rewrite_img_tags(
+    tag: str,
+    attrs: AttrsList,
+    base_href: str | None,
+    url_rewriter: ArticleUrlRewriter,
+    *,
+    auto_close: bool,
+):
+
+    if tag != "img":
+        return
+    if not isinstance(url_rewriter, HtmlUrlsRewriter):
+        raise Exception("Expecting HtmlUrlsRewriter")
+    if not (srcset_value := get_attr_value_from(attrs, "srcset")):
+        # simple case, just need to rewrite the src
+        src_value = get_attr_value_from(attrs, "src")
+        if src_value is None:
+            return  # no need to rewrite this img without src
+    else:
+        scrset_values = [value.strip() for value in srcset_value.split(",")]
+        best_src_value = None
+        best_descriptor = None
+        for src_value in scrset_values:
+            # Ignore RUF005 which prefer to avoid concatenation, because I didn't found
+            # another way to wwite this which still please pyright type checker, which
+            # is not capable to properly infer types of results with other syntaxes
+            url, descriptor = (src_value.rsplit(" ", 1) + [None])[:2]  # noqa: RUF005
+            if best_src_value is None:
+                best_src_value = url
+                best_descriptor = descriptor
+                continue
+            if is_better_srcset_descriptor(
+                new_descriptor=descriptor, current_best_descriptor=best_descriptor
+            ):
+                best_src_value = url
+                best_descriptor = descriptor
+
+        src_value = best_src_value
+
+    rewrite_result = url_rewriter(src_value, base_href=base_href, rewrite_all_url=True)
+    # add 'content/' to the URL since all assets will be stored in the sub.-path
+    new_attr_value = f"content/{rewrite_result.rewriten_url}"
+    url_rewriter.add_item_to_download(rewrite_result)
+
+    values = " ".join(
+        format_attr(*attr)
+        for attr in [
+            (attr_name, attr_value)
+            for (attr_name, attr_value) in attrs
+            if attr_name not in ["src", "srcset", "sizes"]
+        ]
+        + [("src", new_attr_value)]
+    )
+    return f"<img {values}{'/>' if auto_close else '>'}"
diff --git a/scraper/src/mindtouch2zim/processor.py b/scraper/src/mindtouch2zim/processor.py
index 65ec21a..33d0f7e 100644
--- a/scraper/src/mindtouch2zim/processor.py
+++ b/scraper/src/mindtouch2zim/processor.py
@@ -677,13 +677,6 @@ def rewrite_href_src_attributes(
             if rewrite_result.rewriten_url.startswith(url_rewriter.library_path.value)
             else rewrite_result.rewriten_url
         )
-    if tag == "img":
-        rewrite_result = url_rewriter(
-            attr_value, base_href=base_href, rewrite_all_url=True
-        )
-        # add 'content/' to the URL since all assets will be stored in the sub.-path
-        new_attr_value = f"content/{rewrite_result.rewriten_url}"
-        url_rewriter.add_item_to_download(rewrite_result)
     if not new_attr_value:
         # we do not (yet) support other tags / attributes so we fail the scraper
         raise ValueError(
@@ -692,12 +685,6 @@ def rewrite_href_src_attributes(
     return (attr_name, new_attr_value)
 
 
-@html_rules.drop_attribute()
-def drop_sizes_and_srcset_attribute(tag: str, attr_name: str):
-    """Drop srcset and sizes attributes in <img> tags"""
-    return tag == "img" and attr_name in ("srcset", "sizes")
-
-
 @html_rules.rewrite_tag()
 def refuse_unsupported_tags(tag: str):
     """Stop scraper if unsupported tag is encountered"""
diff --git a/scraper/src/mindtouch2zim/utils.py b/scraper/src/mindtouch2zim/utils.py
index ca05336..594e6a1 100644
--- a/scraper/src/mindtouch2zim/utils.py
+++ b/scraper/src/mindtouch2zim/utils.py
@@ -28,3 +28,27 @@ def get_asset_path_from_url(online_url: str, already_used_paths: list[Path]) ->
             break
         index += 1
     return relative_path
+
+
+def is_better_srcset_descriptor(
+    new_descriptor: str | None, current_best_descriptor: str | None
+) -> bool:
+    """Compares two HTML images srcset descriptors
+
+    In srcset="PtolemyWorldMap-1024x701.jpg 1024w", the descriptor is 1024w.
+
+    Descriptors can be None, meaning that they are not set in the srcset.
+
+    This implementation is a bit naive because it supposes the srcset is valid (i.e.
+    no mix of width descriptor and pixel density descriptor), otherwise it assumes that
+    new descriptor is not better than current one if both are set.
+    """
+    if new_descriptor is None:
+        return False
+    if current_best_descriptor is None:
+        return True
+    current_best_descriptor = current_best_descriptor.strip()
+    new_descriptor = new_descriptor.strip()
+    if current_best_descriptor[-1:] != new_descriptor[-1:]:
+        return False
+    return int(new_descriptor[:-1]) > int(current_best_descriptor[:-1])
diff --git a/scraper/tests/test_html_rewriting.py b/scraper/tests/test_html_rewriting.py
new file mode 100644
index 0000000..201aadb
--- /dev/null
+++ b/scraper/tests/test_html_rewriting.py
@@ -0,0 +1,99 @@
+import pytest
+from zimscraperlib.rewriting.html import HtmlRewriter
+from zimscraperlib.rewriting.url_rewriting import (
+    HttpUrl,
+    ZimPath,
+)
+
+from mindtouch2zim.client import LibraryPage
+from mindtouch2zim.html_rewriting import HtmlUrlsRewriter
+
+
+@pytest.fixture()
+def url_rewriter() -> HtmlUrlsRewriter:
+    return HtmlUrlsRewriter(
+        library_url="https://www.acme.com",
+        page=LibraryPage(id="123", title="a page", path="A_Page"),
+        existing_zim_paths=set(),
+    )
+
+
+@pytest.fixture()
+def html_rewriter(url_rewriter: HtmlUrlsRewriter) -> HtmlRewriter:
+    return HtmlRewriter(
+        url_rewriter, pre_head_insert=None, post_head_insert=None, notify_js_module=None
+    )
+
+
+@pytest.mark.parametrize(
+    "source_html, expected_html, expected_items_to_download",
+    [
+        pytest.param(
+            '<img src="https://www.foo.bar/image1.png"></img>',
+            '<img src="content/www.foo.bar/image1.png"></img>',
+            {
+                ZimPath("www.foo.bar/image1.png"): {
+                    HttpUrl("https://www.foo.bar/image1.png")
+                }
+            },
+            id="simple",
+        ),
+        pytest.param(
+            '<img src="https://www.foo.bar/image1.png" '
+            'srcset="https://www.foo.bar/image2.png"></img>',
+            '<img src="content/www.foo.bar/image2.png"></img>',
+            {
+                ZimPath("www.foo.bar/image2.png"): {
+                    HttpUrl("https://www.foo.bar/image2.png")
+                }
+            },
+            id="simple_srcset",
+        ),
+        pytest.param(
+            '<img srcset="https://www.foo.bar/image2.png 1024w"></img>',
+            '<img src="content/www.foo.bar/image2.png"></img>',
+            {
+                ZimPath("www.foo.bar/image2.png"): {
+                    HttpUrl("https://www.foo.bar/image2.png")
+                }
+            },
+            id="simple_srcset_no_src",
+        ),
+        pytest.param(
+            '<img srcset="https://www.foo.bar/image1.png 640w, '
+            'https://www.foo.bar/image2.png 1024w"></img>',
+            '<img src="content/www.foo.bar/image2.png"></img>',
+            {
+                ZimPath("www.foo.bar/image2.png"): {
+                    HttpUrl("https://www.foo.bar/image2.png")
+                }
+            },
+            id="simple_srcset_2_no_src",
+        ),
+        pytest.param(
+            '<img src="https://www.foo.bar/image3.png" '
+            'srcset="https://www.foo.bar/image1.png 640w, '
+            'https://www.foo.bar/ima ge2.png 1024w" '
+            'sizes="(max-width: 300px) 85vw, 300px" '
+            'alt="Image"></img>',
+            '<img alt="Image" src="content/www.foo.bar/ima%20ge2.png"></img>',
+            {
+                ZimPath("www.foo.bar/ima ge2.png"): {
+                    HttpUrl("https://www.foo.bar/ima ge2.png")
+                }
+            },
+            id="simple_srcset_2",
+        ),
+    ],
+)
+def test_html_img_rewriting(
+    url_rewriter: HtmlUrlsRewriter,
+    html_rewriter: HtmlRewriter,
+    source_html: str,
+    expected_html: str,
+    expected_items_to_download: dict[ZimPath, set[HttpUrl]],
+):
+    rewritten = html_rewriter.rewrite(source_html)
+    assert rewritten.content == expected_html
+    assert rewritten.title == ""
+    assert url_rewriter.items_to_download == expected_items_to_download
diff --git a/scraper/tests/test_utils.py b/scraper/tests/test_utils.py
index 843facb..3185c37 100644
--- a/scraper/tests/test_utils.py
+++ b/scraper/tests/test_utils.py
@@ -2,7 +2,7 @@
 
 import pytest
 
-from mindtouch2zim.utils import get_asset_path_from_url
+from mindtouch2zim.utils import get_asset_path_from_url, is_better_srcset_descriptor
 
 
 @pytest.mark.parametrize(
@@ -57,3 +57,37 @@ def test_get_asset_path_from_url(
     assert get_asset_path_from_url(
         online_url, [Path(path) for path in already_used_paths]
     ) == Path(expected_path)
+
+
+@pytest.mark.parametrize(
+    "new_descriptor, current_best_descriptor, expected_result",
+    [
+        pytest.param(None, None, False, id="all_none"),
+        pytest.param(None, "1024w", False, id="new_none"),
+        pytest.param("1024w", None, True, id="current_none"),
+        pytest.param("1024w", "640w", True, id="w_current_better"),
+        pytest.param("640w", "1024w", False, id="w_current_worse"),
+        pytest.param("1024w", "1024w", False, id="w_equal"),
+        pytest.param("3x", "2x", True, id="x_current_better"),
+        pytest.param("2x", "3x", False, id="x_current_worse"),
+        pytest.param("3x", "3x", False, id="x_equal"),
+        pytest.param("3x", "3x", False, id="x_w_mixed1"),
+        pytest.param("1024w", "3x", False, id="x_w_mixed1"),
+        pytest.param("3x", "1024w", False, id="x_w_mixed2"),
+        pytest.param("3w", "1024x", False, id="x_w_mixed3"),
+        pytest.param("1024x", "3w", False, id="x_w_mixed4"),
+    ],
+)
+def test_is_better_srcset_descriptor(
+    new_descriptor: str | None,
+    current_best_descriptor: str | None,
+    *,
+    expected_result: bool,
+):
+    assert (
+        is_better_srcset_descriptor(
+            new_descriptor=new_descriptor,
+            current_best_descriptor=current_best_descriptor,
+        )
+        == expected_result
+    )

From 0f43b02d062148d47c8dc7a1ff400675b3e74d5c Mon Sep 17 00:00:00 2001
From: benoit74 <benoit74@users.noreply.github.com>
Date: Tue, 5 Nov 2024 08:08:31 +0000
Subject: [PATCH 3/4] Move all html rewriting code to dedicated module

---
 scraper/src/mindtouch2zim/errors.py         |  16 ++
 scraper/src/mindtouch2zim/html_rewriting.py | 163 +++++++++++++++++-
 scraper/src/mindtouch2zim/processor.py      | 180 +-------------------
 scraper/src/mindtouch2zim/zimconfig.py      |   6 +-
 4 files changed, 181 insertions(+), 184 deletions(-)
 create mode 100644 scraper/src/mindtouch2zim/errors.py

diff --git a/scraper/src/mindtouch2zim/errors.py b/scraper/src/mindtouch2zim/errors.py
new file mode 100644
index 0000000..34bd1cd
--- /dev/null
+++ b/scraper/src/mindtouch2zim/errors.py
@@ -0,0 +1,16 @@
+class InvalidFormatError(Exception):
+    """Raised when a user supplied template has an invalid parameter."""
+
+    pass
+
+
+class UnsupportedTagError(Exception):
+    """An exception raised when an HTML tag is not expected to be encountered"""
+
+    pass
+
+
+class NoIllustrationFoundError(Exception):
+    """An exception raised when no suitable illustration has been found"""
+
+    pass
diff --git a/scraper/src/mindtouch2zim/html_rewriting.py b/scraper/src/mindtouch2zim/html_rewriting.py
index 120fabd..5c5d171 100644
--- a/scraper/src/mindtouch2zim/html_rewriting.py
+++ b/scraper/src/mindtouch2zim/html_rewriting.py
@@ -1,13 +1,172 @@
+import re
+
 from zimscraperlib.rewriting.html import (
     AttrsList,
     format_attr,
     get_attr_value_from,
 )
 from zimscraperlib.rewriting.html import rules as html_rules
-from zimscraperlib.rewriting.url_rewriting import ArticleUrlRewriter
+from zimscraperlib.rewriting.url_rewriting import (
+    ArticleUrlRewriter,
+    HttpUrl,
+    RewriteResult,
+    ZimPath,
+)
 
-from mindtouch2zim.processor import HtmlUrlsRewriter
+from mindtouch2zim.client import LibraryPage
+from mindtouch2zim.constants import logger
+from mindtouch2zim.errors import UnsupportedTagError
 from mindtouch2zim.utils import is_better_srcset_descriptor
+from mindtouch2zim.vimeo import get_vimeo_thumbnail_url
+
+# remove all standard rules, they are not adapted to Vue.JS UI
+html_rules.rewrite_attribute_rules.clear()
+html_rules.rewrite_data_rules.clear()
+html_rules.rewrite_tag_rules.clear()
+
+
+@html_rules.rewrite_attribute()
+def rewrite_href_src_attributes(
+    tag: str,
+    attr_name: str,
+    attr_value: str | None,
+    url_rewriter: ArticleUrlRewriter,
+    base_href: str | None,
+):
+    """Rewrite href and src attributes"""
+    if attr_name not in ("href", "src") or not attr_value:
+        return
+    if not isinstance(url_rewriter, HtmlUrlsRewriter):
+        raise Exception("Expecting HtmlUrlsRewriter")
+    new_attr_value = None
+    if tag == "a":
+        rewrite_result = url_rewriter(
+            attr_value, base_href=base_href, rewrite_all_url=False
+        )
+        # rewrite links for proper navigation inside ZIM Vue.JS UI (if inside ZIM) or
+        # full link (if outside the current library)
+        new_attr_value = (
+            f"#/{rewrite_result.rewriten_url[len(url_rewriter.library_path.value) :]}"
+            if rewrite_result.rewriten_url.startswith(url_rewriter.library_path.value)
+            else rewrite_result.rewriten_url
+        )
+    if not new_attr_value:
+        # we do not (yet) support other tags / attributes so we fail the scraper
+        raise ValueError(
+            f"Empty new value when rewriting {attr_value} from {attr_name} in {tag} tag"
+        )
+    return (attr_name, new_attr_value)
+
+
+@html_rules.rewrite_tag()
+def refuse_unsupported_tags(tag: str):
+    """Stop scraper if unsupported tag is encountered"""
+    if tag not in ["picture"]:
+        return
+    raise UnsupportedTagError(f"Tag {tag} is not yet supported in this scraper")
+
+
+YOUTUBE_IFRAME_RE = re.compile(r".*youtube(?:-\w+)*\.\w+\/embed\/(?P<id>.*?)(?:\?.*)*$")
+VIMEO_IFRAME_RE = re.compile(r".*vimeo(?:-\w+)*\.\w+\/video\/(?:.*?)(?:\?.*)*$")
+
+
+@html_rules.rewrite_tag()
+def rewrite_iframe_tags(
+    tag: str,
+    attrs: AttrsList,
+    base_href: str | None,
+    url_rewriter: ArticleUrlRewriter,
+):
+    """Rewrite youtube and vimeo iframes to remove player until video is included"""
+    if tag not in ["iframe"]:
+        return
+    if not isinstance(url_rewriter, HtmlUrlsRewriter):
+        raise Exception("Expecting HtmlUrlsRewriter")
+    src = get_attr_value_from(attrs=attrs, name="src")
+    if not src:
+        raise UnsupportedTagError("Unsupported empty src in iframe")
+    image_rewriten_url = None
+    try:
+        if ytb_match := YOUTUBE_IFRAME_RE.match(src):
+            rewrite_result = url_rewriter(
+                f'https://i.ytimg.com/vi/{ytb_match.group("id")}/hqdefault.jpg',
+                base_href=base_href,
+            )
+            url_rewriter.add_item_to_download(rewrite_result)
+            image_rewriten_url = rewrite_result.rewriten_url
+        elif VIMEO_IFRAME_RE.match(src):
+            rewrite_result = url_rewriter(
+                get_vimeo_thumbnail_url(src),
+                base_href=base_href,
+            )
+            url_rewriter.add_item_to_download(rewrite_result)
+            image_rewriten_url = rewrite_result.rewriten_url
+        else:
+            logger.debug(f"iframe pointing to {src} will not have any preview")
+    except Exception as exc:
+        logger.warning(f"Failed to rewrite iframe with src {src}", exc_info=exc)
+
+    if image_rewriten_url:
+        return (
+            f'<a href="{src}" target="_blank">'
+            f'<div class="zim-removed-video">'
+            f'<img src="content/{image_rewriten_url}">'
+            "</img>"
+            "</div>"
+            "</a>"
+            '<iframe style="display: none;">'  # fake opening tag just to remove iframe
+        )
+    else:
+        # replace iframe with text indicating the online URL which has not been ZIMed
+        return (
+            f"This content is not inside the ZIM. "
+            f'View content online at <a href="{src}" target="_blank">'
+            f"<div>"
+            f"{src}"
+            "</div>"
+            "</a>"
+            '<iframe style="display: none;">'  # fake opening tag just to remove iframe
+        )
+
+
+class HtmlUrlsRewriter(ArticleUrlRewriter):
+    """A rewriter for HTML processing
+
+    This rewriter does not store items to download on-the-fly but has containers and
+    metadata so that HTML rewriting rules can decide what needs to be downloaded
+    """
+
+    def __init__(
+        self, library_url: str, page: LibraryPage, existing_zim_paths: set[ZimPath]
+    ):
+        super().__init__(
+            article_url=HttpUrl(f"{library_url}/{page.path}"),
+            article_path=ZimPath("index.html"),
+            existing_zim_paths=existing_zim_paths,
+        )
+        self.library_url = library_url
+        self.library_path = ArticleUrlRewriter.normalize(HttpUrl(f"{library_url}/"))
+        self.items_to_download: dict[ZimPath, set[HttpUrl]] = {}
+
+    def __call__(
+        self, item_url: str, base_href: str | None, *, rewrite_all_url: bool = True
+    ) -> RewriteResult:
+        result = super().__call__(item_url, base_href, rewrite_all_url=rewrite_all_url)
+        return result
+
+    def add_item_to_download(self, rewrite_result: RewriteResult):
+        """Add item to download based on rewrite result"""
+        if rewrite_result.zim_path is not None:
+            # if item is expected to be inside the ZIM, store asset information so that
+            # we can download it afterwards
+            if rewrite_result.zim_path in self.items_to_download:
+                self.items_to_download[rewrite_result.zim_path].add(
+                    HttpUrl(rewrite_result.absolute_url)
+                )
+            else:
+                self.items_to_download[rewrite_result.zim_path] = {
+                    HttpUrl(rewrite_result.absolute_url)
+                }
 
 
 @html_rules.rewrite_tag()
diff --git a/scraper/src/mindtouch2zim/processor.py b/scraper/src/mindtouch2zim/processor.py
index 33d0f7e..90c9c24 100644
--- a/scraper/src/mindtouch2zim/processor.py
+++ b/scraper/src/mindtouch2zim/processor.py
@@ -16,8 +16,7 @@
 from zimscraperlib.image.conversion import convert_svg2png
 from zimscraperlib.image.probing import format_for
 from zimscraperlib.rewriting.css import CssRewriter
-from zimscraperlib.rewriting.html import AttrsList, HtmlRewriter, get_attr_value_from
-from zimscraperlib.rewriting.html import rules as html_rules
+from zimscraperlib.rewriting.html import HtmlRewriter
 from zimscraperlib.rewriting.url_rewriting import (
     ArticleUrlRewriter,
     HttpUrl,
@@ -42,41 +41,18 @@
     logger,
     web_session,
 )
+from mindtouch2zim.errors import NoIllustrationFoundError
 from mindtouch2zim.html import get_text
+from mindtouch2zim.html_rewriting import HtmlUrlsRewriter
 from mindtouch2zim.ui import (
     ConfigModel,
     PageContentModel,
     PageModel,
     SharedModel,
 )
-from mindtouch2zim.vimeo import get_vimeo_thumbnail_url
 from mindtouch2zim.zimconfig import ZimConfig
 
 
-class InvalidFormatError(Exception):
-    """Raised when a user supplied template has an invalid parameter."""
-
-    pass
-
-
-class MissingDocumentError(Exception):
-    """Raised when the user specified a slug that doesn't exist."""
-
-    pass
-
-
-class UnsupportedTagError(Exception):
-    """An exception raised when an HTML tag is not expected to be encountered"""
-
-    pass
-
-
-class NoIllustrationFoundError(Exception):
-    """An exception raised when no suitable illustration has been found"""
-
-    pass
-
-
 class ContentFilter(BaseModel):
     """Supports filtering documents by user provided attributes."""
 
@@ -646,156 +622,6 @@ def _add_indexing_item_to_zim(
         )
 
 
-# remove all standard rules, they are not adapted to Vue.JS UI
-html_rules.rewrite_attribute_rules.clear()
-html_rules.rewrite_data_rules.clear()
-html_rules.rewrite_tag_rules.clear()
-
-
-@html_rules.rewrite_attribute()
-def rewrite_href_src_attributes(
-    tag: str,
-    attr_name: str,
-    attr_value: str | None,
-    url_rewriter: ArticleUrlRewriter,
-    base_href: str | None,
-):
-    """Rewrite href and src attributes"""
-    if attr_name not in ("href", "src") or not attr_value:
-        return
-    if not isinstance(url_rewriter, HtmlUrlsRewriter):
-        raise Exception("Expecting HtmlUrlsRewriter")
-    new_attr_value = None
-    if tag == "a":
-        rewrite_result = url_rewriter(
-            attr_value, base_href=base_href, rewrite_all_url=False
-        )
-        # rewrite links for proper navigation inside ZIM Vue.JS UI (if inside ZIM) or
-        # full link (if outside the current library)
-        new_attr_value = (
-            f"#/{rewrite_result.rewriten_url[len(url_rewriter.library_path.value) :]}"
-            if rewrite_result.rewriten_url.startswith(url_rewriter.library_path.value)
-            else rewrite_result.rewriten_url
-        )
-    if not new_attr_value:
-        # we do not (yet) support other tags / attributes so we fail the scraper
-        raise ValueError(
-            f"Empty new value when rewriting {attr_value} from {attr_name} in {tag} tag"
-        )
-    return (attr_name, new_attr_value)
-
-
-@html_rules.rewrite_tag()
-def refuse_unsupported_tags(tag: str):
-    """Stop scraper if unsupported tag is encountered"""
-    if tag not in ["picture"]:
-        return
-    raise UnsupportedTagError(f"Tag {tag} is not yet supported in this scraper")
-
-
-YOUTUBE_IFRAME_RE = re.compile(r".*youtube(?:-\w+)*\.\w+\/embed\/(?P<id>.*?)(?:\?.*)*$")
-VIMEO_IFRAME_RE = re.compile(r".*vimeo(?:-\w+)*\.\w+\/video\/(?:.*?)(?:\?.*)*$")
-
-
-@html_rules.rewrite_tag()
-def rewrite_iframe_tags(
-    tag: str,
-    attrs: AttrsList,
-    base_href: str | None,
-    url_rewriter: ArticleUrlRewriter,
-):
-    """Rewrite youtube and vimeo iframes to remove player until video is included"""
-    if tag not in ["iframe"]:
-        return
-    if not isinstance(url_rewriter, HtmlUrlsRewriter):
-        raise Exception("Expecting HtmlUrlsRewriter")
-    src = get_attr_value_from(attrs=attrs, name="src")
-    if not src:
-        raise UnsupportedTagError("Unsupported empty src in iframe")
-    image_rewriten_url = None
-    try:
-        if ytb_match := YOUTUBE_IFRAME_RE.match(src):
-            rewrite_result = url_rewriter(
-                f'https://i.ytimg.com/vi/{ytb_match.group("id")}/hqdefault.jpg',
-                base_href=base_href,
-            )
-            url_rewriter.add_item_to_download(rewrite_result)
-            image_rewriten_url = rewrite_result.rewriten_url
-        elif VIMEO_IFRAME_RE.match(src):
-            rewrite_result = url_rewriter(
-                get_vimeo_thumbnail_url(src),
-                base_href=base_href,
-            )
-            url_rewriter.add_item_to_download(rewrite_result)
-            image_rewriten_url = rewrite_result.rewriten_url
-        else:
-            logger.debug(f"iframe pointing to {src} will not have any preview")
-    except Exception as exc:
-        logger.warning(f"Failed to rewrite iframe with src {src}", exc_info=exc)
-
-    if image_rewriten_url:
-        return (
-            f'<a href="{src}" target="_blank">'
-            f'<div class="zim-removed-video">'
-            f'<img src="content/{image_rewriten_url}">'
-            "</img>"
-            "</div>"
-            "</a>"
-            '<iframe style="display: none;">'  # fake opening tag just to remove iframe
-        )
-    else:
-        # replace iframe with text indicating the online URL which has not been ZIMed
-        return (
-            f"This content is not inside the ZIM. "
-            f'View content online at <a href="{src}" target="_blank">'
-            f"<div>"
-            f"{src}"
-            "</div>"
-            "</a>"
-            '<iframe style="display: none;">'  # fake opening tag just to remove iframe
-        )
-
-
-class HtmlUrlsRewriter(ArticleUrlRewriter):
-    """A rewriter for HTML processing
-
-    This rewriter does not store items to download on-the-fly but has containers and
-    metadata so that HTML rewriting rules can decide what needs to be downloaded
-    """
-
-    def __init__(
-        self, library_url: str, page: LibraryPage, existing_zim_paths: set[ZimPath]
-    ):
-        super().__init__(
-            article_url=HttpUrl(f"{library_url}/{page.path}"),
-            article_path=ZimPath("index.html"),
-            existing_zim_paths=existing_zim_paths,
-        )
-        self.library_url = library_url
-        self.library_path = ArticleUrlRewriter.normalize(HttpUrl(f"{library_url}/"))
-        self.items_to_download: dict[ZimPath, set[HttpUrl]] = {}
-
-    def __call__(
-        self, item_url: str, base_href: str | None, *, rewrite_all_url: bool = True
-    ) -> RewriteResult:
-        result = super().__call__(item_url, base_href, rewrite_all_url=rewrite_all_url)
-        return result
-
-    def add_item_to_download(self, rewrite_result: RewriteResult):
-        """Add item to download based on rewrite result"""
-        if rewrite_result.zim_path is not None:
-            # if item is expected to be inside the ZIM, store asset information so that
-            # we can download it afterwards
-            if rewrite_result.zim_path in self.items_to_download:
-                self.items_to_download[rewrite_result.zim_path].add(
-                    HttpUrl(rewrite_result.absolute_url)
-                )
-            else:
-                self.items_to_download[rewrite_result.zim_path] = {
-                    HttpUrl(rewrite_result.absolute_url)
-                }
-
-
 class CssUrlsRewriter(ArticleUrlRewriter):
     """A rewriter for CSS processing, storing items to download as URL as processed"""
 
diff --git a/scraper/src/mindtouch2zim/zimconfig.py b/scraper/src/mindtouch2zim/zimconfig.py
index 107991b..09daa21 100644
--- a/scraper/src/mindtouch2zim/zimconfig.py
+++ b/scraper/src/mindtouch2zim/zimconfig.py
@@ -2,11 +2,7 @@
 
 from pydantic import BaseModel
 
-
-class InvalidFormatError(Exception):
-    """Raised when a user supplied template has an invalid parameter."""
-
-    pass
+from mindtouch2zim.errors import InvalidFormatError
 
 
 class ZimConfig(BaseModel):

From 3a7a734094bddd2373c883e29e3177f5a8a83aa7 Mon Sep 17 00:00:00 2001
From: benoit74 <benoit74@users.noreply.github.com>
Date: Tue, 5 Nov 2024 08:36:44 +0000
Subject: [PATCH 4/4] Add missing tests of HTML rewriting

---
 scraper/src/mindtouch2zim/errors.py         |   6 +
 scraper/src/mindtouch2zim/html_rewriting.py |   6 +-
 scraper/tests/test_html_rewriting.py        | 141 +++++++++++++++++++-
 3 files changed, 149 insertions(+), 4 deletions(-)

diff --git a/scraper/src/mindtouch2zim/errors.py b/scraper/src/mindtouch2zim/errors.py
index 34bd1cd..418650c 100644
--- a/scraper/src/mindtouch2zim/errors.py
+++ b/scraper/src/mindtouch2zim/errors.py
@@ -10,6 +10,12 @@ class UnsupportedTagError(Exception):
     pass
 
 
+class UnsupportedHrefSrcError(Exception):
+    """An exception raised when an href or src is not expected to be encountered"""
+
+    pass
+
+
 class NoIllustrationFoundError(Exception):
     """An exception raised when no suitable illustration has been found"""
 
diff --git a/scraper/src/mindtouch2zim/html_rewriting.py b/scraper/src/mindtouch2zim/html_rewriting.py
index 5c5d171..ec5081e 100644
--- a/scraper/src/mindtouch2zim/html_rewriting.py
+++ b/scraper/src/mindtouch2zim/html_rewriting.py
@@ -15,7 +15,7 @@
 
 from mindtouch2zim.client import LibraryPage
 from mindtouch2zim.constants import logger
-from mindtouch2zim.errors import UnsupportedTagError
+from mindtouch2zim.errors import UnsupportedHrefSrcError, UnsupportedTagError
 from mindtouch2zim.utils import is_better_srcset_descriptor
 from mindtouch2zim.vimeo import get_vimeo_thumbnail_url
 
@@ -52,8 +52,8 @@ def rewrite_href_src_attributes(
         )
     if not new_attr_value:
         # we do not (yet) support other tags / attributes so we fail the scraper
-        raise ValueError(
-            f"Empty new value when rewriting {attr_value} from {attr_name} in {tag} tag"
+        raise UnsupportedHrefSrcError(
+            f"Unsupported {attr_name} encountered in {tag} tag (value: {attr_value})"
         )
     return (attr_name, new_attr_value)
 
diff --git a/scraper/tests/test_html_rewriting.py b/scraper/tests/test_html_rewriting.py
index 201aadb..ef04aea 100644
--- a/scraper/tests/test_html_rewriting.py
+++ b/scraper/tests/test_html_rewriting.py
@@ -6,6 +6,7 @@
 )
 
 from mindtouch2zim.client import LibraryPage
+from mindtouch2zim.errors import UnsupportedHrefSrcError, UnsupportedTagError
 from mindtouch2zim.html_rewriting import HtmlUrlsRewriter
 
 
@@ -14,7 +15,9 @@ def url_rewriter() -> HtmlUrlsRewriter:
     return HtmlUrlsRewriter(
         library_url="https://www.acme.com",
         page=LibraryPage(id="123", title="a page", path="A_Page"),
-        existing_zim_paths=set(),
+        existing_zim_paths={
+            ZimPath("www.acme.com/existing.html"),
+        },
     )
 
 
@@ -97,3 +100,139 @@ def test_html_img_rewriting(
     assert rewritten.content == expected_html
     assert rewritten.title == ""
     assert url_rewriter.items_to_download == expected_items_to_download
+
+
+@pytest.mark.parametrize(
+    "source_html, expected_html, expected_items_to_download",
+    [
+        pytest.param(
+            '<iframe src="https://www.youtube.com/embed/sQaEthBmZB0?vq=hd1080" '
+            'frameborder="0" allowfullscreen="true" '
+            'style="width: 100%; height: 100%; position: absolute"></iframe>',
+            '<a href="https://www.youtube.com/embed/sQaEthBmZB0?vq=hd1080" '
+            'target="_blank">'
+            '<div class="zim-removed-video">'
+            '<img src="content/i.ytimg.com.fuzzy.replayweb.page/'
+            'vi/sQaEthBmZB0/thumbnail.jpg"></img>'
+            "</div>"
+            "</a>"
+            '<iframe style="display: none;"></iframe>',
+            {
+                ZimPath(
+                    "i.ytimg.com.fuzzy.replayweb.page/vi/sQaEthBmZB0/thumbnail.jpg"
+                ): {HttpUrl("https://i.ytimg.com/vi/sQaEthBmZB0/hqdefault.jpg")}
+            },
+            id="youtube",
+        ),
+        pytest.param(
+            '<iframe src="https://player.vimeo.com/video/153300296" '
+            'frameborder="0" allowfullscreen="true" '
+            'style="width: 100%; height: 100%; position: absolute"></iframe>',
+            '<a href="https://player.vimeo.com/video/153300296" '
+            'target="_blank">'
+            '<div class="zim-removed-video">'
+            '<img src="content/i.vimeocdn.com/video/553546340-'
+            '35aa6d23b04e9bdaf254c3cfc4da56bcfd7ff3f75a517c485536082edbf547dd-d_640">'
+            "</img>"
+            "</div>"
+            "</a>"
+            '<iframe style="display: none;"></iframe>',
+            {
+                ZimPath(
+                    "i.vimeocdn.com/video/553546340-"
+                    "35aa6d23b04e9bdaf254c3cfc4da56bcfd7ff3f75a517c485536082edbf547dd-"
+                    "d_640"
+                ): {
+                    HttpUrl(
+                        "https://i.vimeocdn.com/video/553546340-"
+                        "35aa6d23b04e9bdaf254c3cfc4da56bcfd7ff3f75a517c485536082e"
+                        "dbf547dd-d_640"
+                    )
+                }
+            },
+            id="vimeo",
+        ),
+        pytest.param(
+            '<iframe src="https://www.acme.com/embed/sQaEthBmZB0?vq=hd1080" '
+            'frameborder="0" allowfullscreen="true" '
+            'style="width: 100%; height: 100%; position: absolute"></iframe>',
+            "This content is not inside the ZIM. View content online at "
+            '<a href="https://www.acme.com/embed/sQaEthBmZB0?vq=hd1080" '
+            'target="_blank">'
+            "<div>https://www.acme.com/embed/sQaEthBmZB0?vq=hd1080</div>"
+            "</a>"
+            '<iframe style="display: none;"></iframe>',
+            {},
+            id="unhandled",
+        ),
+    ],
+)
+def test_html_iframe_rewriting(
+    url_rewriter: HtmlUrlsRewriter,
+    html_rewriter: HtmlRewriter,
+    source_html: str,
+    expected_html: str,
+    expected_items_to_download: dict[ZimPath, set[HttpUrl]],
+):
+    rewritten = html_rewriter.rewrite(source_html)
+    assert rewritten.content == expected_html
+    assert rewritten.title == ""
+    assert url_rewriter.items_to_download == expected_items_to_download
+
+
+def test_html_picture_rewriting(html_rewriter: HtmlRewriter):
+    with pytest.raises(UnsupportedTagError):
+        html_rewriter.rewrite("<picture>")
+
+
+def test_html_script_rewriting(html_rewriter: HtmlRewriter):
+    with pytest.raises(UnsupportedHrefSrcError):
+        html_rewriter.rewrite("<script src='script.js'>")
+
+
+@pytest.mark.parametrize(
+    "source_html, expected_html, expected_items_to_download",
+    [
+        pytest.param(
+            '<a href="https://www.acme.com/existing.html">Page</a>',
+            '<a href="#/existing.html">Page</a>',
+            {},
+            id="internal_absolute",
+        ),
+        pytest.param(
+            '<a href="/existing.html">Page 1</a>',
+            '<a href="#/existing.html">Page 1</a>',
+            {},
+            id="internal_root",
+        ),
+        pytest.param(
+            '<a href="../existing.html">Page 1</a>',
+            '<a href="#/existing.html">Page 1</a>',
+            {},
+            id="internal_relative",
+        ),
+        pytest.param(
+            '<a href="../outside.html">Page 1</a>',
+            '<a href="https://www.acme.com/outside.html">Page 1</a>',
+            {},
+            id="external_relative",
+        ),
+        pytest.param(
+            '<a href="https://www.foo.bar/index.html">Page 2</a>',
+            '<a href="https://www.foo.bar/index.html">Page 2</a>',
+            {},
+            id="external",
+        ),
+    ],
+)
+def test_html_href_rewriting(
+    url_rewriter: HtmlUrlsRewriter,
+    html_rewriter: HtmlRewriter,
+    source_html: str,
+    expected_html: str,
+    expected_items_to_download: dict[ZimPath, set[HttpUrl]],
+):
+    rewritten = html_rewriter.rewrite(source_html)
+    assert rewritten.content == expected_html
+    assert rewritten.title == ""
+    assert url_rewriter.items_to_download == expected_items_to_download