Closed Bug 483840 Opened 13 years ago Closed 11 years ago

Rewrite SHOWFOR plugin to something more elegant

Categories

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

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: mconnor, Assigned: jsocol)

References

Details

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.
Seconded. 

The way SHOWFOR works now is ugly, and it will only get worse as we add to it.
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...
Blocks: 422473
(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. ;)
OS: Mac OS X → All
Hardware: x86 → All
Summary: SHOWFOR implementation is awful → Rewrite SHOWFOR plugin to something more elegant
I probably won't be bored for a while, but I think the benefits are fairly useful...
Blocks: 414512
Blocks: 437543
Blocks: 467552
Assignee: nobody → jsocol
We're going to reconfigure the wikiplugin for Fennec in the very short term, and work on rewriting this to use jQuery shortly.
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.
Target Milestone: --- → 1.4.1
Target Milestone: 1.4.1 → Future
Fixed in Kitsune.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
No longer blocks: 414512
You need to log in before you can comment on or make changes to this bug.