Skip to content

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.

2 participants