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

WIP Improve redundant p tags #7061

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

pmario
Copy link
Member

@pmario pmario commented Nov 25, 2022

Important: Do NOT use the demo-wiki to publish anything -- It's for testing only!!!

File to play with: p-tags-parser-changed.zip


  • This is a draft, to show the changes and play with it
  • It's intended for discussion.
  • This PR contains visual differences between tiddlywiki.com and the demo-wiki -> that's intentional
  • wikiparser.js is the only file that contains code changes
  • The rest of the 112 files are only elements that need checking, because the PTAG is missing there.
    • I think most changes are cosmetic only. .. BUT I'm not sure yet, since I didn't have a closer look
  • The easiest way to see the difference is to use a little stylesheet as shown below
  • As I wrote at Talk, I did a visual side-by-side comparison for about 1000 tiddlers at tiddlywiki.com
    • The images and the xxx-todo tiddler list the problems that I found ...
      • I think it's not many, since most of them seem to have the same origin
  • The demo wiki here uses blue outline
  • tiddlywiki.com screenshots use red outline
  • This PR uses some code that is not ES2015 children[0]?.tag?.charAt(0) ... that's intentional
p {
  outline: 1px solid blue; /* for the demo-wiki || red for tiddlywiki.com screenshots */
} 

@pmario pmario marked this pull request as draft November 25, 2022 11:00
@pmario pmario changed the title Fix redundant p tags WIP Improve redundant p tags Nov 25, 2022
@pmario
Copy link
Member Author

pmario commented Nov 25, 2022

I think I did identify the main problem, which imo is in the WikiParser .parseBlock() function. ...

/*
Parse a block from the current position
	terminatorRegExpString: optional regular expression string that identifies the end of plain paragraphs. Must not include capturing parenthesis
*/
WikiParser.prototype.parseBlock = function(terminatorRegExpString) {
	var terminatorRegExp = terminatorRegExpString ? new RegExp("(" + terminatorRegExpString + "|\\r?\\n\\r?\\n)","mg") : /(\r?\n\r?\n)/mg;
//                                                        I think from a logical point of view the OR ^^^^^^^^^^^^^   also causes problems
// I think double newline detection should have been a concrete wikitext rule ... but I'm not sure here.
//
	this.skipWhitespace();
	if(this.pos >= this.sourceLength) {
		return [];
	}
	// Look for a block rule that applies at the current position
	var nextMatch = this.findNextMatch(this.blockRules,this.pos);
	if(nextMatch && nextMatch.matchIndex === this.pos) {
		return nextMatch.rule.parse();
	}
	// Treat it as a paragraph if we didn't find a block rule
	var start = this.pos;
	var children = this.parseInlineRun(terminatorRegExp);
	var end = this.pos;
//	I did remove this line here, because it assumes that if this position is reached, 
//	all children have to be covered in a P-TAG ... 
//	-> IMO that assumption is wrong. Examples follow
//	return [{type: "element", tag: "p", children: children, start: start, end: end }];
	// I did add the following 6 lines to change the ptag handling
	if (children[0].type === "text" ||
		($tw.config.htmlBlockElements.indexOf(children[0].tag) === -1 && children[0]?.tag?.charAt(0) !== "$")) {
		return [{type: "element", tag: "p", children: children, start: start, end: end }];
	} else {
		return children;
	}
	// --- end ptag handling ---
};

@pmario
Copy link
Member Author

pmario commented Nov 25, 2022

The simplest example I could come up with is as follows. None of them should have a P tag around them.

<div>Some text line 1</div>
<div>Some text line 1</div>

<div>Some text line 1</div>

<div>Some text line 1</div>

tiddlywiki.com shows:

image

The demo wiki shows

I'm sure I did not get all the exceptions but the improvement is significant

image

@pmario
Copy link
Member Author

pmario commented Nov 25, 2022

There is a problem in the block detection if new-lines at the end of block elements is missing. So that also indicates, that block-end recognition has a problem.

<$text text="some text -- "/>

<$text text="some more text "/>           <-- add or remove new-lines here and see the difference

image

@pmario
Copy link
Member Author

pmario commented Nov 25, 2022

The demo wiki get's it right but has an interesting behaviour if a HTML comment is there. See the difference between

<$text text="some text -- "/>

<$text text="some more text "/><!-- comment -->

and

<$text text="some text -- "/>

<$text text="some more text "/>

<!-- comment -->

image

@pmario
Copy link
Member Author

pmario commented Nov 25, 2022

I think the following behaviour comes from this part of the code |\\r?\\n\\r?\\n)","mg") : /(\r?\n\r?\n)/mg;
Both demo-wiki and tiddlywiki.com have the same behaviour here

image

@pmario
Copy link
Member Author

pmario commented Nov 25, 2022

This PR is related to: #6156

@Alzacon
Copy link
Contributor

Alzacon commented Jan 12, 2023

The related issues to the change in the parse can be grouped in two point:

  • Some widgets return directly text or inline element, their output should be wrapped in the nearest parent that has block element, if it doesn't have parent but it is between break line it would be wrapped in a block element. This could continue be a p element (or a div), how it works in the wiki syntax. The same case is the macros that generate link, for example the .operator-examples macro.
  • listwidget is a special case, it can return a list of text elements or widgets. Its output should be wrapped in a div (to contain other generated block elements). If its contain will work how I comment in the previous point, there shouldn't be relative issues.

The other issues are related a current macros.

  • .tip and .warning macros have the same issue but in the current version the margin of p, that is wrapping the link, hides the problem, but it can also appreciate the problem in the gray border. If the output of .operator-examples macro was wrapped in a p element, the issue is hidden again. The fix will be fit the min-height of .doc-icon-block to the image. The thumbnail macro has the same problem and fix.
  • wikitext-example macro works because there is a weird behaviour of the empty p that is followed the pre, their margin collapse to 0.

@vercel
Copy link

vercel bot commented Jan 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ❌ Failed (Inspect) Mar 15, 2024 9:23pm

@rmunn
Copy link
Contributor

rmunn commented Nov 9, 2023

I was just dealing with a problem trying to make a grid-based layout for editions/tw5.com, which includes the menubar plugin. The menubar is transcluded and has an extraneous <p> element that I could not get rid of, which is a problem because the rules for layout: grid say that the elements in the grid must be direct children. I was able to solve that in the browser dev tools by adding display: contents to the extraneous <p> tag, but of course it's not possible to use that as a generic solution (not all <p> tags added by TW should have display: contents, in fact most of them shouldn't).

But I added the 5-line change to wikiparser.js to my local copy of TW, and voila: the unwanted <p> tag was gone, and the menubar was part of the layout the way it should be (as a direct child of the grid element).

So this PR, if polished up to something that makes all the tests pass, would solve a real problem I'm having that I don't currently see any other way to solve.

@pmario
Copy link
Member Author

pmario commented Nov 9, 2023

So this PR, if polished up to something that makes all the tests pass, would solve a real problem I'm having that I don't currently see any other way to solve.

The tests show "hidden problems". The fixes are relatively straight forward. It needs to be fixed in a theme CSS.

But that fix will make the whole thing not backwards compatible. So it will most likely cause problems with the vanilla theme.

It may be possible to create a plugin, that overwrites the block-mode parser, but that's more or less the same as starting a "patch set" for TW.

On the other hand, it would be a description, what needs to be changed, to fix the problems. So it's probably worth considering.


I think it's also the only way to fix #6687 which bugs me for a long time already

…nts: button, radio, checkbox, link -- In HTML they are inline, but for backwards compatibility they need to be covered in P tags :/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants