Fix copy / paste behavior when using Firefox or Edge [DOM based] (#51)

[anlambert]

I took some time trying to fix the annoying copy / paste behavior observed when using Firefox or Edge browsers. As explained in issue #51, blank lines are inserted in text editors after each pasted line of code, which is pretty annoying.

Based on my understanding, this is due to different behaviors among browsers when copying text from a multi-column based layout wrapped into a HTML pre element.

After numerous tryouts (notably using CSS tables instead of plain HTML ones), the only solution I found ensuring same copy/paste behavior for all browsers is the following. So instead of wrapping line numbers and code in a single table element, use the following approach instead:

1. render the lines number in a table with single column
2. render the lines of code in a div
3. wrap these in a div and make the table float left
4. adjust the left margin of the code div once inserted in the dom

With this approach, the annoying copy / paste behavior with Firefox and Edge goes away while obtaining the exact same rendering as before.

The only drawback I see with these changes is that they may break custom user scripts selecting hljs-ln-* element as the html layout has been slightly modified. Nevertheless, I have also improved the way to select line elements and documented it, so custom user scripts will not be difficult to fix.

[anlambert]

FYI, you can test the effect of that PR here for instance: https://archive.softwareheritage.org/browse/origin/https://git.videolan.org/git/vlc.git/visit/2019-02-05T01:58:05/content/src/libvlc.c/

I deployed to production today an updated version of that web application I am working on including the proposed fixes (gory details can be found here: https://forge.softwareheritage.org/rDWAPPSfcbbef809cdf1569276dd44c6422e0ee4e8645af).

[wcoder]

Hi @anlambert thanks for the hard work!

But after check this PR on my debug samples, I got some side effects, see:

Also, broken line breaks:

Debug samples: debug_html.zip

[anlambert]

Hi @wcoder, thanks for pointing me to your debug examples. Maybe these should be committed somewhere in the repository for testing purposes ?

I have updated this PR fixing some of the reported issues. Turns out that for my tests, the bootstrap stylesheets was loaded. So I did not see that removing some CSS rules was implying side effects when bootstrap was not used. I also fixed some other issues spotted with your examples.

The complete diff of this update can be found below:

diff --git a/src/highlightjs-line-numbers.js b/src/highlightjs-line-numbers.js
index 670bc6e..1aec06a 100644
--- a/src/highlightjs-line-numbers.js
+++ b/src/highlightjs-line-numbers.js
@@ -27,13 +27,15 @@
         var css = d.createElement('style');
         css.type = 'text/css';
         css.innerHTML = format(
-            '.{0} table{float:left}' +
+            '.{0} table{float:left; border-collapse:collapse}' +
             '.{0} table td{padding:0}' +
-            '.{1}:before{content:attr({2})}',
+            '.{1}::before{content:attr({2})}' +
+            '.{3}::before{content: "\\200B"}', // force display of empty lines of code
         [
             TABLE_NAME,
             NUMBER_LINE_NAME,
-            DATA_ATTR_NAME
+            DATA_ATTR_NAME,
+            CODE_BLOCK_NAME
         ]);
         d.getElementsByTagName('head')[0].appendChild(css);
     }
@@ -64,14 +66,16 @@

     function lineNumbersBlock (element, options) {
         if (typeof element !== 'object') return;
-
         async(function () {
             element.innerHTML = lineNumbersInternal(element, options);
             // adjust left margin of code div as line numbers is a float left dom element
-            var codeMargin = element.querySelector('.' + NUMBERS_CONTAINER_NAME).offsetWidth;
-            var codeContainerStyle = 'margin-left:' + codeMargin + 'px';
-            var codeContainer = element.querySelector('.' + CODE_CONTAINER_NAME);
-            codeContainer.style.cssText = codeContainerStyle;
+            var lineNumbersContainer = element.querySelector('.' + NUMBERS_CONTAINER_NAME);
+            if (lineNumbersContainer) {
+              var codeMargin = lineNumbersContainer.offsetWidth;
+              var codeContainerStyle = 'margin-left:' + codeMargin + 'px';
+              var codeContainer = element.querySelector('.' + CODE_CONTAINER_NAME);
+              codeContainer.style.cssText = codeContainerStyle;
+            }
         });
     }

Nevertheless, there is still an issue in responsive mode when the document width is small (with mobile devices for instance). The height of a line number element can end up not synchronized with the one from the wrapped line of code (see images below, first one corresponds to my implementation, second one current release implementation). So this PR still needs some work.

[anlambert]

I have just pushed an updated version of that PR fixing the responsive design issue detailed in my previous comment. Corresponding diff compared to last update is the following:

diff --git a/src/highlightjs-line-numbers.js b/src/highlightjs-line-numbers.js
index 1aec06a..dfdff76 100644
--- a/src/highlightjs-line-numbers.js
+++ b/src/highlightjs-line-numbers.js
@@ -13,6 +13,8 @@
         DATA_ATTR_NAME = 'data-line-number',
         BREAK_LINE_REGEXP = /\r\n|\r|\n/g;

+    var resizeHandlerSet = false;
+
     if (w.hljs) {
         w.hljs.initLineNumbersOnLoad = initLineNumbersOnLoad;
         w.hljs.lineNumbersBlock = lineNumbersBlock;
@@ -64,6 +66,15 @@
         }
     }

