readium / swift-toolkit

A toolkit for ebooks, audiobooks and comics written in Swift
https://readium.org/mobile/
BSD 3-Clause "New" or "Revised" License
223 stars 96 forks source link

Decorations are incorrectly applied to paragraphs that start with a special character #362

Open streamg opened 7 months ago

streamg commented 7 months ago

Bug Report

When applying decorations to ranges that are within a paragraph that starts with a character that has a different font, the decorations are shifted with one character.

What happened?

Taking the first paragraph from a epub source file:

<p class="x03-CO-Body-Text">Everyone knows that Harry began his career in One Direction&#x00A0;.&#x00A0;.&#x00A0;. or did he?! The name of his first band is super ironic considering the charmed career of charming Harry Styles—because in addition to skyrocketing, his career has gone in many directions!</p>

You can notice that the letter E is formatted differently. Because of that when applying a decoration on any of the words in this paragraph the decoration will be shifted with one character. IMG_A2656292FF4C-1

Example of a decoration described by locator:

{\"href\":\"/OEBPS/xhtml/05_Chapter_1_Styles_in_E.xhtml\",\"locations\":{\"progression\":0.062947067238912732,\"totalProgression\":0.2135789222699094},\"text\":{\"after\":\" his career in One Direction . . . or did he?! The name\",\"before\":\"STYLES IN EVERY DIRECTION\\n\\t\\t\\tEveryone knows that Harry \",\"highlight\":\"began\"},\"title\":\"Chapter 1: Styles in Every Direction\",\"type\":\"application/xhtml+xml\"}

Another issue us if we select a word from that paragraph. The selection Locator will describe the text but the after field has an additional character (the last character from the highlight). E.g. selecting the word "Harry" will return this Locator for the selection:

{\"href\":\"/OEBPS/xhtml/05_Chapter_1_Styles_in_E.xhtml\",\"locations\":{\"position\":6,\"progression\":0,\"totalProgression\":0.20833333333333334},\"text\":{\"after\":\"y began his career in One Direction . . . or did he?! The name of his first band is super ironic considering the charmed career of charming Harry Styles—because in addition to skyrocketing, his\",\"before\":\"CHAPTER 1\\n\\t\\t\\tSTYLES IN EVERY DIRECTION\\n\\t\\t\\tEveryone knows that\",\"highlight\":\"Harry\"},\"title\":\"Chapter 1: Styles in Every Direction\",\"type\":\"application/xhtml+xml\"}

Please find a sample epub here: 9780593383865_ewyrds_preview.epub.zip

The chapter name is 05_Chapter_1_Styles_in_E.xhtml

Expected behavior

The decorations should be properly applied and the current selection should return the correct locator.

How to reproduce?

Apply decorations for words in the mentioned paragraph and read the currentSelection.locator by selecting a word in the same paragraph.

<p class="x03-CO-Body-Text">Everyone knows that Harry began his career in One Direction&#x00A0;.&#x00A0;.&#x00A0;. or did he?! The name of his first band is super ironic considering the charmed career of charming Harry Styles—because in addition to skyrocketing, his career has gone in many directions!</p>

Environment

Development environment

macOS: 13.5.1 platform: arm64 carthage: Xcode 14.3 Build version 14E222b

Testing device

iOS version: 16.6.1 (not OS related) Model (e.g. iPhone 11 Pro Max): iPhone 12 Is it an emulator? No

Additional context

mickael-menu commented 7 months ago

Are you able to reproduce this issue with a highlight in the stock Test App? I'll reopen the issue if that's the case.

I believe the issue is caused by the strategy you used to produce the DOM range from the locator in your caching PR: https://github.com/readium/swift-toolkit/pull/354 I'll answer with more details on the PR.

mickael-menu commented 7 months ago

I'm reopening, I tested myself and could notice the same problem with a highlight.

mickael-menu commented 7 months ago

It looks like an issue in WebKit when using ::first-letter and getBoundingClientRects() which is used to layout the decorations.

You can reproduce it from the Safari Web Inspector:

  1. Select a portion of text.
  2. Get the range: const r = window.getSelection().getRangeAt(0)
  3. Observe the client rect: r.getBoundingClientRect() -> DOMRect {x: 197, y: 247, ...}
  4. Enable a property of the CSS pseudo-element ::first-letter on this paragraph. You can change only the text color which should not impact the position at all.
  5. Observe the client rect again, note that the x is shifted for some reason: r.getBoundingClientRect() -> DOMRect {x: 212, y: 247, ...}.

I don't think we can address this in the toolkit without removing the ::first-letter pseudo-elements.

mickael-menu commented 7 months ago

By the way it doesn't happen on your other sample because it doesn't use ::first-letter but a <span> element. In case you have control over the authoring...

Screenshot 2023-11-24 at 18 13 49