Closed
Bug 483840
Opened 16 years ago
Closed 14 years ago
Rewrite SHOWFOR plugin to something more elegant
Categories
(support.mozilla.org :: Knowledge Base Software, task)
support.mozilla.org
Knowledge Base Software
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.
Comment 1•16 years ago
|
||
Seconded.
The way SHOWFOR works now is ugly, and it will only get worse as we add to it.
Comment 2•16 years ago
|
||
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•16 years ago
|
||
(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
Reporter | ||
Comment 4•16 years ago
|
||
I probably won't be bored for a while, but I think the benefits are fairly useful...
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → jsocol
Assignee | ||
Comment 5•15 years ago
|
||
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•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Target Milestone: 1.4.1 → Future
Assignee | ||
Comment 7•14 years ago
|
||
Fixed in Kitsune.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•