fix: running element page counter incorrect

[EnochGao]

#1325
I'm not sure if this modification is elegant, but the result seems to be correct. I'm not sure how to handle the total number of pages

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
vivliostyle	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 27, 2024 1:13am

[MurakamiShinyu]

Thank you for the fix! I will check tomorrow.

[EnochGao]

and target-counter in running element equally incorrect🙋🙋🙋

[MurakamiShinyu]

Suggested change

	let value = arr[0];
	if (arr[1]) {
	value = arr[1];
	}
	return format(value);
	return format(arr[arr.length - 1]);

value = arr[1] is not good when counter-reset: page happens multiple times, for example:

  @page :nth(2) {
    counter-reset: page 20;
  }
  @page :nth(3) {
    counter-reset: page 30;
  }

[MurakamiShinyu]

I'm not sure how to handle the total number of pages

I found the problem in CounterStore.finishLastPage() in counters.ts:

vivliostyle.js/packages/core/src/vivliostyle/counters.ts

Lines 877 to 886 in 126436b

	finishLastPage(viewport: Vgen.Viewport) {
	const nodes = viewport.root.querySelectorAll(`[${PAGES_COUNTER_ATTR}]`);
	const pages = this.currentPageCounters["page"][0];
	for (const node of nodes) {
	const key = node.getAttribute(PAGES_COUNTER_ATTR);
	const i = this.pagesCounterExprs.findIndex((o) => o.expr.key === key);
	Asserts.assert(i >= 0);
	node.textContent = this.pagesCounterExprs[i].format([pages]);
	}
	}

In this code, the "pages" counter value is obtained from the "page" counter value at the last page:

    const pages = this.currentPageCounters["page"][0];

It doesn't work when counter-reset: page is used for the same reason as return format(arr[0]); in getPageCounterVal(). However, there is a further problem.

In the CSS Paged Media spec, the definition of the pages counter is "Its value is always the total number of pages in the document" and not same as the "page" counter value at the last page. So the "pages" counter value should be obtained from the number of generated pages.

Suggested change:

    const pages = viewport.contentContainer.childElementCount;

[MurakamiShinyu]

With the last commit f5083bb the test result seems good except the last page with "??" remaining.

https://vivliostyle-7iz81mer5-vivliostyle.vercel.app/#src=https://gist.githubusercontent.com/MurakamiShinyu/7616ab180e72e38addafc1a7ea44d8d0/raw/issue-1325-running-element-target-counter.html

[MurakamiShinyu]

@EnochGao Thanks for the fix and the test case "counter_in_running.html".

The fix seems almost good, but I found a problem that it does not work when a page-based
counter other than "page" is used. Example: https://gist.githubusercontent.com/MurakamiShinyu/7616ab180e72e38addafc1a7ea44d8d0/raw/counter_in_running_custom_page.html

[MurakamiShinyu]

Thanks. The fix 72fa5bc is good for custom page-based counter, but still has a problem with element-based counter.

I made a test case which tests element-based counter in addition to custom page-based counter:

https://gist.githubusercontent.com/MurakamiShinyu/7616ab180e72e38addafc1a7ea44d8d0/raw/counter_in_running_custom_page2.html

Test: https://vivliostyle-git-fork-enochgao-fix-counter-vivliostyle.vercel.app/#src=https://gist.githubusercontent.com/MurakamiShinyu/7616ab180e72e38addafc1a7ea44d8d0/raw/counter_in_running_custom_page2.html

The "(Section 11)" in the page header is wrong. It should be "(Section 2)".

---

I found this problem is caused by the following code in finishLastPage():

      const transformedId = node.getAttribute(TARGET_PAGE_COUNTER_ID_ATTR);
      const expr = this.targetPageCounterExprs.find(
        (o) => o.transformedId === transformedId,
      );

There are multiple target counter expressions with the same transformedId in this.targetPageCounterExprs and the wrong one is selected. I think we should use key instead of transformedId to identify the target counter expression. So the code should be changed to:

      const key = node.getAttribute(TARGET_PAGE_COUNTER_ATTR);
      const expr = this.targetPageCounterExprs.find((o) => o.expr.key === key);

If this change is ok, the TARGET_PAGE_COUNTER_ID_ATTR attribute is no longer needed and can be removed.

Also the naming TARGET_PAGE_COUNTER_ATTR is no longer appropriate because it is used for not only page-based counters but also element-based counters. I suggest renaming it to TARGET_COUNTER_ATTR and TARGET_PAGE_COUNTER_IN_RUNNING_ATTR to TARGET_COUNTER_IN_RUNNING_ATTR, and change their values to "data-vivliostyle-target-counter" and "data-vivliostyle-target-counter-in-running" respectively.

Please check the proposed changes in the following diff:
https://gist.github.com/MurakamiShinyu/d175de0ddb9175e3f2ba711735aa0e17

[EnochGao]

I see. Thanks for the correction.

[MurakamiShinyu]

This <div class="reset-page">reset page</div> element is not used for resetting page counter. So I think this should be removed.

Suggested change

<div class="reset-page">reset page</div>

[EnochGao]

ok

[MurakamiShinyu]

Thanks! LGTM 💯

[EnochGao]

Thanks! LGTM 💯

Thanks for the correction. Made me know more about this place