Page MenuHomePhabricator

DismissableSiteNotice uses document.write
Closed, ResolvedPublic

Description

This extension, deployed on all WMF properties is using document.write from an inlineScript. We should fix this.

Related: T107399

Event Timeline

TheDJ created this task.Aug 12 2015, 11:37 AM
TheDJ updated the task description. (Show Details)
TheDJ raised the priority of this task from to Needs Triage.
TheDJ added a project: DismissableSiteNotice.
TheDJ added a subscriber: TheDJ.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 12 2015, 11:37 AM
Nemo_bis triaged this task as High priority.Aug 18 2015, 5:12 PM
Nemo_bis added a subscriber: Nemo_bis.
Arkanosis added a subscriber: Jules78120.EditedDec 14 2015, 3:57 PM

Hi everyone,

@Jules78120 reported an issue with MediaWiki:AnonNotice on the French Wikipedia today, where the notice was added at the bottom left of the page and not at the top.

Could this be related to the use of document.write as well?

I've not experienced this issue with a fresh install of the 1.26 release with the DismissableSiteNotice extension I made last night. Edit: actually, I do.

0x010C added a subscriber: 0x010C.Dec 14 2015, 3:58 PM
Arkanosis set Security to None.Dec 14 2015, 3:58 PM
Arkanosis added a subscriber: Orlodrim.

Here is a screenshot of the problem, which doesn't always occurr

ashley added a subscriber: ashley.Dec 14 2015, 4:18 PM

For what it's worth...years ago, in r41679, I merged DismissableSiteNotice into core. Skizzerz improved it substantially in the follow-up revisions (r41706, r41711), making it work properly for browsers with JavaScript disabled and everything; usage of document.writeln was also removed in this latter revision (instead of document.writeln the current version of DismissableSiteNotice uses document.write).
These improvements were lost when the core merge was reverted in r41958 by Tim Starling. Maybe now would be an appropriate time to salvage some of those improvements and ditch document.write altogether?

For what it's worth...years ago, in r41679, I merged DismissableSiteNotice into core. Skizzerz improved it substantially in the follow-up revisions (r41706, r41711), making it work properly for browsers with JavaScript disabled and everything; usage of document.writeln was also removed in this latter revision (instead of document.writeln the current version of DismissableSiteNotice uses document.write).
These improvements were lost when the core merge was reverted in r41958 by Tim Starling. Maybe now would be an appropriate time to salvage some of those improvements and ditch document.write altogether?

I agree that the sitenotice feature should be dismissable without having to install an extension. Wasn't there an issue with the non-js version that made search engines index the sitenotice?

I guess the “quick fix” for now is to replace the current

$notice = Html::inlineScript( Xml::encodeJsCall( 'document.write', array( $notice ) ) );

with something like

$notice = Html::inlineScript( Xml::encodeJsCall( '$('#content').prepend', array( $notice ) ) );

(not quite sure about how encodeJsCall works).

See here for the code.

@Jules78120 reported an issue with MediaWiki:AnonNotice on the French Wikipedia today, where the notice was added at the bottom left of the page and not at the top.

Could this be related to the use of document.write as well?

This probably is related.

I've not experienced this issue with a fresh install of the 1.26 release with the DismissableSiteNotice extension I made last night.

Whether the site notice ends up at the bottom of the page would depend on whether the "mediawiki.legacy.wikibits" module, which overrides document.write, is executed before the embedded script containing the document.write() call.

This comment was removed by Vatadoshu.
This comment was removed by Jules78120.

I've not experienced this issue with a fresh install of the 1.26 release with the DismissableSiteNotice extension I made last night.

Err: actually, I do.

I guess the “quick fix” for now is to replace the current

$notice = Html::inlineScript( Xml::encodeJsCall( 'document.write', array( $notice ) ) );

with something like

$notice = Html::inlineScript( Xml::encodeJsCall( '$('#content').prepend', array( $notice ) ) );

(not quite sure about how encodeJsCall works).

See here for the code.

Actually, this wouldn't work, since inlineScript puts the code right where the notice is, meaning that jQuery is not yet available. Putting the code at the end of the page might work, but this would cause the page to move after loading, which is bad™. So I start to believe that whatever happens with search engines, putting the content right in the HTML instead of relying on JavaScript might be the way to go.

Here are links to the commits @ashley was talking about:

Vatadoshu added a comment.EditedDec 18 2015, 11:23 PM

We need the banner for the WikiMOOC that will be launch the
22 february. The banner will probably be on wikipedia one month or 15 days before the launch (i don't know exactly). But it will be in few months.
early january says Jules. Registrations have started and we have failed to put the banner for the registrations . And we expect we can put it for the next time.
@Aklapper Do you think the priority could be review as "unbreak now" ?
@Jules78120 if you have more details on dates ...

Jules78120 added a comment.EditedDec 18 2015, 11:32 PM

We need the banner for the WikiMOOC that will be launch the 22 february. The banner will probably be on wikipedia one month or 15 days before the launch (i don't know exactly). But it will be in few months. Inscriptions have started and we have failed to put the banner for the inscriptions . And we expect we can put it for the next time.
@Aklapper Do you think the priority could be review as "unbreak now" ?
@Jules78120 if you have more details on dates ...

Our goal is to put the banner on Wikipedia from early January.

This banner is really very important: without it, the project (we are 15 Wikipedians working hard on this project since May 2015! More than 1000 hours of work...) will have much less impact... :(.

Because the banner didn't work when we tried to use it monday, we have for now only 200 enrolled, which is far less than what we expect. I hope you will be able to help us on that problem.

I hope you will be able to help us.

Nemo_bis added a comment.EditedDec 19 2015, 7:51 AM

Do you think the priority could be review as "unbreak now" ?

No, there is a workaround: use CentralNotice. https://meta.wikimedia.org/wiki/CentralNotice/Usage_guidelines

Please don't add more comments specific to your campaign here, instead ask help on https://meta.wikimedia.org/wiki/WM:RFH

Do you think the priority could be review as "unbreak now" ?

No, there is a workaround: use CentralNotice. https://meta.wikimedia.org/wiki/CentralNotice/Usage_guidelines

This workaround is an absolutely unacceptable one for any and all non-WMF sites. CentralNotice is extremely heavy and a lot more complicated to install than DismissableSiteNotice.

Based on some quick tests I ran locally, the current (git master) version of DismissableSiteNotice fails to position the notice correctly in about or over half of the cases. This becomes especially problematic if and when you have extensions like ShoutWiki Ads which somehow append things (in this case, a leaderboard ad) to the sitenotice area.

Change 261758 had a related patch set uploaded (by PleaseStand):
Remove code to hide the site notice from search engines

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

Change 261758 had a related patch set uploaded (by PleaseStand):
Remove code to hide the site notice from search engines

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

That's one of the main requirements for this extension, so if the code is removed a bug should be filed to reintroduce the functionality (insofar possible).

Florian closed this task as Resolved.Apr 22 2017, 2:50 PM
Florian assigned this task to Krinkle.

Change 261758 merged by jenkins-bot:
[mediawiki/extensions/DismissableSiteNotice@master] Update code that hides site notice from search engines

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