Skip to content
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

Fix Issue 17594 - Define DDOC_BLANKLINE as an empty HTML paragraph, t… #1795

Merged
merged 1 commit into from Jul 5, 2017

Conversation

andralex
Copy link
Member

@andralex andralex commented Jul 4, 2017

…hus obviating the need to wrap text in $(P ...)

There are two typographical notions related to DDOC_BLANKLINE: "paragraph separator" (LaTeX: \par) and "vertical space" (LaTeX: \vspace). Of these, "paragraph separator" is the closest and most useful.

Paragraph separators must enjoy somewhat special treatment: adjacent paragraph separators should be merged together so empty paragraphs do not increase vertical spacing. Even if a paragraph has only whitespace, it should be considered empty and merged along with surrounding separators. LaTeX's \par is defined as such.

Fortunately, in HTML empty paragraphs designated as <p></p> do the same thing:

  • Runs of <p></p> and whitespace (either between the tags or between tag pairs) are always merged together so there's no difference in text rendering
  • If <p></p> is inserted between two paragraphs <p>... text ...</p> and <p>... more text ...</p>, it produces no additional spacing between paragraphs
  • If <p></p> is inserted between two blocks of text that are not tagged with <p> (e.g. are body-level), a paragraph separator space is inserted.

That all means with this PR, we don't need to use $(P ...) around text in .dd files.

This change will enter in effect after dlang/dmd#5344 is merged.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @andralex!

Bugzilla references

Fix Bugzilla Description
17594 Define DDOC_BLANKLINE as an empty HTML paragraph, thus obviating the need to wrap text in $(P ...)

@CyberShadow
Copy link
Member

