Page MenuHomePhabricator

improve/fix CentralNotice style loading
Closed, ResolvedPublic

Description

Author: herd

Description:
Currently CentralNotice drops a <style> tag in via "document.writeln($encShowStyle);" from an offsite script. In some older browser engines this can arbitrarily drop the style tag almost anywhere in the dom (including the textarea). A document.writeln() should not be performed via offsite script due to the unpredictablility of exactly when it will execute. This should be changed to an appendCSS() call (a core feature in wikibits.js since 1.13, and using proper DOM methods) or such.

Also, is this CSS strictly necessary if the notice has been dismissed or has been overwritten locally? Can this appendCSS call be moved inside the "if(wgNotice != '')" scope below?


Version: unspecified
Severity: trivial

Details

Reference
bz19704

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:42 PM
bzimport set Reference to bz19704.

There's no ambiguity here; per HTML spec, the <script> must be executed before any further processing of following HTML source.

herd wrote:

(In reply to comment #1)

There's no ambiguity here; per HTML spec, the <script> must be executed before
any further processing of following HTML source.

Fine, but the second point of the bug is still valid. There is no need to generate the CSS if the notice is dismissed.

There are several issues rolled up into this "bug":

  1. Dislike for use of document.write() based on rumor of instability:

There's no ambiguity in behavior when this is used by synchronously-loaded scripts as in current CentralNotice. Only browser reported to have problems is a 6-year old pre-alpha of Mozilla Firebird, and can be quite freshly ignored.

  1. The text is also inserted via document.write(), which there's no other general complaint about here... There is however in bug 11746 which appears to have been (mistakenly?) resolved as FIXED.

Adding bug 11746 as a dependency; document.write() is incompatible with the wiki being served out as application/xhtml+xml.

  1. Request not to append styles unnecessarily when the item is dismissed.

Collapse/expand or dissmiss/show behavior in CentralNotice are actually *produced* via CSS; there's no inherent notion of a dismissed CentralNotice. (In fact it's highly discouraged; all messages should have a way to return to them, which is why it's designed to support collapsing/expanding primarily.)

Not showing the CSS would simply result in the message being always shown.

  1. The insertion of <style> into the document body instead of the header where it belongs in theory. This wasn't specifically mentioned as a reason, but would be resolved if doing an appendCss() call as requested.

Note however that there are two style blocks relevant:
4a) The setup for hiding/showing of components based on cookie state
4b) Notice campaign-specific CSS, defined as a <style> chunk in the campaign template

The latter is not produced by the software, and would continue to be inserted via document.write() (or else via something like innerHTML, but still in the wrong place) unless CentralNotice were changed to have separate HTML body and style sections for each campaign notice.

I should add another:

  1. Loading the notice state via a synchronous inline <script> delays page rendering a bit, which is a bit annoying. If the way things are inserted is changed to be friendly to a synchronous load, we could let the browser render the page more quickly.

On the downside, this could mean the page "jumps" when a banner gets inserted afterwards.

*** Bug 18250 has been marked as a duplicate of this bug. ***

The document.writeln($encShowStyle) got dropped a few months ago. This should be fixed now.