Refined-github: Misaligned diff view with tabs

[fjeremic]

When mixing tabs and spaces within a GitHub diff view Refined GitHub seems to screw up the indentation to make it look like there is a formatting issue when there is actually not. The issue can be seen in [1] and the raw diff in [2] where we can see there is no formatting issue. I've attached screenshots with Refined GitHub enabled and disabled.

[1] https://github.com/eclipse/omr/commit/75c09985f5d5dd6cfd7c9d1e4f4d56e9406a45c4#diff-3990e65f3237ed46d278a1a8aefbaeeaR389
[2] https://raw.githubusercontent.com/eclipse/omr/75c09985f5d5dd6cfd7c9d1e4f4d56e9406a45c4/port/common/j9nls.c

Refined GitHub enabled

Refined GitHub disabled

[fregante]

cc @notlmn

From what I'm seeing, the count and size is correct, but text-indent isn't behaving the way we want; it looks like it's still trying to follow the tabulation.

On that line, after the feature has run, try changing text-indent to -61px... and then to -62px. It will jump more than 1px:

[notlmn]

I'm not seeing this. Not in commit, review, different diff views in PR files. It's all fine for me everywhere.

---

On that line, after the feature has run, try changing text-indent to -61px... and then to -62px.

For me the text-indent value is -59.3789px on a Windows machine running in Chrome 74.0.3729.169 with Consolas at 12px;

I'm not sure why you'd want to change it after the feature has run. And it looks fine in your first screenshot too (with white-spaces enabled).

---

@fjeremic Which version of RGH are you on. I'm testing https://github.com/sindresorhus/refined-github/tree/19.5.27.1925 manually loading the extension because for some reason RGH on Chrome Web Store is locked to 19.5.22.1921 for me.

[fregante]

I'm not sure why you'd want to change it after the feature has run

To show what’s happening

[notlmn]

I'm not sure why you'd want to change it after the feature has run

To show what’s happening

Sorry if I sounded over cautious, what I meant was that text-indent wouldn't change once it is set by the feature.

[fregante]

text-indent wouldn't change once it is set by the feature.

Nobody is saying that; the gif is only to show that the tab is interacting with the text-indent and causing this issue, probably depending on the font/OS. I'm testing on master on macOS

[lode]

I'm seeing the same. A diff in a docblock shows misaligned whitespace, even if the whitespace didn't change.

Also, don't know if it is related, but lines which extend beyond the viewport of the diff are wrapped.

(I'm sorry I don't have a public PR to make a screenshot of.)

Happens to me on FF Ubuntu.

[notlmn]

I managed to isolate this issue to what might be a possible text rendering bug in Firefox.

The second line in the comment has a similar padding-left and text-indent values, that should cancel each other out and the line shouldn't move.

| Firefox | Chrome |
|:-:|:-:|
| | |

The mixed white-spaces on the second line is probably making Firefox go crazy. I might be wrong but one would expect for this to work as intended, where the tab characters don't collapse in mixed indentation.

---

Isolated bin: https://jsbin.com/jelomov/edit?html,css,output.

In the bin above, all the * characters should be aligned.

| Firefox | Chrome |
|:-:|:-:|
| | |

[notlmn]

I'm seeing the same. A diff in a docblock shows misaligned whitespace, even if the whitespace didn't change.

Also, don't know if it is related, but lines which extend beyond the viewport of the diff are wrapped.

@lode The misaligned white-space is a real issue. But code beyond viewport _is_ expected to wrap onto the next line, that's intended.

[fregante]

Thanks for looking into this! 🤗

[notlmn]

One possible quick fix I could find without disabling this feature and not waiting for the browser to fix (if this were a browser issue) is surprisingly #2073, where adding display: inline-block to .pl-ws in the PR _should_ fix this, forcing browser to not collapse tabs inside mixed indentation.

[lode]

_{But code beyond viewport _is_ expected to wrap onto the next line, that's intended.}

_{Thanks, found indented-code-wrapping. I find the previous situation more readable, but I can just disable the feature.}

[fregante]

If changing the dom is unavoidable, I'd wrap the whitespace in a span and make it .sr-only (invisible but copy-pastable), so we don't need the text-indent but it's more of a last resort.

[fjeremic]

@fjeremic Which version of RGH are you on. I'm testing 19.5.27.1925

I'm on Firefox 66.0.5 running RGH 19.5.27. I checked for updates and this is the latest version available. Thanks everyone for looking into this by the way. RGH is by far my most used add-on.

[notlmn]

Confirmed as Firefox bug on Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1555903#a136463_475400.

(Up vote on Bugzilla for possibly quick fix)

[jerone]

If I change the following inline css for the example it works for me:

-padding-left: calc((var(--tab-size, 4) * 2ch) + 1ch) !important;
+padding-left: calc((var(--tab-size, 4) * 2ch) + 0ch) !important;
-text-indent: calc((var(--tab-size, 4) * -2ch) - 1ch) !important;
+text-indent: calc((var(--tab-size, 4) * -2ch) - 0ch) !important;

[fregante]

That does seem to work, perhaps this needs to be tested further, with more indentation types, e.g.

 -> hello
 -> •space
 -> ••spaces

To make things easier, it only has to work correctly if the spaces follow the tab, not the other way around.

[notlmn]

@jerone that's specific for this example, what you did there is similar to ignoring space character count and only counting tab characters (the bug with Firefox). It was already thought of by me before opening #2088, but it failed for other cases.

[fregante]

Can that fix be applied only where it works?

e.g. if space == 1 & lastIndent == space & isFirefox, then spaces = 0

[jerone]

Are there more test-cases?

[notlmn]

Can that fix be applied only where it works?

e.g. if space == 1 & lastIndent == space & isFirefox, then spaces = 0

The space count be anything, not just 1, as long as it is under the value of --tab-size. These additional spaces can be substituted for with an additional tab, this wouldn't affect the overall alignment as long as the specified condition is met.

The advantage with this is that it works in both Chrome and Firefox, so some part of the code can be removed.

But the downside to this is that wrapped text would not be at the same level of the actual line, it would be where the last tab character ended. And the major downside is that it only works for code indented with tab, any code indented with completely with spaces end up depending on --tab-size, which kind of defeats the purpose of space-indented code (spaces are fixed in length, --tab-size affects this length).

---

Are there more test-cases?

Yes, like code that is completely indented with spaces where there are no tabs (explained above).

[sindresorhus]

I can reproduce this in Chrome too: https://github.com/sindresorhus/electron-context-menu/pull/76#discussion_r289699052

I think we should force-disable this feature until we can fix it properly.

[notlmn]

I can reproduce this in Chrome too: sindresorhus/electron-context-menu#76 (comment)

That's because GitHub adds some extra negative text indentation for inline wrapped code. Originally spotted in https://github.com/sindresorhus/refined-github/pull/1989#issuecomment-493647728, but was ignored in between other issues. 😅

---

Unfortunately this ended up being a completely different bug in Chrome rather than in Firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=1555903#c8).

Will have to try a different solution for this based on https://github.com/sindresorhus/refined-github/issues/2085#issuecomment-498032629, but until then this feature remains in a limbo as we have to wait for https://bugs.chromium.org/p/chromium/issues/detail?id=969616 to be solved. If there were to be a fix (considering Firefox does it right), it has to take Chrome's different layout calculation into consideration as a temporary fix.

Will try to see what I can whip up, and how it behaves in different browsers (event if I have to write a bit of code to calculate tab-stops and work from there).