Skip to content

inlineToStr is not exhaustive and does not remove html tags inside HtmlTag [ci: last-only]#5728

Merged
lrytz merged 1 commit intoscala:2.12.xfrom
Philippus:issue/html-tag-in-hover
Mar 2, 2017
Merged

inlineToStr is not exhaustive and does not remove html tags inside HtmlTag [ci: last-only]#5728
lrytz merged 1 commit intoscala:2.12.xfrom
Philippus:issue/html-tag-in-hover

Conversation

@Philippus
Copy link
Member

@Philippus Philippus commented Feb 22, 2017 •
edited
Loading

as can be seen on f.e. this page:
http://www.scala-lang.org/files/archive/api/current/scala/beans/BeanInfoSkip.html
when hovering over "BeanInfoSkip" on the right the title tag contains some unwanted text.
HtmlTag(...) and the <strong>, </strong> should not be inside the title-tag.
This PR should fix that problem.

  • added missing Inline matches to inlineToStr so it is now exhaustive
  • scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag

@scala-jenkins scala-jenkins added this to the 2.12.2 milestone Feb 22, 2017
@SethTisue
Copy link
Member

SethTisue commented Feb 22, 2017

would it be possible (and not unreasonably hard) to add test coverage...?

@lrytz
Copy link
Member

lrytz commented Feb 23, 2017

I quickly looked at existing scaladoc tests, they test on the model. The PR here doesn't change the model. Maybe @VladUreche can give a hint if it's easy to test this fix?

lrytz
lrytz reviewed Feb 23, 2017
View reviewed changes
src/scaladoc/scala/tools/nsc/doc/html/Page.scala Outdated
case Monospace(in) => inlineToStr(in)
case Text(text) => text
case Summary(in) => inlineToStr(in)
case HtmlTag(tag) => scala.xml.XML.loadString(tag).text
Copy link
Member

@lrytz lrytz Feb 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tag data could be invalid xml, no? For example:

import scala.tools.nsc.doc.model._
import scala.tools.partest.ScaladocModelTest

object Test extends ScaladocModelTest {
  override def code = """
  /** Hi<br>there. */
  class A
  """

  def scaladocSettings = ""

  def testModel(root: Package) = {
    import access._
    root._class("A").comment foreach println
  }
}

prints

Body(List(Paragraph(Chain(List(Summary(Chain(List(Chain(List(Text(Hi), HtmlTag(<br>), Text(there))), Text(.)))))))))

and

scala> scala.xml.XML.loadString("<br>")
org.xml.sax.SAXParseException: XML document structures must start and end within the same entity.

Copy link
Member Author

@Philippus Philippus Feb 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I will look into it a little later. I'd prefer to not use a regex, but maybe that's the way to go.

@Philippus
Copy link
Member Author

Philippus commented Feb 23, 2017

I would like to add test cases but it will be a little annoying to do so as the method is inside a larger structure.
The initial tests for the PR where the method inlineToStr is introduced are in /scala/test/scaladoc/run/shortDescription-annotation.scalaand those tests repeat the inlineToStr-method inside the test.

@adriaanm adriaanm changed the title inlineToStr is not exhaustive and does not remove html tags inside HtmlTag inlineToStr is not exhaustive and does not remove html tags inside HtmlTag [ci: last-only] Feb 23, 2017
@lrytz
Copy link
Member

lrytz commented Feb 24, 2017

/rebuild

@lrytz
Copy link
Member

lrytz commented Feb 24, 2017

/synch

@SethTisue
Copy link
Member

SethTisue commented Feb 24, 2017

(in my experience, retroactive application of [ci: last-only] doesn't work as a way to make the "combined" red X disappear, you just have to /nothingtoseehere it)

@lrytz
Copy link
Member

lrytz commented Feb 24, 2017

ok, that "/synch" didn't help, the "combined" status is still red. anyway, it would be a good idea to squash all these commits into one and push -f.

lrytz
lrytz requested changes Feb 24, 2017
View reviewed changes
test/scaladoc/run/inlineToStr-strips-unwanted-text.scala Outdated
case Summary(in) => inlineToStr(in)
case HtmlTag(tag) => "<[^>]*>".r.replaceAllIn(tag, "")
case EntityLink(Text(text), _) => text
}
Copy link
Member

@lrytz lrytz Feb 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-pasting is not ideal, obviously.. Given that this function doesn't depend on the class Page in which it is defined, how about pulling it out into the companion object? I don't see a problem with that function being public (vs. protected).

Copy link
Member Author

@Philippus Philippus Feb 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok!

lrytz
lrytz reviewed Feb 24, 2017
View reviewed changes
test/scaladoc/run/inlineToStr-strips-unwanted-text.scala
/** This comment contains a link [[https://scala.epfl.ch/]] */
def baz = ???

/** This comment contains an <strong>html tag</strong> */
Copy link
Member

@lrytz lrytz Feb 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could alo add a test with nested html tags

Copy link
Member Author

@Philippus Philippus Feb 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok!

@Philippus
Copy link
Member Author

Philippus commented Feb 25, 2017

after the tests complete, I will try squashing the commits.

@Philippus
added missing Inline matches to inlineToStr so it is now exhaustive
e3c5df8 
scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag

use a regex to remove html tags inside the tag

added some tests for the inlineToStr-method

moved inlineToStr to companion object of Page

added test for nested html tags
@Philippus Philippus force-pushed the issue/html-tag-in-hover branch from 438ae26 to e3c5df8 Compare February 25, 2017 13:45
@Philippus
Copy link
Member Author

Philippus commented Mar 2, 2017

@lrytz when you have time, could you take another look?

@lrytz
Copy link
Member

lrytz commented Mar 2, 2017

Thanks a lot!

@lrytz lrytz merged commit f2e05c2 into scala:2.12.x Mar 2, 2017
@Philippus Philippus deleted the issue/html-tag-in-hover branch March 2, 2017 10:04
@SethTisue
Copy link
Member

SethTisue commented Mar 3, 2017

seen in the Scala community build at https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/1301/consoleFull:

[scalatest] scala.MatchError: scala.tools.nsc.doc.base.MemberLookupBase$$anon$1@1ee38638 (of class scala.tools.nsc.doc.base.MemberLookupBase$$anon$1)
[scalatest] 	at scala.tools.nsc.doc.html.Page$.inlineToStr(Page.scala:124)
[scalatest] 	at scala.tools.nsc.doc.html.Page$.$anonfun$inlineToStr$1(Page.scala:113)
[scalatest] 	at scala.tools.nsc.doc.html.Page$.$anonfun$inlineToStr$1$adapted(Page.scala:113)
[scalatest] 	at scala.collection.TraversableLike.$anonfun$flatMap$1(TraversableLike.scala:241)
[scalatest] 	at scala.collection.immutable.List.foreach(List.scala:390)
[scalatest] 	at scala.collection.TraversableLike.flatMap(TraversableLike.scala:241)
[scalatest] 	at scala.collection.TraversableLike.flatMap$(TraversableLike.scala:238)
[scalatest] 	at scala.collection.immutable.List.flatMap(List.scala:353)

looks like a regression as a result of this PR?

@lrytz
Copy link
Member

lrytz commented Mar 3, 2017

Indeed - the pattern here is too narrow: https://github.com/scala/scala/pull/5728/files#diff-53f51aedd9537e3ba6f849e248398165R124, should probably be

    case EntityLink(title, _) => inlineToStr(title)

@Philippus do you have time to take a look?

@Philippus
Copy link
Member Author

Philippus commented Mar 3, 2017

yes I have time. I've managed to reproduce it with
/** This comment contains a [[link ,,with a subscript title,,]] */
shall I commit a fix (and test) to this branch?

@SethTisue
Copy link
Member

SethTisue commented Mar 3, 2017

shall I commit a fix (and test)

yes please

to this branch?

hmm... well, we'll need a new PR. I guess you could reuse the same branch? I don't think it matters

you're in good company, the community build has been catching a lot of regressions lately.

@Philippus
Copy link
Member Author

Philippus commented Mar 3, 2017

ok, I'll make a new PR.

@Philippus Philippus mentioned this pull request Mar 3, 2017
Merged
SethTisue added a commit to SethTisue/scala that referenced this pull request Mar 6, 2017
@SethTisue
Merge remote-tracking branch 'origin/2.12.x' into 2.13.x
5095b1e 
only trivial merge conflict involving a method renaming

