Pull Request UI code diff: A code row is split into many useless spans each with mouseover handler attached (OD-1329)

jbauer opened 3 years ago

I just noticed in a pull request that some letters in a java literal string were rendered as bold text. I checked the HTML source and it seems as if OneDev thinks that java literal strings contain code because a lot of span elements have been created with mouseover event handler attached (which would show occurrences popup). One of the span elements had css class for keywords assigned which is why some letters were rendered bold. The letters in question were est.

For example if you have something like

output += "This is some error message for the UI containing useful information";

OneDev generates HTML like (just an example):

<td class="content right new noselect" data-new="181">
  <span class=""></span>
  <!-- every span below has event handler attached to open popup for code occurrences -->
  <span class="cm-variable">ou</span> 
  <span class="cm-variable cm-operator cm-variable">tp</span>
  <span class="cm-variable cm-variable">u</span>
  <span class="cm-variable cm-variable cm-variable">t</span>
  <span class="cm-variable cm-variable"> </span> 
  <span class="cm-operator cm-variable cm-variable">+=</span>
  <span class="cm-variable cm-variable"> </span>
  <span class="cm-string cm-variable cm-variable">"Thi</span>
  <span class="cm-string cm-variable">s i</span>
  <span class="cm-string">s</span>
  <span class="cm-string cm-variable cm-operator"> some</span>
  <span class="cm-string cm-variable"> </span>
  <span class="cm-string cm-variable cm-atom">error messag</span>
  <span class="cm-string cm-variable">e for the </span>
  <span class="cm-string">UI containing useful information"</span>
</td>

However I would more likely expect something like

<td class="content right new noselect" data-new="181">
  <span class=""></span>
   <!-- only this span should have event handler attached to open popup for code occurrences -->
  <span class="cm-variable">output</span>
  <span class=""> </span>
  <span class="cm-operator">+=</span>
  <span class=""> </span>
  <!-- maybe this could also have an event handler attached to show occurrences of the complete string in code base -->
  <span class="cm-string">"This is some error message for the UI containing useful information"</span>
</td>

Activities

jbauer changed title 3 years ago

Previous Value	Current Value
Pull Request UI code diff: Java literal strings are treated as code resulting in many spans with mouseover handler being created	Pull Request UI code diff: A code row is split into many useless spans each with mouseover handler attached

Previous Value

Current Value

Pull Request UI code diff: Java literal strings are treated as code resulting in many spans with mouseover handler being created

Pull Request UI code diff: A code row is split into many useless spans each with mouseover handler attached

jbauer commented 3 years ago

Updated the title because not just string literals are affected. Also code expressions are affected, e.g. variable names or type names. For example I have seen boolean being split into multiple spans with olean being rendered bold because it is marked as keyword. Or variable names being split and having spans with cm-string which renders some letters blue.

I feel like some kind of text index is broken?
Robin Shen commented 3 years ago

Again can you demonstrate this issue with an example repo? Tried to reproduce but it works fine at my side.
jbauer commented 3 years ago

No. However we did nothing special. Just normal PR workflows. Someone has updated the pull request in my installation and now everything seems to look okay again.

No idea if that might be the cause but the pull request exists in 8.0.10 and I noticed the issue a few days after upgrading to 8.0.15 while reviewing the PR. However now it is gone in 8.0.15 after PR has been updated again. From a language perspective the PR contains a mixture of English and German. Other "old" PRs do not have that issue.

However in the PR in question I now notice that I have a <span class="cm-string">long error message in german</span> with a mouseover handler attached and when I hover that line the popup shows CSS rules as possible declarations:

The above content originates from a minified css file (1 long line of css classes + rules). A little weird that OneDev's full text index somehow thinks this might be related.
Robin Shen commented 3 years ago

Hmm... This looks odd. For this css rule matching issue, is it reproducible with a demo repo?
Robin Shen changed state to 'Closed' 3 years ago
Previous Value Current Value
Open

Closed
Robin Shen commented 3 years ago

This should have been fixed in 8.1.0 now
jbauer commented 3 years ago

Oh really? And what was the issue?
Robin Shen commented 3 years ago

This fix as part of addressing issue #1333:

https://code.onedev.io/onedev/server/~commits/79e40ca059e4a7184f8b2ce2ec46516bdaf388f9?path-filter=server-core/src/main/java/io/onedev/server/web/asset/codemirror/lib/codemirror.js&mark=79e40ca059e4a7184f8b2ce2ec46516bdaf388f9:server-core/src/main/java/io/onedev/server/web/asset/codemirror/lib/codemirror.js:874.1-875.52-1

Please upgrade to 8.1.0 to see if the issue still exist.
jbauer commented 3 years ago

@robin Just updated and will keep an eye on the span issue which I could not reproduce since someone updated the pull request.

However the popup with the weird possible declarations still exist in 8.1.0. Just now I noticed that .n seem to look bold so I guess that is what has matched? The string literal in the pull request that causes the above popup actually contains "this is some text.\n\n". So I guess it matches the .\n in java string literal with the .n css class in a css file.
Robin Shen commented 3 years ago

This is a bug and will be fixed in next patch release.
Login to comment

Previous Value	Current Value
Open	Closed

Type	Bug
Priority	Normal
Assignee	Robin Shen
Affected Versions	8.0.15

Issue Votes (0)

Watchers (4)

Reference

OD-1329