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

Fix locationOf for empty hash targets in Webkit #1300

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elliotdickison
Copy link

Internal hash links were broken in Webkit if they pointed to empty elements. This change fixes that by temporarily inserting a zero-width space into elements before measuring them.

Internal hash links were broken in Webkit if they pointed to empty elements. This change fixes that by temporarily inserting a zero-width space into elements before measuring them.
@johnfactotum
Copy link
Contributor

johnfactotum commented Nov 15, 2022

The function seems to contradict itself. In WebKit, in the case of a collapsed range, it tries to use an element instead of the range, but then in the case of an element, it does the exact opposite, creating a range from the element, which in the case of an empty element would result in a collapsed range.

Testing it a bit, with the latest WebKitGTK, I believe the problem is actually that a collapsed range doesn't return the correct rect. For elements, it reports the correct rect, whether the element is empty or in columns or not. So I think this can be fixed by simply removing the check all together:

diff --git a/src/contents.js b/src/contents.js
index 3effe72..63d0482 100644
--- a/src/contents.js
+++ b/src/contents.js
@@ -666,14 +666,7 @@ class Contents {
                        let id = target.substring(target.indexOf("#")+1);
                        let el = this.document.getElementById(id);
                        if(el) {
-                               if (isWebkit) {
-                                       // Webkit reports incorrect bounding rects in Columns
-                                       let newRange = new Range();
-                                       newRange.selectNode(el);
-                                       position = newRange.getBoundingClientRect();
-                               } else {
-                                       position = el.getBoundingClientRect();
-                               }
+                               position = el.getBoundingClientRect();
                        }
                }

@elliotdickison
Copy link
Author

Ha good catch, works for me!

@elliotdickison
Copy link
Author

@fchasen Any chance you could merge this?

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

Successfully merging this pull request may close these issues.

None yet

2 participants