Page MenuHomePhabricator

Hovercards: Fix the bracket removal code
Closed, ResolvedPublic

Description

Read the string character by character in JavaScript instead of using Regular Expressions.


Version: unspecified
Severity: normal

Details

Reference
bz65138

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:16 AM
bzimport added a project: Page-Previews.
bzimport set Reference to bz65138.
bzimport added a subscriber: Unknown Object (MLST).

The current implementation has the following problem - http://rubular.com/r/DDFhG1LaNt

We need to select and remove only the first level of brackets.

Change 135759 had a related patch set uploaded by Prtksxna:
render.article: Remove brackets using code instead of RegExp

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

However, see also https://www.mediawiki.org/w/index.php?title=Talk:Beta_Features/Hovercards&workflow=rvrf5mi32feehqf3

Re-factored copy of the first comment in that topic:

Hovercards don't appear to render text in brackets from the target page. For instance, hovercards concerning the Wikipedia page for [[RAF Hospital Wegberg]] state that it is

"commonly abbreviated to RAFWegberg"

while the page itself states that it is

"commonly abbreviated to RAF(H) Wegberg"

both the '(H)' and the space after it appear to be missed by the hovercard.

See the thread for full discussion.

This 'bracket removal' might need to be re-examined. (Note that it's something Navpopups gets wrong, too. Not an easy problem to solve.)

(In reply to Quiddity in comment #3)

The space after the bracket problem will get fixed with this patch. Do we also want to remove stuff inside square brackets, as mentioned by Sasuke_Sarutobi?

Is it a possibility that we'll have to be much smarter about this and not blindly remove things within brackets?

Navpopups just skips templates altogether, so that is a different problem in that regard.

(In reply to Prateek Saxena from comment #4)

(In reply to Quiddity in comment #3)

The space after the bracket problem will get fixed with this patch. Do we
also want to remove stuff inside square brackets, as mentioned by
Sasuke_Sarutobi?

Is it a possibility that we'll have to be much smarter about this and not
blindly remove things within brackets?

That's a question for Dan. CCing.

Change 135759 merged by jenkins-bot:
render.article: Remove brackets using code instead of RegExp

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

All patches mentioned in this report were merged - is there more work left to do here (if yes: please reset the bug report status to NEW or ASSIGNED), or can you close this ticket as RESOLVED FIXED?

No reply to comment 8.
All patches mentioned in this report were merged or abandoned - assuming this bug is FIXED. If that is not the case: Please reopen and elaborate what is left to do here to get this report fixed.