AppendVirtualColumnIfNeeded Firefox workaround

[wrCisco]

Firefox doesn't support break-before: "column", so appendVirtualColumnIfNeeded in navigator-html-injectables/src/helpers/document.ts is not generally effective there. The same problem is present in the updated version of the function in the Preferences API branch.

ts-toolkit/navigator-html-injectables/src/helpers/document.ts

Lines 19 to 43 in 2a44a91

	export function appendVirtualColumnIfNeeded(wnd: ReadiumWindow) {
	const id = "readium-virtual-page";
	let virtualCol = wnd.document.getElementById(id);
	if (getColumnCountPerScreen(wnd) !== 2) {
	if (virtualCol) {
	virtualCol.remove();
	}
	} else {
	const documentWidth = wnd.document.scrollingElement!.scrollWidth;
	const colCount = documentWidth / wnd.innerWidth;
	const hasOddColCount = (Math.round(colCount * 2) / 2) % 1 > 0.1;
	if (hasOddColCount) {
	if (virtualCol)
	virtualCol.remove();
	else {
	virtualCol = wnd.document.createElement("div");
	virtualCol.setAttribute("id", id);
	virtualCol.dataset.readium = "true";
	virtualCol.style.breakBefore = "column";
	virtualCol.innerHTML = "​"; // zero-width space
	wnd.document.body.appendChild(virtualCol);
	}
	}
	}
	}

I added a new check right after the line wnd.document.body.appendChild(virtualCol); and conditionally set the height of the virtualCol equal to the body's clientHeight:

const newDocWidth = wnd.document.scrollingElement!.scrollWidth;
const newColCount = newDocWidth / wnd.innerWidth;
if ((Math.round((newColCount) * 2) / 2) % 1 > 0.1) {
    virtualCol.style.height = wnd.document.body.clientHeight + 'px';
    virtualCol.style.backgroundColor = 'transparent';
    virtualCol.style.borderWidth = '0';
    virtualCol.style.outlineWidth = '0';
    virtualCol.style.margin = '0';
    virtualCol.style.padding = '0';
}

It seems to work ok in Firefox and Chrome, but I'm not sure if there are other scenarios that I didn't consider in which this solution could cause problems.

[JayPanoz]

Thanks for pointing this out.

The root of the issue being 14 years old, I’m not holding my breath for a quick resolution in that case.

In order to minimise possible side-effects, I’d probably be inclined to check for the lack of break-before: column support using the CSS.supports() static method and then adding the workaround for Firefox.

I can’t really tell if there are other scenarios myself at the moment, I’d be just super cautious with the things we added in the Preferences API such as zoom, etc., which may have edge cases that are difficult to predict. My main worry would be with things throwing flow off e.g. extra columns/frames being created.

If there are such issues, maybe there could be alternative options. In the Preferences API branch, it’s worth mentioning we are checking the number of columns that are actually needed for the scrollWidth is a multiple integer of the windows’s width – it was simpler and faster to do that instead of trying to handle virtual columns dynamically –, so maybe we could just set the width instead of appending columns at all. I can’t tell though as I have not tested that yet.

[JayPanoz]

Alright so could give this a quick look.

Setting width is a no-go since columns are set on the same element.

I can see the height solution working in an interesting way, although partly some side-effect of having the height being large enough:

The div flows without any fragmentation break but since its height is the height of the column, that works as it should always overflow into the next column.

I’ve tested an alternative workaround that perhaps keep the spirit of the original function for Firefox:

if (CSS.supports("break-before", "column")) {
  virtualCol.style.breakBefore = "column";
} else {
   virtualCol.style.breakInside = "avoid-column";
   virtualCol.style.height = document.documentElement.clientHeight * 0.8 + "px";
}

Basically, this makes sure the height is large enough that the virtualCol will be entirely laid out in a column by avoiding a break inside – that Firefox supports.

I’m probably leaning towards implementing something like this in the Preferences API branch for the time being:

if (CSS.supports("break-before", "column")) {
  virtualCol.style.breakBefore = "column";
} else if (CSS.supports("break-inside", "avoid-column") {
  virtualCol.style.breakInside = "avoid-column";
  virtualCol.style.height = document.documentElement.clientHeight * 0.8 + "px";
} else {
  virtualCol.style.height = document.documentElement.clientHeight + "px";
}

So that it can cover all cases with some peace of mind because we have to handle n virtual columns, and not just a single one.

Note clientHeight contains padding-block so I will also account for that in the Preferences API branch, although ReadiumCSS, that is used to fragment using columns, loosely protects against that.

@chocolatkey Do you reckon this should also be fixed in v1 considering it’s quite a significant bug impacting a major browser?

[wrCisco]

Hi @JayPanoz, thanks for testing this. The break-inside: avoid-column solution seems cool, are you saying that in the Preferences API branch the 80% (or what you will set) of the documentElement.clientHeight will always be enough to ensure that the div slides to a new column?

[JayPanoz]

@wrCisco in theory anything >50% should work in combination with the break-inside: avoid-column as the fragmentation algo should not be able to stack 2 divs in a single column w/o breaking inside one of them, so 80% is some additional cautiousness on my part, it's completely arbitrary.

I've not taken the deepest look into the rendering engine's source code and its quirks, mind you, but it seems reasonable to me right now after testing.

Being in charge of ReadiumCSS, the implementation of the Preferences API in ts-toolkit – which has a massive impact –, and leading the development of Thorium Web, you can be reassured I'll have to make sure it works w/o any noticeable side effect and resolve edge cases if they ever pop up.

Thanks again for pointing this out! 🙏

[chocolatkey]

Do you reckon this should also be fixed in v1 considering it’s quite a significant bug impacting a major browser?

Probably a good idea. You could check with @oscar-rivera-demarque who maintains the DeMarque implementation if this will break anything there since they are the biggest "old" ts-toolkit user

[JayPanoz]

As a resolution during the Readium Web meeting, we agreed to merge the fix through PR #106 in version 2. It has been marked as ready for review, although some details still have to be discussed.

Unless there is a strong request to also commit it into develop (version 1) I will do that on Friday.

[wrCisco]

I tried to do some testing, most of the time on Firefox I can turn the pages without problems, but if the last page of a section contains only one or two lines of text and is not a multiple of the "pages per view", the first added div does not shift to the next column and the bug is still there.

I'm not sure if I'm doing something wrong, I cloned the Preferences API branch, built the workspaces as specified in the READMEs and opened a book with the testapp.

[JayPanoz]

@wrCisco Thanks for catching that, my original assumption was actually wrong with the height as I was focused on multiple virtual columns so much I forgot that in theory a single one could fit w/o needing to avoid breakage inside.

So yeah, on Firefox, the virtual column being 80% height of the :root and the text in the column being less than 20% the virtual column can indeed fit. My bad, I will commit a fix.