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

Autolink username via the @username syntax miss #735

Open
Martii opened this issue Sep 4, 2015 · 14 comments
Open

Autolink username via the @username syntax miss #735

Martii opened this issue Sep 4, 2015 · 14 comments
Labels
bug You've guessed it... this means a bug is reported.

Comments

@Martii
Copy link
Member

Martii commented Sep 4, 2015

@swole_hamster is showing as @swole_hamster ... EDIT: ... from here.

Ref:

  • 091ef9c (miss here probably because tokens are split with regular expression look-ahead non-capture default from here ... or try to use marked lexer?)
  • 6d31f78 (possibly revert until a work-around or fix is found)

Trace:

> renderer.text()
Crowd Task Google Search Link by @swole
> renderer.text()
_hamster
@Martii Martii added the bug You've guessed it... this means a bug is reported. label Sep 4, 2015
@Martii
Copy link
Member Author

Martii commented Oct 8, 2015

And another one with info@omertabeyond.com here.

Possibly need to see if there is a look ahead and a look behind option... and during require a space before the at symbol.

@sizzlemctwizzle
Copy link
Member

To stop matching email addresses replace the regex here with this: /(?:^|\s)@([^\s\\\/:*?\'\"<>|#;@=&]+)/

I'm not sure why the underscore is breaking the match yet.

@Martii
Copy link
Member Author

Martii commented Oct 19, 2015

I'm not sure why the underscore is breaking the match yet.

The reason is in the trace message... e.g. the package is splitting a "chunk", processing that "chunk", and we're missing it due to routine we are using with it... this is why it would be useful to have a look ahead and look behind with that package so we could see if the "chunk" needs aggregation.

@sizzlemctwizzle
Copy link
Member

To be fair. The text rendering method isn't one listed in the official api. I found it by going through the source of the package.

@sizzlemctwizzle
Copy link
Member

The html renderer method would work, but I was trying to avoid linking in code and pre blocks.

@sizzlemctwizzle
Copy link
Member

We could probably do it in here but skip when aType === 'html' so we don't do it twice.

@sizzlemctwizzle sizzlemctwizzle self-assigned this Oct 19, 2015
@sizzlemctwizzle
Copy link
Member

I'll take this issue since it was my feature.

@Martii
Copy link
Member Author

Martii commented Oct 19, 2015

but I was trying to avoid linking in code and pre blocks.

Might be a little irritating when we get notifications (#15 and/or #242) but may not... since I can divine this is another reason why you wanted this feature. ;)

@Martii
Copy link
Member Author

Martii commented Nov 23, 2015

@sizzlemctwizzle
Should this be autolinking in code md blocks? e.g. @sizzlemctwizzle ??

@Martii Martii reopened this Nov 23, 2015
@Martii Martii added the question A question has been encountered by anyone and has remained unanswered until cleared. label Nov 23, 2015
@Martii
Copy link
Member Author

Martii commented Nov 23, 2015

@sizzlemctwizzle Cc: @Zren

Do either of you know if there is an equivalent of DocumentFragment in node (preferably native)? It seems that we need to check the parent node to see if it's a pre or code tag... if so don't do the replacing. Using a regular expression with dynamic content can get tricky. Back on USO I started using a document fragment to detect things... in this case we would possibly need to modify the text nodes then export out the innerHTML equivalent, assuming there is one.

Closest non-native I have found is html-document but that says it is only a partial implementation.

@Martii
Copy link
Member Author

Martii commented Nov 23, 2015

I do see a xml parser/serializer at xmldomᴳᴴ (with xpathᴳᴴ)... it seems to me that express should have this as a method somewhere though. e.g. convert a string to a DOM and a DOM to a string for html (poorly formed) and xml (strictly formed).

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Nov 25, 2015
* This is definitely **not** the final solution as this only covers single line `code` and not multi-line
* HTML `pre` blocks seem unaffected

Applies to OpenUserJS#735
@Martii Martii added the later A long time ahead, in a galaxy near, near... label Nov 25, 2015
@Martii
Copy link
Member Author

Martii commented Nov 25, 2015

Another miss at https://openuserjs.org/scripts/TimidScript/%5BTS%5D_Pixiv++/issues/Pixivv++#comment-15140bea129 with a trailing comma.

Moving this to later... @sizzlemctwizzle I would suggest not implementing notifications until this is finalized... there's a bunch of work to do with some research before we can implement that feature correctly.

Martii added a commit to Martii/OpenUserJS.org that referenced this issue Oct 10, 2017
* Use a pseudo negative floating look behind since re in V8 still doesn't support this yet

NOTE(S):
* Sanitizing first to get rid of any unclosed tags e.g. this is a post op
* At some point the prior fix stopped working from OpenUserJS#840
* There are still some users missed since other rendering is done first before any post op is applied... those should be filtered at some point.

Applies to OpenUserJS#735
Martii added a commit that referenced this issue Oct 10, 2017
* Use a pseudo negative floating look behind since re in V8 still doesn't support this yet

NOTE(S):
* Sanitizing first to get rid of any unclosed tags e.g. this is a post op
* At some point the prior fix stopped working from #840
* There are still some users missed since other rendering is done first before any post op is applied... those should be filtered at some point.

Applies to #735

Auto-merge
@Martii
Copy link
Member Author

Martii commented Oct 10, 2017

Ref:

Martii added a commit to Martii/OpenUserJS.org that referenced this issue Oct 11, 2017
* Use a script **disabled** virtual DOM for *node* to autolink in order to find parent if code/pre/a tags e.g. don't link on those but otherwise do
* This still doesn't cover every username but it's more than OpenUserJS#1161

NOTE(S):
* Tried using their fragment creation methods however `createElement` was absent during tests

Applies to OpenUserJS#735
Martii added a commit that referenced this issue Oct 11, 2017
* Use a script **disabled** virtual DOM for *node* to autolink in order to find parent if code/pre/a tags e.g. don't link on those but otherwise do
* This still doesn't cover every username but it's more than #1161

NOTE(S):
* Tried using their fragment creation methods however `createElement` was absent during tests

Applies to #735

Auto-merge
@Martii Martii removed later A long time ahead, in a galaxy near, near... question A question has been encountered by anyone and has remained unanswered until cleared. labels Oct 23, 2017
@Martii
Copy link
Member Author

Martii commented Oct 23, 2017

Moving this to GH dev discussion.

Currently @_foo_ and @~foo~ is an issue still.

I think what should happen is:

  1. Sanitize the content
  2. Replace the @mentions
  3. Render with marked

... or ...

  1. Replace the @mentions
  2. Render with marked
  3. Sanitize the content

Currently it:

  1. Renders with marked
  2. Sanitizes the content
  3. Replaces the @mentions

Notes:

  • Be careful not to bust rendering... I seem to recall there was a prior issue on order. This concept will need to be thoroughly tested.

Refs:

  • Current code point at
    // Sanitize first to close any tags
    var sanitized = sanitize(marked.Renderer.prototype[aType].apply(renderer, arguments));
    // Autolink most usernames

Martii added a commit to Martii/OpenUserJS.org that referenced this issue Apr 18, 2018
* Clean up the extra spaces some

Applies indirectly to OpenUserJS#735
Martii added a commit that referenced this issue Apr 18, 2018
* Clean up the extra spaces some

Applies indirectly to #735 

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Aug 19, 2019
* Create system reserved role type
* Bump DB/project version as this is not backwards compatible and a breaking change
* Amend TOS for an explicit instead of implied reserved right for renaming access. This is useful to notify the author including possible account name change requests.
* When OUJS needs to reserve a future (or current) specific name to continue the Sites integrity, especially from "bad actors", this is useful to have for a reservation with rename. Perhaps useful later on as well for some additional feature. These instance should be rare but declare none-the-less.

Post OpenUserJS#1137 OpenUserJS#735 *(inverse)*
@Martii Martii mentioned this issue Aug 19, 2019
Martii added a commit that referenced this issue Aug 19, 2019
* Create system reserved role type
* Bump DB/project version as this is not backwards compatible and a breaking change
* Amend TOS for an explicit instead of implied reserved right for renaming access. This is useful to notify the author including possible account name change requests.
* When OUJS needs to reserve a future (or current) specific name to continue the Sites integrity, especially from "bad actors", this is useful to have for a reservation with rename. Perhaps useful later on as well for some additional feature. These instance should be rare but declare none-the-less.

Post #1137 #735 *(inverse)*

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Nov 18, 2023
* Also minimize JSDOM initialization since we don't currently use the full `jsdom` identifier.
* Remove silence from 10d41e7
* New dep to restore highlighting with this version of *marked*
* DOM and Server side retested on FAQ/Script Info/ Dev OpenUserJS#735 local discussion

NOTE: Seems kind of "sucky" that highlighting was taken out this way as it looks like highlighting won't have the possibility of easily happening in the DOM.
@Martii Martii mentioned this issue Nov 18, 2023
Martii added a commit that referenced this issue Nov 18, 2023
* Also minimize JSDOM initialization since we don't currently use the full `jsdom` identifier.
* Remove silence from 10d41e7
* New dep to restore highlighting with this version of *marked*
* DOM and Server side retested on FAQ/Script Info/ Dev #735 local discussion

NOTE: Seems kind of "sucky" that highlighting was taken out this way as it looks like highlighting won't have the possibility of easily happening in the DOM.

Auto-merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug You've guessed it... this means a bug is reported.
Development

No branches or pull requests

2 participants