*   d1d700e - (origin/HEAD, origin/2.12.x) Merge pull request scala#5754 from Philippus/issue/html-tag-in-hover (2 days ago) <Lukas Rytz>
|\
| * cf64718 - pattern for entitylink was too narrow, cleaned up the tests (3 days ago) <Philippus Baalman>
* |   f771395 - Merge pull request scala#5671 from retronym/topic/stubby-2 (3 days ago) <Lukas Rytz>
|\ \
| * | ad13063 - Remove non-essential fix for stub symbol failure (4 days ago) <Jason Zaugg>
| * | 7f2d1fa - Avoid forcing info transforms of primitive methods (2 weeks ago) <Jason Zaugg>
| * | 37a0eb7 - Avoid stub symbol related crash in backend (2 weeks ago) <Jason Zaugg>
* | |   96a7617 - Merge pull request scala#5622 from edmundnoble/extra-errs (4 days ago) <Adriaan Moors>
|\ \ \
| * | | 466e52b - Match error lengths (4 weeks ago) <Edmund Noble>
| * | | d1fc983 - Improved error messages for identically named, differently prefixed types (9 weeks ago) <Edmund Noble>
|  / /
* | |   f2e05c2 - Merge pull request scala#5728 from Philippus/issue/html-tag-in-hover (4 days ago) <Lukas Rytz>
|\ \ \
| | |/
| |/|
| * | e3c5df8 - added missing Inline matches to inlineToStr so it is now exhaustive scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag (9 days ago) <Philippus Baalman>
* | |   920bc4e - Merge pull request scala#5743 from som-snytt/issue/10207-bad-update (7 days ago) <Lukas Rytz>
|\ \ \
| * | | 094f7f9 - SI-10207 Error before update conversion (8 days ago) <Som Snytt>
* | | |   1b4d36f - Merge pull request scala#5746 from paulp/pr/partest (7 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | eac00e1 - Add partest paths to the list of watched sources. (8 days ago) <Paul Phillips>
|/ / /
* | |   5f1a638 - Merge pull request scala#5732 from retronym/topic/build-info-malarkey (10 days ago) <Adriaan Moors>
|\ \ \
| * | | 5e9acac - More predictable performance of SBT build startup, reload (11 days ago) <Jason Zaugg>
|  / /
* | |   759a7b7 - Merge pull request scala#5735 from SethTisue/sd-313 (10 days ago) <Adriaan Moors>
|\ \ \
| * | | ed4ddf5 - increase timeouts on some sys.process tests (11 days ago) <Seth Tisue>
* | | |   e2aaf2c - Merge pull request scala#5723 from dragos/issue/regression-assert-ide (11 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | e5c957e - Fix regression in 5751763 (12 days ago) <Iulian Dragos>
* | | |   f174bfb - Merge pull request scala#5731 from janekdb/issue/scalaGH-644/fix-spec-latex-rendering (11 days ago) <Seth Tisue>
|\ \ \ \
| * | | | ba4c6d4 - scalaGH-644: Remove static html styling of spec code blocks (11 days ago) <Janek Bogucki>
|/ / / /
* | | |   8e40bef - Merge pull request scala#5729 from scala/revert-5658-topic/hashhash (12 days ago) <Adriaan Moors>
|\ \ \ \
| |_|/ /
|/| | |
| * | | 86cd70f - (origin/revert-5658-topic/hashhash) Revert "Fix erasure of the qualifier of ##" (12 days ago) <Adriaan Moors>
|/ / /
* | |   cbf7daa - Merge pull request scala#5681 from Philippus/issue/9704 (13 days ago) <Lukas Rytz>
|\ \ \
| * | | b8a8ac1 - moved Pattern and TagsNotToClose to a HtmlTag companion object (13 days ago) <Philippus Baalman>
| * | | a019082 - SI-9704 don't add a closed HtmlTag if it is already closed (4 weeks ago) <Philippus Baalman>
|  / /
* | |   effde0c - Merge pull request scala#5726 from scala/revert-5629-issue/10120-quote-err (13 days ago) <Adriaan Moors>
|\ \ \
| * | | d60f6e3 - (origin/revert-5629-issue/10120-quote-err) Revert "SI-10133 Require escaped single quote char lit" (13 days ago) <Adriaan Moors>
|/ / /
* | |   a8c4a54 - Merge pull request scala#5663 from gourlaysama/ticket/sd-256-enable-repl-colors-unix-2 (13 days ago) <Adriaan Moors>
|\ \ \
| * | | 6411170 - SD-256 enable colored output by default on unix (13 days ago) <Antoine Gourlay>
* | | |   c96a977 - Merge pull request scala#5658 from retronym/topic/hashhash (13 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | f85c62e - Fix erasure of the qualifier of ## (6 weeks ago) <Jason Zaugg>
|  / /
* | |   76bfb9e - Merge pull request scala#5708 from szeiger/issue/si10194 (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 1d22ee4 - SI-10194: Fix abstract type resolution for overloaded HOFs (13 days ago) <Stefan Zeiger>
|  / /
* | |   dabec1a - Merge pull request scala#5700 from retronym/ticket/10154-refactor (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 06eee79 - Refactor implementation of lookupCompanion (2 weeks ago) <Jason Zaugg>
|  / /
* | |   2f1e0c2 - Merge pull request scala#5704 from som-snytt/issue/10190-elide-string (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 6fb3825 - SI-10190 Elide string to empty instead of null (3 weeks ago) <Som Snytt>
|  / /
* | |   13f7b2a - Merge pull request scala#5640 from optimizely/repl-import-handler (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | aa7e335 - Fix ImportHandler's reporting of importedNames and importedSymbols (8 weeks ago) <Hao Xia>
| * | | c89d821 - Fix SIOOBE in Name#pos for substrings of length 1 (8 weeks ago) <Jason Zaugg>
|  / /
* | |   023a96a - Merge pull request scala#5629 from som-snytt/issue/10120-quote-err (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | 05cc3e2 - SI-10120 ReplReporter handles message indent (7 weeks ago) <Som Snytt>
| * | | 939abf1 - SI-10120 Extra advice on unclosed char literal (8 weeks ago) <Som Snytt>
| * | | 855492c - SI-10133 Require escaped single quote char lit (8 weeks ago) <Som Snytt>
|  / /
* | |   e21ab42 - Merge pull request scala#5660 from som-snytt/issue/9464-spec (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | a6dcceb - SI-9464 Clarify spec on no final trait (6 weeks ago) <Som Snytt>
|  / /
* | |   e87a436 - Merge pull request scala#5659 from retronym/ticket/10026 (2 weeks ago) <Adriaan Moors>
|\ \ \
| |/ /
|/| |
| * | 777a0e5 - SI-10026 Fix endless cycle in runtime reflection (2 weeks ago) <Jason Zaugg>
| |/
* |   23e5ed9 - Merge pull request scala#5707 from retronym/topic/java9-prepare (2 weeks ago) <Lukas Rytz>
|\ \
| * | 96e8e97 - Workaround bug in Scala runtime reflection on JDK 9 (3 weeks ago) <Jason Zaugg>
| * | fe2d9a4 - Avoid ambiguous overload on JDK 9 (3 weeks ago) <Jason Zaugg>
| * | 6bba8f7 - Adapt to change in ClassLoader in JDK 9 (3 weeks ago) <Jason Zaugg>
| * | 8136057 - Bump scala-asm version (3 weeks ago) <Jason Zaugg>
|  /
* |   cad3c3d - Merge pull request scala#5709 from adriaanm/sam_wild_bound (2 weeks ago) <Lukas Rytz>
|\ \
| * | c396e96 - Ignore BoundedWildcardType in erasure type map (2 weeks ago) <Adriaan Moors>
* | |   3e9df41 - Merge pull request scala#5711 from retronym/ticket/jrt (2 weeks ago) <Lukas Rytz>
|\ \ \
| * | | 09c7edc - Faster and simpler Java 9 classpath implementation (2 weeks ago) <Jason Zaugg>
|  / /
* | |   7b9d3cc - Merge pull request scala#5713 from janekdb/issue/scalaGH-644/sync-jekyll-README-to-Gemfile (2 weeks ago) <Lukas Rytz>
|\ \ \
| * | | 5e5ec9a - scalaGH-644: Expand note regarding Jekyll versions (2 weeks ago) <Janek Bogucki>
|  / /
* | |   144f7e0 - Merge pull request scala#5714 from dragos/issue/usage-sterr-SI-10178 (2 weeks ago) <Lukas Rytz>
|\ \ \
| |/ /
|/| |
| * | 640c85e - SI-10178 Route reporter.echo to stdout (2 weeks ago) <Iulian Dragos>
|  /
* |   2fec08b - Merge pull request scala#5717 from som-snytt/issue/10148-followup (2 weeks ago) <Adriaan Moors>
|\ \
| |/
|/|
| * f3d271b - SI-10148 Accept verbose zero (2 weeks ago) <Som Snytt>
* 147e5dd - Merge pull request scala#5716 from adriaanm/i296 (2 weeks ago) <Jason Zaugg>
* 12437a0 - Ensure ordering for args to `choose` in DurationTest (2 weeks ago) <Adriaan Moors>
@lrytz lrytz mentioned this pull request Mar 7, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lrytz lrytz lrytz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.12.2

Development

Successfully merging this pull request may close these issues.

4 participants

@Philippus @SethTisue @lrytz @scala-jenkins