Hmm. Seems like there's two notions at play here, depending on the output format:

  • Paragraph separator (LaTeX's \par).
  • Paragraph block (HTML's <p>).

As I understand, this tries to simulate paragraph separators in HTML by using an empty <p> block.

What happens if the surrounding text is already in a <p> block? I'm pretty sure that paragraph tags aren't nestable, meaning that in <p>a<p></p>b</p>, the b text is going to be completely outside any paragraph block.

It would certainly be a lot nicer to see a proper attempt at dlang/dmd#4338 instead of trying to hack our way around this problem,

Let's see what the doc tester results are going to look like.

@aG0aep6G
Copy link
Contributor

aG0aep6G commented Jul 4, 2017

We already have something similar in std.ddoc: DDOC_BLANKLINE = $(DIVC blankline)$(LF).

I suggest moving that to dlang.org.ddoc if the behavior is desired for the whole site. I'd prefer the div because it doesn't make a statement about its content, so it's less of a violation of semantic HTML.

@andralex
Copy link
Member Author

andralex commented Jul 4, 2017

@aG0aep6G good idea, done.

@adamdruppe
Copy link
Contributor

adamdruppe commented Jul 4, 2017 via email

@andralex
Copy link
Member Author

andralex commented Jul 4, 2017

Unrelated: @CyberShadow @s-ludwig I see a lot of errors in http://dtest.dlang.io/results/c0008ecf5c03916698a3295aeab89f0ac72bfa29/6c1f175178e8d33718e19c8aff2fb0c8fab2f349/build.log, even in a successful build, which I assume are benign. Is anyone looking over those? Are those adversely affecting the produced ddox?

@CyberShadow
Copy link
Member

Front page has regressed (layout slightly broken, example roulette gone)

@CyberShadow
Copy link
Member

CyberShadow commented Jul 4, 2017

https://github.com/dlang/dlang.org/blob/master/spec/expression.dd#L38-L45 looks fine, but new rendered version has a gratuitous amount of empty space between the definitions of Expression and CommaExpression

@andralex
Copy link
Member Author

andralex commented Jul 4, 2017

@CyberShadow thx. Looked into that, it's because that text is inside a <pre> block. Previously a \n was inserted which was rendered as an empty line. Then with <p> the space is larger, and with <div blankline> it's even larger. I remove the empty line because it was out of place in the first time.

@andralex
Copy link
Member Author

andralex commented Jul 4, 2017

Still investigating why the example roulette is gone. Insights?

@CyberShadow
Copy link
Member

CyberShadow commented Jul 4, 2017

I remove the empty line because it was out of place in the first time.

There's going to be a lot of instances of these, that one was just an example; changing the source one at a time is not a "scalable solution".

I think a better approach would be a CSS rule that completely hides the divs in pre blocks, as pre blocks are already preformatted and don't need extra markup to indicate where empty lines should be.

@CyberShadow
Copy link
Member

Still investigating why the example roulette is gone. Insights?

@@ -250,6 +257,7 @@ Neptune</code>
     var rouletteIndex = Math.floor(Math.random() * examples.length);
     var rouletteChild = examples[rouletteIndex];
     rouletteChild.style.display = "block";
+<div class="blankline"></div>
 
     // build a list of the titles of all examples and add an option chooser to
     // allow switching between them

It's inserting HTML in inline <script> blocks.

@CyberShadow
Copy link
Member

@CyberShadow
Copy link
Member

CyberShadow commented Jul 4, 2017

Either way, seems like this change breaks a lot of stuff, so will require a careful review of all changed pages. Before we further invest into it, do you have any simpler alternatives in mind? Perhaps defining this macro only on spec / article pages?

@adamdruppe
Copy link
Contributor

adamdruppe commented Jul 4, 2017 via email

@CyberShadow
Copy link
Member

Maybe ddoc needs something similar too.

That would be https://issues.dlang.org/show_bug.cgi?id=16498 :)

@andralex
Copy link
Member Author

andralex commented Jul 4, 2017

I think a better approach would be a CSS rule that completely hides the divs in pre blocks, as pre blocks are already preformatted and don't need extra markup to indicate where empty lines should be.

That's a pretty cool idea, could you please implement it in parallel with this?

@CyberShadow
Copy link
Member

That's a pretty cool idea, could you please implement it in parallel with this?

OK:

pre div.blankline { display: none; }

Here's your implementation :)

@andralex andralex force-pushed the DDOC_BLANKLINE branch 3 times, most recently from 40e1d7b to 275258c Compare July 4, 2017 22:12
@andralex
Copy link
Member Author

andralex commented Jul 4, 2017

@CyberShadow cool, I added that to the css and also similar directives for lists (we want lists to be compactly spaced). I've also restricted the blanklines thing to the spec.

@@ -38,7 +38,6 @@ $(H3 $(LEGACY_LNAME2 Expression, expression, Expressions))
$(GRAMMAR
$(GNAME Expression):
$(I CommaExpression)

Copy link
Member

Choose a reason for hiding this comment

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

I understand this commit is no longer needed.

index.dd Outdated
@@ -126,7 +126,6 @@ $(SCRIPT
var rouletteIndex = Math.floor(Math.random() * examples.length);
var rouletteChild = examples[rouletteIndex];
rouletteChild.style.display = "block";

Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@@ -847,7 +847,7 @@ h3.download

div.summary, div.description, div.keyval, div.blankline
{
margin: 1.12em 0;
margin: 1em 0;
Copy link
Member

Choose a reason for hiding this comment

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

Let's see what this looks like

@@ -123,7 +123,7 @@ DDOC_PARAM_ROW = <tr class="param">$0</tr>
DDOC_PARAM_ID = <td class="param_id">$0</td>
DDOC_PARAM_DESC = <td class="param_desc">$0</td>
DDOC_PARAMS = $(DDOCKEYVAL Parameters, <table class="params">$0</table>)
DDOC_BLANKLINE =
Copy link
Member Author

Choose a reason for hiding this comment

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

I assume this still appears as a change because it was a tab.

Copy link
Member

Choose a reason for hiding this comment

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

Heh, indeed

Copy link
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase to squash the redundant commits.

@andralex andralex force-pushed the DDOC_BLANKLINE branch 2 times, most recently from 3d01c58 to f223927 Compare July 5, 2017 03:23
…hus obviating the need to wrap text in $(P ...)
@dlang-bot dlang-bot merged commit a54adec into dlang:master Jul 5, 2017
@andralex andralex deleted the DDOC_BLANKLINE branch July 5, 2017 03:35
@s-ludwig
Copy link
Member

s-ludwig commented Jul 5, 2017

@andralex:

Unrelated: @CyberShadow @s-ludwig I see a lot of errors in http://dtest.dlang.io/results/c0008ecf5c03916698a3295aeab89f0ac72bfa29/6c1f175178e8d33718e19c8aff2fb0c8fab2f349/build.log, even in a successful build, which I assume are benign. Is anyone looking over those? Are those adversely affecting the produced ddox?

I'll open a ticked against DDOX for these. Usually this will result in a raw type being displayed instead of one that is properly cross linked.

@CyberShadow
Copy link
Member

Heh, I wrote to squash unrelated commits, not all of them...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants