-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change from spans to divs #122
Conversation
… variable license text Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@@ -98,17 +98,17 @@ private void removeEndParagraphTag() { | |||
*/ | |||
public static String formatReplaceabledHTML(String text, String id) { | |||
StringBuilder sb = new StringBuilder(); | |||
sb.append("\n<span "); | |||
sb.append("\n<div "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going too far in the other direction :p. In #121, I'm advocating <var>
(or <span>
) when we're wrapping phrasing content, and <div>
when we're wrapping flow content. I don't think there's a single element that will cover both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is used for the old license spreadsheet and license text. I'll change this back to spans since the old format will not include any phrasing content.
It would be design change with some effort to detect if there is phrasing content. The div's seem to render correctly if I use the inline style. @wking what is the effect of wrapping non-phrasing content with an inline |
On Wed, Nov 29, 2017 at 08:59:23PM +0000, goneall wrote:
what is the effect of wrapping non-phrasing content with an inline
<div>?
Upon closer inspection, it appears that the phrasing-content elements
[1] are a strict subset of the flow-content elements [2]. So
including phrasing content in a div is valid HTML.
However, because a div is not phrasing content itself, you cannot use
it inside an element that requires phrasing content. For example, [3]
now has broken newlines around these divs. And the W3C's validator
has complaints like [4]:
No p element in scope but a p end tag seen.
where it hits the </p> in a <p>…<div>…</div>…</p> situation.
That error message isn't great because HTML has lots of rules about
implicit paragraph tags; you can get a clearer error message for
<b><div></div></b>, where the validator reports:
Element div not allowed as child of element b in this
context. (Suppressing further errors from this subtree.)
From line 8, column 6; to line 8, column 34
ody>↩ <b><div style="display: inline">Hello<
Contexts in which element div may be used:
Where flow content is expected.
As a child of a dl element.
Content model for element b:
Phrasing content.
You can include a var in phrasing content (e.g. <b><var>…</var></b>).
You cannot include flow content inside var (e.g. <var><p>…</p></var>),
with W3C's validator reporting:
Element p not allowed as child of element var in this
context. (Suppressing further errors from this subtree.)
From line 7, column 10; to line 7, column 12
↩ <var><p>hi</p>
Contexts in which element p may be used:
Where flow content is expected.
Content model for element var:
Phrasing content.
So I think we want to be using var whenever possible, and only falling
back to div when the element contains flow content. That way we
aren't changing the content type with our wrapping.
[1]: https://www.w3.org/TR/2014/REC-html5-20141028/dom.html#phrasing-content-1
[2]: https://www.w3.org/TR/2014/REC-html5-20141028/dom.html#flow-content-1
[3]: https://spdx.org/licenses/preview/ISC.html
[4]: https://validator.w3.org/nu/?doc=https%3A%2F%2Fspdx.org%2Flicenses%2Fpreview%2FISC.html
|
Hmmm - looks like something I need to fix. May take a couple days since this will require some additional parsing. |
On Wed, Nov 29, 2017 at 11:46:04PM +0000, goneall wrote:
May take a couple days since this will require some additional
parsing.
I think you probably just need logic like “if our alt/optional
contains a p or list element, use a div, otherwise use a var”.
And I don't think there's a hurry. spdx/license-list-XML#327 will
take a while to chew through, and we don't need pretty HTML until that
is done.
|
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
Added a commit to conditionally use a Attached is the generated ISC license. The var and optional text passes the w3c validator and it seems to render OK on Chrome. |
if (id != null && !id.trim().isEmpty()) { | ||
sb.append("id=\""); | ||
sb.append(escapeIdString(id)); | ||
sb.append("\" "); | ||
} | ||
sb.append("class=\""); | ||
sb.append(REPLACEABLE_LICENSE_TEXT_CLASS); | ||
sb.append("\">"); | ||
sb.append("\" style=\"display: inline\">"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With <var>
for phrasing content, I don't think you need this style anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be needed for the div's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be needed for the div's?
I don't think so, because all the inline cases will now be var
. This styling was a workaround for a case (inline div
s) that should no longer happen.
…ng element Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
Check the latest commit - see if that fixes it. The These constants refer to the input license XML elements (not the output HTML elements). |
How about alt's or optionals embedded in other optionals? Should those be wrapped in a div? Otherwise we would have |
In looking at the W3C validator link above, I noticed the following error:
Is this anything to worry about? |
…nges to the HtmlTemplateOutputHandler - back to spans Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
When none of the content contains a p or list element (at any depth, not just as immediate children), then I think we should be using
If you mean the “Error: End tag
And we're serving that content as HTML, not XHTML: $ curl -sI https://spdx.org/licenses/preview/ISC.html | grep Content-Type
Content-Type: text/html; charset=UTF-8 But that's orthogonal to this issue. |
@@ -117,6 +117,12 @@ | |||
EXAMPLE_UNPROCESSED_TAGS.add(LICENSEXML_ELEMENT_BULLET); | |||
} | |||
|
|||
static HashSet<String> PHRASING_ELEMENTS = new HashSet<String>(); | |||
static { | |||
PHRASING_ELEMENTS.add(LICENSEXML_ELEMENT_LIST); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHRASING_ELEMENTS
still needs to be renamed to FLOW_CONTENT_ELEMENTS
(or similar).
sb.append(HtmlTemplateOutputHandler.OPTIONAL_LICENSE_TEXT_CLASS); | ||
sb.append("\">"); | ||
sb.append(childSb.toString()); | ||
sb.append("</span>"); | ||
if (includesPhrasing(element)) { | ||
sb.append("</div>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a bit DRYer and slightly more performant if you had a single:
if (includesFlowContent(element)) {
tag = "div";
} else {
tag = "var";
}
and then used that tag
variable for both the start and end tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - I was just being lazy ;) I'll update the PR
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
* @param element parent element | ||
* @return true if the element includes any phrasing content per https://www.w3.org/TR/2014/REC-html5-20141028/dom.html#phrasing-content-1 | ||
*/ | ||
private static boolean includesPhrasing(Element element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should be includesFlowControl
, and you'll want to update its comment, callers, etc. This is bubbling up the FLOW_CONTROL_ELEMENTS
rename to use the correct name throughout the new code.
* @return true if the element includes any phrasing content per https://www.w3.org/TR/2014/REC-html5-20141028/dom.html#phrasing-content-1 | ||
*/ | ||
private static boolean includesPhrasing(Element element) { | ||
NodeList children = element.getChildNodes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getChildNodes
seems to only return immediate children. We want something that iterates over all descendants to catch <optional>…<optional><p>…</p></optional></optional>
. A quick grep through the docs didn't turn up anything that sounded right, so we may need to implement this locally (or call includesFlowControl
recursively?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It already does a recursive search (line 355)
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
Just pushed a fix for the end tag br. |
Is this an issue with the templates in the resources folder (e.g.
|
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
I believe all the comments have been resolved and a new issue was spun off to track the XHTML issue #123 |
Change from spans to divs for the HTML representation of optional and variable license text.
Address issue #121
Signed-off-by: Gary O'Neall gary@sourceauditor.com