Page MenuHomePhabricator

Highlight void tags and other tags correctly
Closed, ResolvedPublic5 Story Points

Description

Currently, all tags are treated in the XML way, which is wrong.

a<br>b</br>c
Expected: <br> green, </br> red
Actual: both green
Void HTML tags must not have an end tag. This applies to br, hr and wbr.

a<div>b<br>c</div>d
Expected: all three tags in green
Actual: </div> red
Void HTML tags are automatically closed, so this nesting is correct.

a<div />b
Expected: red
Actual: green
While self-closed HTML tags currently work as expected, they are deprecated. A self-closing tag should only be accepted for void HTML tags, and for MediaWiki tags (note that pre is a MediaWiki tag in this sense).

Event Timeline

Schnark created this task.Jul 11 2017, 7:55 AM
Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptJul 11 2017, 7:55 AM
Pastakhov triaged this task as Normal priority.
kaldari set the point value for this task to 5.Oct 3 2017, 10:47 PM
kaldari moved this task from To be estimated/discussed to Estimated on the Community-Tech board.
TBolliger moved this task from Estimated to Archive on the Community-Tech board.Feb 14 2018, 12:58 AM

I started to work on this, then noticed @Pastakhov had already claimed the task. I will not step on any toes, but allow me to share what little I came up with:

In mediawiki.js, under permittedHtmlTags I added a new definition set for void HTML tags: voidHtmlTags = { br: true, hr: true, wbr: true }

Then in eatWikiText, line 798, I added:

if ( isCloseTag === true && tagname in voidHtmlTags ) {
	// @todo message
	return 'error';
}

Pretty straightforward. This makes </br>, etc., highlight as red, but it still queues up <br> as if it needed a closing tag. That's where I got stuck. I'm somewhat confident I can figure it out with time, so let me know if you need help.

Another thing... we might need some special handling for <p>. This is valid by itself or with a closing tag. To be precise, the opening tag would have to be followed by flow/palpable content, but the MediaWiki parser will convert plain text into <p> tags anyway, so you shouldn't have to go crazy with validating adjacent content. Instead, just allow people to put <p> by itself, which from my experience is already a common practice in the wiki world.

@MusikAnimal I don't think Pastakhov is working on this. You can safely take it. You approach seems sane.

Niharika removed Pastakhov as the assignee of this task.Mar 5 2018, 11:16 PM
Niharika added a subscriber: Pastakhov.
MusikAnimal edited projects, added Community-Tech-Sprint; removed Community-Tech.
MusikAnimal moved this task from Ready to In Development on the Community-Tech-Sprint board.

Change 416628 had a related patch set uploaded (by MusikAnimal; owner: MusikAnimal):
[mediawiki/extensions/CodeMirror@master] Highlight void tags and invalid self-closing tags correctly

https://gerrit.wikimedia.org/r/416628

Niharika closed this task as Resolved.Mar 6 2018, 8:58 PM
Niharika moved this task from Needs Review/Feedback to Q1 2018-19 on the Community-Tech-Sprint board.

Works great!

Gonna close this ticket. Thanks Leon!

Wait a minute.
I note that the ticket description says -
a<div />b
Expected: red
Actual: green

That should have been fixed by this patch, right?
FYI, the patch is on commtech wiki for testing.

Wait a minute.
I note that the ticket description says -
a<div />b
Expected: red
Actual: green
That should have been fixed by this patch, right?
FYI, the patch is on commtech wiki for testing.

I guess I forgot to say that I couldn't figure that one out. It's not the same implementation approach, that's for sure, so I think it may warrant its own ticket.

Another thing... this all seems very fragile. I put some console output to test the flow of execution, and it seems any "state" you set is going to affect everything that follows it, with every keystroke. I'm not saying this is a bad system (performance is quite good, after all), but my point being one could easily fix something and inadvertently break something else. Manually testing for regressions with every change isn't feasible. At quick glance, the code looks unit testable to me, using something like Jasmine (as opposed to acceptance tests, which are a huge, huge pain). This would probably be worth our while if we plan on making more logical changes.

@Niharika It looks like this wasn't merged: https://gerrit.wikimedia.org/r/#/c/416628/ Did you mean to +2 ? :)

Oh whoops. I did mean to +2 it. :)

Change 416628 merged by jenkins-bot:
[mediawiki/extensions/CodeMirror@master] Highlight void tags and invalid self-closing tags correctly

https://gerrit.wikimedia.org/r/416628