Fossil Forum

Web UI: truncated diffs II
Login

Web UI: truncated diffs II

Web UI: truncated diffs II

(1) By Florian Balmer (florian.balmer) on 2022-09-23 15:13:39 [link] [source]

I've noticed another minor web UI glitch:

The side-by-side diffs are truncated at the right, along with the scrollbar.

A possible simple fix seems to be to add the box-sizing: border-box style to the elements containing the diff text, as accomplished by the patch below.

The following screenshots demonstrate the bug (top row) and the fix (bottom row) with FF and Chromium:

https://i.imgur.com/eUvo28q.png

So far, I've only tested the fix on Windows desktops at 100% DPI with various page zoom levels, so not sure if it also works for *nix/mobile systems, other browsers, non-standard DPI settings, etc.

Since the fix is quite easy to apply through the browser Developer Tools (right-click the diff text, select "Inspect", and append box-sizing: border-box; to the style attribute of the <pre> element) -- could people who also see the bug on systems and browsers different from mine please take a quick check of the fix and post feedback here whether it works, so this could be weaved into a proper commit?

Thank you!

Index: src/default.css
==================================================================
--- src/default.css
+++ src/default.css
@@ -584,12 +584,13 @@
 table.diff td > pre {
   /* Workaround for "slight wiggle" when using mouse-wheel in some FF
      versions, apparently caused by the increased line-height forcing
      these elements to be a *tick* larger than they should be but not
      large enough to force a scroll bar to show up. */
   overflow-y: hidden;
+  box-sizing: border-box;
 }
 tr.diffskip.jchunk {
   /* jchunk gets added from JS to diffskip rows when they are
      plugged into the /jchunk route. */
   background-color: aliceblue;
   padding: 0;

(2) By Stephan Beal (stephan) on 2022-09-23 15:19:10 in reply to 1 [link] [source]

so this could be weaved into a proper commit?

Please do :). i've seen this numerous times in Firefox on Linux, both on "low resolution" screens (1080p) and 4k, but it's never bothered me enough to experiment with a fix.

(3) By Florian Balmer (florian.balmer) on 2022-09-23 15:31:28 in reply to 2 [link] [source]

Thanks for the quick feedback!

I think both your and my systems already cover quite a reasonable range. Because I'm currently online only sporadically, this commit may take a few days -- but maybe we also get feedback from a Mac user in the meantime (but I wouldn't expect Safari to be too different from Chromium, in this case).

(4) By Florian Balmer (florian.balmer) on 2022-09-25 07:49:04 in reply to 1 [link] [source]

After some more tests with different skins I was confident enough to commit the solution.

(5) By Daniel Dumitriu (danield) on 2023-01-05 09:34:43 in reply to 1 [source]

Ressurecting an old thread, since the subject is the same. This time:

For one-column diffs there is no "scroller", so on mobile/small displays the diffs hopelessly disappear on the right side limbo. I refrain to test my hand at it lest I break something else.

(6) By mark on 2023-01-05 09:46:59 in reply to 5 [link] [source]

so on mobile/small displays the diffs hopelessly disappear on the right side limbo

I've noticed this for quite some time now when browsing on my iPhone but kept forgetting to report it after mentioning it in either libf or fsl /chat when I first noticed. It makes viewing diffs on the phone pretty much impossible.

(7.1) By Florian Balmer (florian.balmer) on 2023-01-10 16:06:58 edited from 7.0 in reply to 6 [link] [source]

I have a draft patch to add vertical horizontal scrolling to the unified diff text blocks. It's a quick patch to omit proper indentation of the last block in favor of a simple diff to copy-paste and read in the Forum.

Disclaimers:

  • Not tested on real mobile devices (just in the browser simulator).
  • It's somewhat down the guts of stephan's JS and CSS, so I'd like stephan to decide whether, and maybe also how to do this.
  • A "less hacky" solution may require quite some refactoring (I'm ready to help, but somewhat lacking ideas...).

My Fossil time is currently a bit on-and-off, but I think so is stephans...

Index: src/default.css
==================================================================
--- src/default.css
+++ src/default.css
@@ -575,14 +575,10 @@
   padding: inherit;
 }
 table.diff td.diffln > pre {
   padding: 0 0.35em 0 0.5em;
 }
