Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In CfiNavigationLogic.getVisibleElements, ignore empty <a> tags. #289

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

tddpirate
Copy link

See E-mail to Daniel Weck from 25 Apr 2016

A bugfix in the code which manages bookmarks in Readium.
What happened was that after a bookmark was created in page N in a
chapter, when we tried to jump to the bookmark, the reader jumped to
page M such that 0 <= M < N.

The bug was traced to getVisibleElements, which is a function declared in
CfiNavigationLogic. Turns out that empty tags were considered to be visible in pages following the page in whose markup they appear.

It may be a good idea investigate if and which other empty tags should be
considered to be invisible as well.

percentVisible: visibilityPercentage
});
}
if (($element[0].tagName !== "A") || $node[0].nodeValue) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that be true for any element whose content model model is non-empty, but whose actual content is? (e.g. h1)

Copy link
Author

@tddpirate tddpirate Apr 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes.

  1. My patch is conservative in that it ignores empty text nodes only when they are in tags.
    It may be that we can safely treat all empty text nodes and their enclosing elements as invisible regardless of what checkVisibilityByRectangles() tells us.
  2. If we should not ignore all elements enclosing empty text nodes:
    Is there a function (either a DOM element function or a Readium-specific) for identifying elements with non-empty content model?
    If yes, its inverse should replace the ($element[0].tagName !== "A") test.
  3. It is possible that checkVisibilityByRectangles() has a bug (a corner case) in handling empty text nodes. At the moment I do not have time to look into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants