483840 - Rewrite SHOWFOR plugin to something more elegant

Status: RESOLVED FIXED

(support.mozilla.org :: Knowledge Base Software, task)

Description

[Mike Connor [:mconnor]]

Problems just from looking at output:

1) seemingly relies on generating custom script per-page (fragile, probably unnecessary with a different impl)
2) code generated is pretty bad/inefficient (view source on http://support.mozilla.com/en-US/kb/keyboard+shortcuts for details, or just watch the lag when switching OSes).
3) relies on directly setting/removing "display: none" on _every_ span, instead of using descendant selectors
4) uses string matching on class attribute, via a bunch of if statements.
4) generated script is inline, and thus has to be sent+parsed on every pageload

What I'd like to see (hell, I might even do it if I get bored):

1) Change the impl to use descendant selectors from an attribute on a div:

i.e. div.bodybox[os="windows"] > * > span.noWin { display: none }

2) set defaults in the template (i.e. <div class="bodybox" os="windows" version="3.1">)

3) showfor() just sets the attributes

This also means that the initial pageload isn't ugly/jarring for the Windows users on current version, we default to showing current Windows version, and switch if detection shows something else.

Key benefits:

1) Smaller pageload, no inline script needed, can ideally remove all of the inline style attrs
2) Less load on the server from generating custom scripts
3) Faster switching, making the non-default case faster too. (CSS re-resolving of styles is faster than setting attribute values on every span element)
4) Fixes, as a nice byproduct, what users with JS disabled will see.
5) CSS-based approach should make it easier to fix bug 463184, since you can reuse the styles in the print stylesheets.

Comment 1

[E. Cooper [:ecooper]]

Seconded. 

The way SHOWFOR works now is ugly, and it will only get worse as we add to it.

Comment 2

[Nelson Ko [:nkoth]]

The CSS manipulation approach was avoided due to my unfamiliarity with it more
than anything else (augmented with a fear of it working on one browser and not
another, or needing different implementations for different browsers). So I
decided on using the more ancient technology which turned out "awful" I suppose.

CSS re-resolving of styles might well be faster than setting attribute values
on every span element (even though I guessed it could be slower if there are
only a couple of elements needing direct attribute change). I also thought that
CSS manipulation could also be more memory intensive. However, I am not
familiar enough with the CSS manipulation approach to say. Anyway, I agree
it is the "cleaner" approach.

http://support.mozilla.com/en-US/kb/keyboard+shortcuts is an extreme example
because the number of elements there is very many...

Comment 3

[David Tenser [:djst]]

(In reply to comment #0)
> What I'd like to see (hell, I might even do it if I get bored):

By all means take this bug; seems like you have a very good idea of how this should work. Just need to make you bored, I suppose. ;)

Comment 4

[Mike Connor [:mconnor]]

I probably won't be bored for a while, but I think the benefits are fairly useful...

Comment 5

[James Socol [:jsocol, :james]]

We're going to reconfigure the wikiplugin for Fennec in the very short term, and work on rewriting this to use jQuery shortly.

Comment 6

[Chris Ilias [:cilias]]

I'm probably going to get yelled at for targeting this for 1.4.1; but I want to include this in the triage, so we know where it fits in the roadmap.

Comment 7

[James Socol [:jsocol, :james]]

Fixed in Kitsune.