Page MenuHomePhabricator

Phonos does not display correctly in Visual Editor
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
The HTML is displayed

What should have happened instead?:
Showing the rendered output instead of raw markup.

Software version (skip for WMF-hosted wikis like Wikipedia):

  • Deployed to Beta

Other information (browser name/version, screenshots, etc.):

image.png (268Γ—1 px, 62 KB)

Acceptance criteria

  • Since Parsoid doesn't support HTML parser functions (T279094), we should change Phonos to use a tag instead, ala <phonos ipa="[ipa]">[label]</phonos>. This will make it appear correctly in VisualEditor.
  • For now, we won't build a custom form for Phonos (i.e. when editing the tag in VisualEditor), but we may explore this in a separate ticket

Event Timeline

Any ideas, is this "just" a VisualEditor style issue?

The sanitizer in Parsoid seems to be escaping the <a> tag coming from the parser function.

[subbu@earth:~/work/wmf/parsoid] echo "{{#phonos: ipa=hΙ™ΛˆlΙ™ΚŠ | text=Hello | lang=en}}" | php bin/parse.php --apiURL https://en.wikipedia.beta.wmflabs.org/w/api.php --trace sanitizer
...
0-[SANITY]     | {"type":"TagTk","name":"a","attribs":[{"k":"role","v":"button","srcOffsets":[623,627,629,635],"vsrc":"button"},{"k":"tabindex","v":"0","srcOffsets":[637,645,647,648],"vsrc":"0"},{"k":"data-x-rel","v":"nofollow","srcOffsets":[650,653,655,663],"vsrc":"nofollow"},{"k":"class","v":"oo-ui-buttonElement-button","srcOffsets":[665,670,672,698],"vsrc":"oo-ui-buttonElement-button"}],"dataAttribs":{"tmp":null,"stx":"html"}}
0-[SANITY]     |  ---> "<a role='button' tabindex='0' data-x-rel='nofollow' class='oo-ui-buttonElement-button'>"
...

To be investigated.

I think this is a known issue: T279094 (and maybe T178588 is a duplicate of that?).

It might be resolved by the work on T268144.

Thanks @matmarex for refreshing my memory. :)

MusikAnimal changed the task status from Open to Stalled.Sep 6 2022, 11:22 PM
MusikAnimal removed MusikAnimal as the assignee of this task.
MusikAnimal subscribed.

T279094 indeed is our issue. Thanks everyone for the help!

One workaround is to change Phonos to be a tag extension instead of a parser function, i.e. with <phonos ipa="/hΙ™ΛˆvΓ¦nΙ™/" />. I tested this and it looks good in VisualEditor, and it even has the built-in attributes editor like you see with some other tags and templates (I don't think we want to bother building a custom Phonos editor for VE, since communities are most likely to put the parser function/tag in a template rather than use it directly). We almost switched to a tag before, now I'm wondering if we should as I suspect T279094/T268144 will take some time to be addressed.

I'm marking this stalled for now.

Change 830310 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/extensions/Phonos@master] [POC] Use tag instead of a parser function

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

Change 830310 merged by jenkins-bot:

[mediawiki/extensions/Phonos@master] Use tag instead of a parser function

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

Huh, I tested this locally with VE, worked as expected

image.png (362Γ—426 px, 10 KB)

But having just tested something on beta in prep for a deployment:

image.png (429Γ—935 px, 72 KB)


Ignore the above, it now appears correctly on Beta

MusikAnimal changed the task status from Stalled to Open.Sep 7 2022, 3:12 PM
MusikAnimal claimed this task.

As evidenced above, we've decided to go the route of changing Phonos to use a tag. Re-assigning, and I think this is ready for QA now?

To reiterate what was said on Slack, and just to be crystal clear on what's changing:

{{#phonos: ipa=[IPA notation] |text=[plain text] |lang=[language] |file=[filename] |label=[label] }}

is now:

<phonos ipa="[IPA notation]" text="[plain text]" lang="[language]" file="[filename]">[label]</phonos>

Note self-closing tags should also work if there's no label:

<phonos ipa="[IPA notation]" text="[plain text]" lang="[language]" file="[filename]" />

There are test cases of these in the parser tests which are all passing, so everything should work fine. I'm primarily just want to make sure QA knows the syntax changed, since that's going to effect all of your testing moving forward :)


As a side note, we can now make a customized form for Phonos rather than the automatic one created by VE. We need product input to determine priority on that, but in my opinion it is quite low (for now) since on the WMF farm most wikis use templates and would rarely add a <phonos> tag directly into pages. Either way this should be a different ticket.

MusikAnimal set the point value for this task to 3.Sep 7 2022, 3:22 PM

Tested it out on Patch Demo and there are no more issues showing the rendered output when selecting Visual Editor as seen in the screenshot.

Phonos_VE_Pass.png (577Γ—3 px, 109 KB)