+    function adjustLineNumbersHeights(element) {
+      var lnNumbers = element.querySelectorAll('.' + NUMBERS_BLOCK_NAME);
+      var lnCode = element.querySelectorAll('.' + CODE_BLOCK_NAME);
+
+      for (var i = 0 ; i < lnNumbers.length; ++i) {
+        lnNumbers[i].style.height = lnCode[i].offsetHeight + 'px';
+      }
+    }
+
     function lineNumbersBlock (element, options) {
         if (typeof element !== 'object') return;
         async(function () {
@@ -75,6 +86,20 @@
               var codeContainerStyle = 'margin-left:' + codeMargin + 'px';
               var codeContainer = element.querySelector('.' + CODE_CONTAINER_NAME);
               codeContainer.style.cssText = codeContainerStyle;
+
+              // adjust each line number cell height to the one of the div containing
+              // the wrapped line of code and set a handler to execute this
+              // operation when the browser window gets resized.
+              adjustLineNumbersHeights(element);
+              if (!resizeHandlerSet) {
+                window.addEventListener('resize', function() {
+                  var hljsLnElts = document.querySelectorAll('.' + TABLE_NAME);
+                  for (var i = 0 ; i < hljsLnElts.length; ++i) {
+                    adjustLineNumbersHeights(hljsLnElts[i]);
+                  }
+                });
+                resizeHandlerSet = true;
+              }
             }
         });
     }

Let me know if you find other issues.

[anlambert]

@wcoder , friendly ping to merge this one. I have just updated that PR to match your code formatting style (4 spaces indentation). I do not observe any particular regression after my last updates. If you have any doubts on my changes, you can test them live here: https://archive.softwareheritage.org/ . This is simply the largest public source code archive available so far, so you have some material here ;-)

[wcoder]

Hi @anlambert, I'm very appreciated for your help!

[anlambert]

Hi @wcoder, I think you did not get right my last updates (surely because I pushed force my branch after amending commits). As you can see on that screenshot of my firefox, line heights are correct:

I attach your debug samples (hljsln_debug_samples.zip incuding the modified version of highlightjs-line-numbers, do you still observe the above issue ?

[wcoder]

@anlambert the same issue on hljsln_debug_samples.

macOS - Firefox 65.0.1
Windows - Microsoft Edge 44.17763.1.0

Works correctly only for Firefox 65.0.2 on Windows

[anlambert]

@wcoder , I managed to reproduce your issue simply by playing with the zoom level for the current browsed page (I am working on Debian by the way). You were right, the cause of the issue was the line-height CSS property not being consistent between DOM elements rendering numbers and lines of code. To fix that, I added the following CSS rules in updated js code:

.hljs-ln-code,
.hljs-ln-numbers {
      line-height: 16px;
      line-height: 1rem;
}

Contemporary browsers will use the line-height: 1rem; property while (really) older ones will use line-height: 16px; as a fallback.

I also noticed a small issue by playing with your debug examples regarding the synchronization of line heights between numbers and code after resizing/zooming. In order for this process to work properly, it must be done once the lines of code have been rendered. So to avoid computation issues, I made this extra processing step asynchronous.

Corresponding diff compared to my last update is the following:

diff --git a/src/highlightjs-line-numbers.js b/src/highlightjs-line-numbers.js
index a200c60..3099d5e 100644
--- a/src/highlightjs-line-numbers.js
+++ b/src/highlightjs-line-numbers.js
@@ -29,15 +29,19 @@
         var css = d.createElement('style');
         css.type = 'text/css';
         css.innerHTML = format(
-            '.{0} table{float:left; border-collapse:collapse}' +
-            '.{0} table td{padding:0}' +
-            '.{1}::before{content:attr({2})}' +
-            '.{3}::before{content: "\\200B"}', // force display of empty lines of code
+            '.{0} table {float: left; border-collapse: collapse}' +
+            '.{0} table td {padding: 0}' +
+            '.{1}::before {content: attr({2})}' +
+            // force display of empty lines of code
+            '.{3}::before {content: "\\200B"}' +
+            // ensure consistent line heights in dom elements for numbers and code
+            '.{3},.{4} {line-height: 16px; line-height: 1rem}',
         [
             TABLE_NAME,
             NUMBER_LINE_NAME,
             DATA_ATTR_NAME,
-            CODE_BLOCK_NAME
+            CODE_BLOCK_NAME,
+            NUMBERS_BLOCK_NAME
         ]);
         d.getElementsByTagName('head')[0].appendChild(css);
     }
@@ -90,13 +94,16 @@
                 // adjust each line number cell height to the one of the div containing
                 // the wrapped line of code and set a handler to execute this
                 // operation when the browser window gets resized.
-                adjustLineNumbersHeights(element);
+                // execute this operation asynchronously once the code has beed rendered
+                async(adjustLineNumbersHeights(element));
                 if (!resizeHandlerSet) {
                     window.addEventListener('resize', function() {
-                        var hljsLnElts = document.querySelectorAll('.' + TABLE_NAME);
-                        for (var i = 0 ; i < hljsLnElts.length; ++i) {
-                            adjustLineNumbersHeights(hljsLnElts[i]);
-                        }
+                        async(function() {
+                            var hljsLnElts = document.querySelectorAll('.' + TABLE_NAME);
+                            for (var i = 0 ; i < hljsLnElts.length; ++i) {
+                                adjustLineNumbersHeights(hljsLnElts[i]);
+                            }
+                        });
                     });
                     resizeHandlerSet = true;
                 }

Everything seems to work as fine as before my changes now.

[anlambert]

@wcoder, as pointed by https://github.com/jkcclemens/paste/issues/108#issuecomment-471328864, there might be a simpler fix to that issue by processing the copied text through the Clipboard API.

However, this API is still in draft state by the W3C. Recent browsers support it while older ones will surely miss support for it. From my point of view, relying on the DOM is better to ensure wider compatibility across all browsers in the wild.

What is your opinion on this ?

[anlambert]

@wcoder , I could not resist to test the Clipboard API approach ;-) I ended up with a much better solution than the current one IMHO. Turns out that browsers support greatly this API since at least a decade (with some slight variations though, I am looking at you MS Edge).

I have created #62 showcasing the results. From my point of view, this first PR should be closed as the solution is quite too complex and there is still some rare side effects that show up when resizing the browser window or playing with the zoom level.

What do you think ?

[wcoder]

Hi @anlambert, thanks, very appreciate you for the contribution 🎉

I decided to merge your PR with the Clipboard API approach (it is future and Edge will be migrated to Chromium). Also, It will more painless.

I'm going to close this PR, maybe we will come back to it later.