I think it's fine to hand-parse <br/> into newlines, but in general a bad idea to inject HTML from recipe output into the page.

It works currently, I assume we want parity with the old system?

We want to imitate it as close as possible, but not copy its flaws verbatim.  Injecting html is a little scary of a prospect, but supporting formatting tags currently used is a fine compromise (and avoids fears of xss/csrf)

How about we whitelist <br> (and <br/>) tags since it will solve this problem and won't be harmful? In the future we can clean up places generating them.

Please assign back to me if you won't be able to work on this and I'll find someone else. I'd like to do it before I show it to the rest of Sydney since there are many blink people there. Thanks!

 Issue 671747  has been merged into this issue.

for CrOS, i believe only supporting <br> tags should be sufficient.  when we want links, we use STEP_LINK instead.

if you're planning on a regex, might suggest:
 r'<br */>'

CL out (uses "<br */*>")

https://codereview.chromium.org/2608493002

Demo:
https://1351-6eccb06-tainted-hinoka-dot-luci-milo.appspot.com/buildbot/chromium.fyi/WebKit%20Linux%20-%20RandomOrder/1068

The following revision refers to this bug:
  https://chromium.googlesource.com/external/github.com/luci/luci-go.git/+/95bcda791c9722b5ea601b34e9ae309d12754d3d

commit 95bcda791c9722b5ea601b34e9ae309d12754d3d
Author: hinoka <hinoka@google.com>
Date: Wed Dec 28 19:36:43 2016

Milo: Parse <br> from step text and renders the line breaks

BUG= 670172 

Review-Url: https://codereview.chromium.org/2608493002

[modify] https://crrev.com/95bcda791c9722b5ea601b34e9ae309d12754d3d/milo/appengine/buildbot/build.go
[modify] https://crrev.com/95bcda791c9722b5ea601b34e9ae309d12754d3d/milo/appengine/buildbot/expectations/win_chromium_rel_ng.246309.build.json
[modify] https://crrev.com/95bcda791c9722b5ea601b34e9ae309d12754d3d/milo/appengine/frontend/expectations/bootstrap-buildbot.TestableBuild-Debug_page-_win_chromium_rel_ng_246309.html
[modify] https://crrev.com/95bcda791c9722b5ea601b34e9ae309d12754d3d/milo/appengine/frontend/expectations/buildbot-buildbot.TestableBuild-Debug_page-_win_chromium_rel_ng_246309.html

We would like unordered lists too.

tansell, can you provide an example link?

would generating `* <list item text>` step text entry for each list item solve your problem?

I've repurposed this bug to support <br>, so please file a new one if you want to have arbitrary html support

I've created https://bugs.chromium.org/p/chromium/issues/detail?id=704387

It includes examples and justifications why I think we need each formatting type.

The fix was done in buildbot code, thus it wasn't applied to LUCI. Look at step 164 in https://luci-milo.appspot.com/swarming/task/35365242dffc9d10

perhaps we should have a common function that normalizes an annotation proto so that both swarming and buildbot code can use it

I think supporting <br>s and eating double-<br>s are two different things.
https://luci-milo.appspot.com/swarming/task/35365242dffc9d10
still displays <br>s

OP link is now readable, no?

yes, OP link is now readable. It is one of two use cases. We've fixed the bug in the buildbot use case, but the bug still exists in the swarming case.

I think we need to pick one of 3 things for swarmbucket:
* General markdown support crbug.com/704387
* Styling defined strictly in the proto
* parsing <br> (this bug)

Given the other two options, I'm not convinced this is the route we want to pursue, because that would be too many different modes of operations.

Removing myself as owner since i'm not working on this, and the pre-req is to pick a step_text strat we want to pursue and drop the other ones.

I agree that it is cleaner not to have ad-hoc support for <br> in swarmbucket case. In buildbot we don't have an option because we have to be compatible. In swarmbucke twe don't have to. I will move this to Milo>Buildbot component and mark it as fixed.

We will fix the https://luci-milo.appspot.com/swarming/task/35365242dffc9d10 problem in a different way, perhaps by producing separate step_text values for separated lines

https://luci-milo.appspot.com/swarming/task/35365242dffc9d10 is showing <br> - whats up?

See 2nd paragraph of comment 22

it appears that recipe engine supports only one line https://cs.chromium.org/chromium/infra/recipes-py/recipe_engine/types.py?q=step_text+package:%5Echromium$&l=84
and that's why users put <br>s in step_text
so it seems hard to change this

we may have to support a subset of HTML in step_text

I think it's probably fine in v1 to support a subset of html; that's the API that buildbot made available. Soon after luci-lite we should scrape and review usage of the annotation protos and formalize concepts like these into a v2 proto, then do a transition. Since we control the recipe engine, we can push the html subset down to the recipe engine (which will do html -> proto v2). Eventually we can push it down to the actual recipes (i.e. give the recipes a solid v2 api to use directly).

Are you saying we need to represent rich text using protobufs? Why?

I think it is simpler to write a parser of a very limited (I mean VERY limited) subset of HTML and use that as the language. Dnj loves this kind of tasks.