-table.diff td.difftxt > pre {
-  min-width: 100%;
-  max-width: 100%;
-}
 table.diff td > pre {
   box-sizing: border-box;
   /* Workaround for "slight wiggle" when using mouse-wheel in some FF
      versions, apparently caused by the increased line-height forcing
      these elements to be a *tick* larger than they should be but not

Index: src/fossil.diff.js
==================================================================
--- src/fossil.diff.js
+++ src/fossil.diff.js
@@ -666,10 +666,17 @@
     }
     f.colsR.forEach(function(e){
       e.style.width = w + "px";
       e.style.maxWidth = w + "px";
     });
+    if(force || !f.colsU){
+      f.colsU = document.querySelectorAll('td.difftxtu pre');
+    }
+    f.colsU.forEach(function(e){
+      e.style.width = (w*2 + 100) + "px";
+      e.style.maxWidth = (w*2 + 100) + "px";
+    });
     if(0){ // seems to be unnecessary
       if(!f.allDiffs){
         f.allDiffs = document.querySelectorAll('table.diff');
       }
       w = lastWidth;
@@ -691,15 +698,19 @@
     const table = this.parentElement/*TD*/.parentElement/*TR*/.
       parentElement/*TBODY*/.parentElement/*TABLE*/;
     table.$txtPres.forEach((e)=>(e===this) ? 1 : (e.scrollLeft = this.scrollLeft));
     return false;
   };
-  Diff.initTableDiff = function f(diff){
+  Diff.initTableDiff = function f(diff, unified){
     if(!diff){
       let i, diffs = document.querySelectorAll('table.splitdiff');
       for(i=0; i<diffs.length; ++i){
-        f.call(this, diffs[i]);
+        f.call(this, diffs[i], false);
+      }
+      diffs = document.querySelectorAll('table.udiff');
+      for(i=0; i<diffs.length; ++i){
+        f.call(this, diffs[i], true);
       }
       return this;
     }
     diff.$txtCols = diff.querySelectorAll('td.difftxt');
     diff.$txtPres = diff.querySelectorAll('td.difftxt pre');
@@ -710,15 +721,16 @@
     //console.debug("diff.$txtPres =",diff.$txtPres);
     diff.$txtCols.forEach((e)=>e.style.width = width + 'px');
     diff.$txtPres.forEach(function(e){
       e.style.maxWidth = width + 'px';
       e.style.width = width + 'px';
-      if(!e.classList.contains('scroller')){
+      if(!unified && !e.classList.contains('scroller')){
         D.addClass(e, 'scroller');
         e.addEventListener('scroll', scrollLeft, false);
       }
     });
+    if(!unified){
     diff.tabIndex = 0;
     if(!diff.classList.contains('scroller')){
       D.addClass(diff, 'scroller');
       diff.addEventListener('keydown', function(e){
         e = e || event;
@@ -729,10 +741,11 @@
            but txtPres[0] does not, no scrolling happens here. We need
            to find the widest of txtPres and scroll that one. Example:
            Checkin a7fbefee38a1c522 file diff.c */
         return false;
       }, false);
+    }
     }
     return this;
   }
   window.fossil.page.tweakSbsDiffs = function(){
     document.querySelectorAll('table.splitdiff').forEach((e)=>Diff.initTableDiff(e));

Edit: vertical → horizontal

(8) By Stephan Beal (stephan) on 2023-01-10 17:16:02 in reply to 7.1 [link] [source]

My Fossil time is currently a bit on-and-off, but I think so is stephans...

i just moved to temporary housing in Berlin (from Bavaria) yesterday and will alternate between offline and online at unpredictable intervals for the near-term future. My huge, lovely monitor is in storage so i'll be working from a tiny laptop screen, which will definitely reduce my desire to be on the computer ;).

FWIW, the patch looks just fine to me and this isn't a task which needs lots of refinement or perfection. If you're happy with it, let's check it in.

(9) By Florian Balmer (florian.balmer) on 2023-01-11 08:09:19 in reply to 8 [link] [source]

Ok, thanks for the "go"! The patch is now on a branch for easier review and testing. I'll do more tests with /fileedit and /wikiedit, and on Firefox, at the next opportunity.

Should horizontal scrolling also be synchronized for all chunks of the same file in unified diff view? This is a nice feature in side-by-side diff view, but somehow I think not necessary (or even confusing?) in unified diff view?

(10) By Daniel Dumitriu (danield) on 2023-01-11 08:41:16 in reply to 9 [link] [source]

I've just tested your branch on FF and Chrome, both on Windows (by shrinking the window) and, more interestingly, on Android (compiling under Termux), and it works as expected, so from my side it's thumbs-up for merge. Thanks!

I think it's better not to sync scrolling the hunks, but I won't suffer if we decide otherwise.

(11) By Stephan Beal (stephan) on 2023-01-11 10:20:15 in reply to 10 [link] [source]

I think it's better not to sync scrolling the hunks, but I won't suffer if we decide otherwise.

FWIW, 100% agreed. We can add it later if it's desired (we do it in side-by-side diffs, so have the code already).

(12) By Florian Balmer (florian.balmer) on 2023-01-11 18:11:13 in reply to 11 [link] [source]

Thanks for the additional testing and feedback! Now merged to trunk.