source code improvements/suggestions
ScottJehl
EnhanceJS that could be cleaned up a bit:
http://gist.github.com/348486
With his permission, I'm posting our email thread here, and I'm
logging issues in the tracker now for the points I'd like to address.
If anyone has ideas related to these observations, please chime in!
-Scott
--------------------------------------------------------------------------------------------------------------------
scottjehl said 2 days ago:
Hey,
Scott from EnhanceJS here.
I was just looking at the critique notes you posted on EnhanceJS.
Thanks for posting. I mostly agree with the points you noted, like the
className spaces and the undeclared assignments, etc. I'd love to tie
up any of these "sloppy" loose ends in the library, since overall it
does a lot to improve accessibility across browsers and we'd really
like to encourage wider adoption.
I wanted to clear up the purpose of the browser sniffing portion and
get your take on how it could be avoided. The IE UA sniff in there is
not used by EnhanceJS internally, but it's provided as a utility for
those trying to mimic appending scripts and styles through conditional
comments in IE. Basically, when calling enhance(), you can specify an
iecondition along with a stylesheet, like:
enhance({ addStyles: [{'href': 'css/iefixes.css', 'iecondition':
6}] });
and that'll end up using the UA detect to filter out whether that
stylesheet should be appended. Since EnhanceJS is designed to prevent
advanced CSS/Scripts from applying where they shouldn't, many
developers find this option necessary when fixing IE bugs.
That said, I hate having this code included in the library,
particularly given that its entire purpose is capability-test-driven-
PE (the irony is not lost). I considered this alternative, with no
luck so far: http://code.google.com/p/enhancejs/issues/detail?id=3
Any thoughts you have on helping us improve EnhanceJS would be greatly
appreciated.
Let me know if you have any thoughts on this.
Thanks again
Scott Jehl
--------------------------------------------------------------------------------------------------------------------
kangax said 1 day ago:
Hey Scott. Good to hear you're open to improving the script.
As far as sniffing for IE... yes, mimicking conditional comments via
user agent parsing is a pretty bad idea :) What I see in your issue #3
— using conditional compilation — is exactly what needs to be done.
Which problem did you have with it?
Undeclared assignments can be fixed easily; same for className/indexOf
bug. I see that you're attaching class to HTML element. Technically
(as per HTML 4.01 or even XHTML 1.0), HTML element can not have class
attribute (http://www.w3.org/TR/REC-html40/struct/global.html#edef-
HTML), so given that you're setting className which as per DOM L2 HTML
corresponds directly to "class" attribute (http://www.w3.org/TR/DOM-
Level-2-HTML/html.html#ID-95362176), this might very well not work in
some environments (mobile devices, older desktop browsers, etc.)...
that is if you want to play safe and script defensively :) On the
other hand, assigning className to document.documentElement could well
be a de-facto standard and work more or less reliably. Its just that
in my experience, playing "by the rules" is usually the safest way to
go, and if you have another—safer—option (say, adding class to body),
it'd go with that.
I also think that overall testing strategy of EnhanceJS is not as
defensive as it has to be. Host objects are tested with !! which
boolean-type-converts expression (and can easily blow up in IE, where
host objects with "unknown" type throw error on [[Get]]). Some things
are used before being tested for existence (style,
getElementsByTagName). Or did I miss those checks?
If I have some time in the future, I'll look at it some more.
And I'm really glad to see you guys talk/write about progressive
ehnancement. This is something I've been following and teaching about
for a while.
Best,
kangax
--------------------------------------------------------------------------------------------------------------------
scottjehl said 1 day ago:
Hi again,
Thanks so much for the feedback!
Your idea for using conditional compilation for the IE detect sounds
like a much better approach. I added a comment with a potential fix if
you're interested in checking it out: http://code.google.com/p/enhancejs/issues/detail?id=3
I'm hoping to implement several of the changes you suggest soon,
particularly the easy ones, like the undeclared assignments.
The HTML class is pretty useful in a number of situations, such as a
workaround that helps prevent a FOUC (which is documented in our new
book Designing with Progressive Enhancement), so it'd be nice to keep
it in there if we can. :) Assuming we keep it, do you think it would
be any better/safer to use set/getAttribute for this portion, under an
assumption that some browser might not define a className property on
documentElement? I'm not sure it'd make much of a difference but
thought you might have ideas.
As far as the host objects, I'd be interested in your ideas for how to
improve that part. Maybe that's even the reason we're seeing a couple
of errors in IE4. How do you recommend changing those? Should we check
that it's not "unknown" before trying to return it as a boolean?
Should we just return the object without converting it?
You're right that there are some objects in use that aren't tested for
existence beforehand. I'll file an issue for that so we can remember
to track them down.
Glad to hear you're pleased with what we're doing with PE.
Thanks again for chatting, it's been very helpful.
-Scott
ScottJehl
Issue 3: for pseudo IE conditional comment support, use conditional
compilation instead of UA string.
http://code.google.com/p/enhancejs/issues/detail?id=3
Issue 10: Undeclared assignments
http://code.google.com/p/enhancejs/issues/detail?id=10
Issue 11: type-converting host objects
http://code.google.com/p/enhancejs/issues/detail?id=11
Issue 12: add a test for className support on documentElement
http://code.google.com/p/enhancejs/issues/detail?id=12
Issue 13: some things are used before being tested for existence
http://code.google.com/p/enhancejs/issues/detail?id=13