Prev: FAQ Topic - What is JScript? (2010-07-16)
Next: How to find if statusbar is hidden or visible in firefox?
From: David Mark on 17 Jul 2010 14:18
On Jul 17, 9:55 am, RobG <rg...(a)iinet.net.au> wrote:
> On Jul 17, 12:01 pm, David Mark <dmark.cins...(a)gmail.com> wrote:
> [...]
>
> > Now where was I?  Ah yes, the conditional augmentation of native
> > prototypes.
>
> I think you meant built-in prototypes (those of Object, String, etc.).
> Native prototypes (i.e. those of constructors created using
> javascript) are the only ones that should be augmented.
>

Yes. I meant the prototypes of built-ins like Number, String, etc.
They didn't go so far as to augment Object.prototype, which would have
played havoc with their unfiltered for-in loops.
From: David Mark on 17 Jul 2010 21:25
On Jul 17, 6:28 am, Andrew Poulos <ap_p...(a)hotmail.com> wrote:

[...]

>
> It would be heartening to read something like "yes, you're right but
> we're working to improve".
>

Revisiting the rest of this thing, ISTM that they would have to first
learn a lot more about browser scripting and then rewrite the whole
thing from scratch. I mean, if there are glaring errors,
misconceptions and design missteps in virtually every function, it
indicates that most proficient of those involved is in over their
head.

/**
* Normalizes currentStyle and computedStyle.

No it doesn't, The currentStyle bit was cropped out as it is only
needed for IE.

* @param {String} property The style property whose value is
returned.
* @return {String} The current value of the style property for
this element.
*/
getStyle: function(prop) {
var dom = this.dom,
value,
computedStyle,
result,
display;

if (dom == document) {
return null;
}

That's ridiculous. If the application passes a reference to a
document node to this function, the application has a problem (which
needs to be tracked down). The null return value is needed to
represent other (more expected) calamities (like elements with a
display style of "none" or any number of possibilities in IE).

prop = Ext.Element.normalize(prop);

result = (value = dom.style[prop]) ? value : (computedStyle =
window.getComputedStyle(dom, null)) ? computedStyle[prop] : null;

First off, you don't use the inline style, except as a last resort.
It may well be something like "30%" or "3em" (according to the above
comments this function seeks to "normalize" computed/current styles).

Second, using window.getComputedStyle is simply wrong. The method is
to be found on the document.defaultView object, which by coincidence
often references the window object.

Third the "concise" nested ternary operations with assignments is a
pattern only a "Jorge" could love.

Instead:-

computedStyle = document.defaultView.getComputedStyle(dom, null));

if (computedStyle) {
computedStyle = computedStyle[prop];
}

if (!computedStyle) {
computedStyle = dom.style[prop] || null;
}

Of course, the last bit isn't really needed as it won't come into play
unless the element has a display style of "none" (in which case the
application developer has made a mistake).

// Fix bug caused by this: https://bugs.webkit.org/show_bug.cgi?id=13343

Yes, WebKit has at least a couple of problems with right margins.

if (Ext.platform.hasRightMarginBug && marginRightRe.test(prop)
&& out != '0px') {

That flag (hasRightMarginBug) is clearly from ExtJS (and almost
certainly set by a UA sniff). It is not set anywhere in this script.

This is the sort of issue that is to be designed around. Cross-
browser designs based on retrieving accurate computed style values are
flawed from the start. This is especially true for margins, which may
be collapsed (though not the case for horizontal margins). To
determine a horizontal (left/right) margin in pixels, I imagine I
would set the appropriate style to "0" and check if the offsetLeft
changed. Personally, I've never had a cause to do this in any
context.

Of course for an ostensibly cross-browser, GP framework, you can't
design around anything. You've got to try to cram in every solution
to every problem to suit every context (which is inherently
inappropriate for software that must be downloaded).

These are the hardest scripts to write and virtually impossible to get
100% correct. This is why only newcomers attempt them, making the
resulting failures easily predictable. What's troubling is that the
starry-eyed neophytes typically refuse to admit defeat, instead opting
to hide their unrealized designs behind browser sniffing (and
asserting not to care about browsers, old, unknown or yet to be
created that break such ill-advised inferences). That's why the
notion of users changing their UA strings makes them so
uncomfortable. If only they realized that it is the browser sniffing
scripts that force users to do so (i.e. they are the problem, not the
savvy users).

http://www.jibbering.com/faq/notes/detect-browser/

display = this.getStyle('display');

No good. The correct line for this occasion is:-

display = el.style.display;

el.style.display = 'inline-block';
result = view.getComputedStyle(el, '');
el.style.display = display;

See what I mean? Imagine if the inline display style is not specified
(fairly typical). And consider that this script uses classes to hide
elements. If this code were ever executed, there's a good possibility
that an inline style will be set (where none existed before) and the
element will rebuff all future attempts by the framework to hide it.
Alternatively, that last line may end up trying to set an inline style
of "null" (good for a warning or exception, depending on the browser).

}

// Webkit returns rgb values for transparent.

So what? The script only endeavors to support WebKit. As for
normalization, the rgb format is far more useful for a script as it
matches the other umpteen billion color/opacity combinations.

if (result == 'rgba(0, 0, 0, 0)') {
result = 'transparent';
}

return result;
},

/**
* Wrapper for setting style properties, also takes single object
parameter of multiple styles.
* @param {String/Object} property The style property to be set,
or an object of multiple styles.
* @param {String} value (optional) The value to apply to the
given property, or null if an object was passed.
* @return {Ext.Element} this
*/
setStyle: function(prop, value) {
var tmp,
style;

if (!Ext.isObject(prop)) {

Again, try this:-

if (typeof prop == 'string') {

No function call, no property lookup and no negative assertion. Use a
"constant" and it will minify to something like:-

if(typeof a==b){

....which is shorter than:-

if(!Ext.isObject(a)){

tmp = {};
tmp[prop] = value;
prop = tmp;
}

for (style in prop) {

Forgot the filter again.

value = prop[style];
style = Ext.Element.normalize(style);
this.dom.style[style] = value;

That's a lot of extra work each time through (glohal identifier
resolution and property lookups). Each function call opens the door
for a reflow/repaint as well.

}

return this;
},

/**
* <p>Returns the dimensions of the element available to lay
content out in.<p>

Available to lay content out in?

* <p>If the element (or any ancestor element) has CSS style
<code>display : none</code>, the dimensions will be zero.</p>
*/
getViewSize: function() {
var doc = document,
dom = this.dom;

if (dom == doc || dom == doc.body) {
return {
width: Ext.Element.getViewportWidth(),
height: Ext.Element.getViewportHeight()
};
}

Ridiculous. The client dimensions of the body have nothing to do with
the viewport dimensions (except in IE5 and IE quirks mode). Some
browsers in quirks mode (stupidly) store the clientHeight/Width of the
HTML element in those properties on the BODY in a botched attempt to
copy IE's quirks mode implementation. Yes, really. :)

http://www.cinsoft.net/viewport.asp

So here we have library authors misinterpreting a misinterpretation of
a ten-year-old MS standard.

Given how screwed up the non-IE browsers are in this respect, if you
really had a need to find the client dimensions of the BODY (hard to
imagine), you would need to subtract the border dimensions from its
offsetHeight/Width.

else {
return {
width: dom.clientWidth,
height: dom.clientHeight
};
}
},

So this method retrieves the clientHeight/Width of an element, except
for the BODY which returns window.innerHeight/Width. I suppose with
the Ext CSS reset in place, that might just pass superficial testing,
despite being completely wrong. This is why I dismiss the "show me
where it fails" mentality. If you don't understand what you are
doing, there is always the possibility of guessing and stumbling on to
an "answer" that appears to work in *some* cases (see also "we don't
care about other cases").

/**
* Forces the browser to repaint this element

Browsers repaint elements when they are damned good and ready. The
presence of a function like this indicates an attempt to solve a
problem created elsewhere in the script (and likely due to a
misunderstanding of how/when browsers reflow/repaint content). And
this is the second such function encountered in this script.

* @return {Ext.Element} this
*/
repaint: function() {
var dom = this.dom;
this.addClass("x-repaint");
dom.style.background = 'transparent none';
setTimeout(function() {
dom.style.background = null;

Dear Christ. They actually set a style to null *on purpose* (earlier
it was pointed out that they left the door open for this to happen by
accident).

Am I being to hard on them? Remember, they intend to charge money for
this tripe (and have already used it to induce investors to fork over
millions). It's well-known that jQuery (and especially its plug-ins)
are awful, but I'd sooner use those. At least they are free.

Ext.get(dom).removeClass("x-repaint");
},
1);
return this;
},

/**
* Retrieves the width of the element accounting for the left and
right
* margins.
*/
getOuterWidth: function() {
return this.getWidth() + this.getMargin('lr');
},

How about:-

return this.dom.offsetWidth + ...

Function calls are *expensive* and mobile devices don't have resources
to burn.

// private

Nope. :)

sumStyles: function(sides, styles) {
var val = 0,
m = sides.match(/\w/g),

Why not just split on spaces?

len = m.length,
s,
i;

for (i = 0; i < len; i++) {
s = m[i] && parseInt(this.getStyle(styles[m[i]]), 10);

As mentioned previously, this can result in a round-off error. Use
parseFloat.

if (s) {
val += Math.abs(s);
}

I guess they don't like negative margins. :)

}
return val;
}

And so on and so on (and scooby dooby dooby).

Skipping past more everyday code to try to find something close to
resembling HTML5...

if (Ext.platform.isPhone) {
cfg = {
tag: 'embed',
type: 'audio/mpeg',
target: 'myself',
src: this.src || this.url,
controls: 'true'
};
} else {
cfg = {
tag: 'audio',
src: this.src || this.url
};
}

That represents virtually the entire "HTML5" audio bit. This is
Ext.platform.isPhone:-

isPhone: /android|iphone/i.test(Ext.userAgent) && !(/ipad/
i.test(Ext.userAgent)),

As mentioned, this thing only *attempts* to work in two types of
phones (90% of the market according to some unidentified study) plus
one tablet. Still, the above code assumes that all iPhones/Androids
(past, present and future) are better off with a non-standard EMBED
element. My guess is they didn't like the way AUDIO elements take up
the entire screen when played on these devices (but that's what users
expect them to do).

This unfortunate "isPhone" heuristic is also used to style the body.

if (Ext.platform.isPhone) {
cls.push('x-phone');
}

Who needs media queries?

I don't see any reason to mince words. This thing stinks. You'd have
to be insane to use it on a mobile Website. Using it to create a faux
native app is ill-advised as well. The "Sencha" name got the green
part right, but this is almost (but not entirely) unlike tea. :)
From: Garrett Smith on 18 Jul 2010 02:18
On 2010-07-17 06:25 PM, David Mark wrote:
> On Jul 17, 6:28 am, Andrew Poulos<ap_p...(a)hotmail.com> wrote:
>
> [...]
>
>>
>> It would be heartening to read something like "yes, you're right but
>> we're working to improve".
>>
>
> Revisiting the rest of this thing, ISTM that they would have to first
> learn a lot more about browser scripting and then rewrite the whole
> thing from scratch. I mean, if there are glaring errors,
> misconceptions and design missteps in virtually every function, it
> indicates that most proficient of those involved is in over their
> head.
>

[...]

>
> /**
> * Forces the browser to repaint this element
>

The name "repaint" has the outward indication of a mystical incantation.
We know that there is no way to force a repaint, and so when I see a
name like that, I know that the method won't directly cause the browser
to repaint and that it will, at best, perform an unrelated action that
was associated with addressing a perceived problem.

> Browsers repaint elements when they are damned good and ready. The
> presence of a function like this indicates an attempt to solve a
> problem created elsewhere in the script (and likely due to a
> misunderstanding of how/when browsers reflow/repaint content). And
> this is the second such function encountered in this script.
>


> * @return {Ext.Element} this
> */
> repaint: function() {
> var dom = this.dom;
> this.addClass("x-repaint");
> dom.style.background = 'transparent none';
> setTimeout(function() {
> dom.style.background = null;
>

Neither null nor "null" are a valid value for background.

DOM 2 Style defines what must happen when the display property is set:
<http://www.w3.org/TR/1998/REC-CSS2-19980512/visuren.html#propdef-display>

| display of type DOMString
| See the /_background property definition_/ in CSS2.
| Exceptions on setting
| DOMException
|
|
| SYNTAX_ERR: Raised if the new value has a syntax error and is
| unparsable.

That says that a SYNTAX_ERR is if the new value is unparseable, and to
determine if the value `null` is parseable (it isn't, but just in case
that was not blatantly obvious), the reader is directed to the
definition in CSS 2 which is linked from the text "/_background property
definition_/" links to DOM 2 Style:
<http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSS2Properties-background>

Looking at CSS2:
| 'background'
| Value: [<'background-color'> || <'background-image'> ||
| <'background-repeat'> || <'background-attachment'> || <'background-
| position'>] | inherit

And so it is clear that neither null nor "null" are valid property
values. Setting `element.style.display = null`, if the value `null` is
interpreted as the string value "null", then it should raise a SYNTAX_ERR.

That error will happen in IE but since IE is not supported they don't
care. That exception won't happen in webkit so they get away with it there.

[...]

> native app is ill-advised as well. The "Sencha" name got the green
> part right,

That's actually funny.

Sencha, or Ext for TouchScreen devices, supports only a couple devices,
and at a nearly 500k, what does it do?

One might look to the documentation but the documentation is for Ext-JS
and the code for Sencha is largely different.

As various parts of Sencha have been discussed, the query engine has not
yet been mentioned. Of all of the javascript query engines I have
reviewed, Ext-JS is hands down, the worst (and by a very large margin).
The problems with the query engine depend on the version of Ext-JS go
back and forth depending on the version of Ext. The latest version uses
NFD (native first, dual) approach. I'll detail more on these, as well as
the NFD query antipattern, in a future post. For now, lets have a look
at the query engine for Sencha.

The current release of Sencha's query selector uses only
document.querySelectorAll, and supports none of the extensions that are
supported by other version of Ext-JS. Since Sencha splits input on ","
first, it breaks on any selector that contains a comma, such as
[title="Hey, Joe"].

These problems in the source code should be easy for anyone to see.

Splitting input on "," creates two significant problems, actually.

The first problem is that it creates invalid selectors out of any
perfectly valid selector that contains a comma. For example, Sencha will
convert the selector '[title="Hey, Joe"]' to '[title="Hey,' and ' Joe"],
both of which are invalid. Sencha then passes each item to
document.querySelectorAll which throws an uncaught error on the invalid
'[title="Hey,' selector.

The second problem is that when it "works", the result can contain
duplicates.

For example, for a document that contains three divs:

<body>
<div></div> <div></div> <div></div>
</body>

The following code:
[Ext.query("div").length , Ext.query("div, div").length]

- results [3, 6]

My code comments below are annotated with (GS), as in
// (GS) A review comment left by Garrett Smith.

Code review begins:

/**
* Selects a group of elements.
* @param {String} selector The selector/xpath query (can be a comma
* separated list of selectors)
* @param {Node/String} root (optional) The start of the query
* (defaults to document).
* @return {Array} An Array of DOM elements which match the selector.
* If there are no matches, and empty Array is returned.
*/
select : function(q, root) {
var results = [],
nodes,
i,
j,
qlen,
nlen;

root = root || document;
if (typeof root == 'string') {
// (GS) This call may return null.
root = document.getElementById(root);
}

// (GS) splitting the input on "," will break lexical rules and
// (GS) will match any simple selector that has a comma in it.
q = q.split(",");
for (i = 0, qlen = q.length; i < qlen; i++) {
if (typeof q[i] == 'string') {
// (GS) If root is null, an error will result.
nodes = root.querySelectorAll(q[i]);

for (j = 0, nlen = nodes.length; j < nlen; j++) {
results.push(nodes[j]);
}
}
}
return results;
},

As seen in the results, selectors that are separated by a comma are
added to the results. After reading the code, it is clear that the code
does exactly what should be expected of it.

There is no apparent reason for such design decision; the results are
surprising and probably undesirable for most. This appears to be a yet
another careless mistake and one that adds the overhead of creating an
array and looping through the result.

Misleading Comment
The code comment documents the selector param as a selector/xpath query.
That is misleading because the code is not designed to handle XPath
expressions. Method document.querySelectorAll uses CSS Selectors. It
will throw errors on any invalid syntax. There are a couple syntax
similiarties between XPath and CSS Selectors, however the differences
between the two are much greater. XPath expressions and CSS Selectors
are represented by different standards.

Since Sencha uses document.querySelectorAll, it can be assured that an
error will occur when using XPath. Ext.query("//foo"); will result in an
error being thrown.

Bigger is Better?
At nearly 500k, with comments stripped, the core of Sencha is larger
than jQuery and YUI combined. The drawbacks to such a large codebase on
mobile device that has a web browser include: slow download times, extra
cost to end user (depends on plan, connection and roaming charges),
memory and processing overhead of interpreting the javascript and
keeping it in memory, and filling or exceed a browser's cache-limit.

Despite the size, Sencha is only supported on a few browsers and only on
those browsers that have native NodeSelector support. Despite those
limitations, it still manages to introduce more bugs than the browsers
provide.

Other Browsers
Sencha is apparently not intended to be used on any version of Internet
Explorer. By including sencha js on a page, errors are thrown in
Internet Explorer due to calling document.addEventListener.
--
Garrett
From: kangax on 18 Jul 2010 09:10
On 7/18/10 2:18 AM, Garrett Smith wrote:
> On 2010-07-17 06:25 PM, David Mark wrote:
[...]
>>
>> /**
>> * Forces the browser to repaint this element
>>
>
> The name "repaint" has the outward indication of a mystical incantation.
> We know that there is no way to force a repaint, and so when I see a
> name like that, I know that the method won't directly cause the browser
> to repaint and that it will, at best, perform an unrelated action that
> was associated with addressing a perceived problem.

I'm not sure where you coming from with this.

There certainly are observable ways to trigger both � reflow and
repaint; at least in WebKit (as that's the layout engine being discussed
here).

In Chrome you can use Speed Tracer
(<http://code.google.com/webtoolkit/speedtracer/>, developed by Google
themselves, IIRC) to take a peek into when browser reflows the document
and when it repaints the screen.

Try something like this, to see both � reflow and repaing hapenning at
~3085ms:

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<title></title>
<style type="text/css">
#test { width: 100px; background: red; }
</style>
</head>
<body>
<div id="test">test</div>
<script type="text/javascript">
setTimeout(function() {
document.styleSheets[0].addRule('#test', 'width: 200px');
}, 3000);
</script>
</body>
</html>

[...]

Or am I missing something?

--
kangax
From: David Mark on 18 Jul 2010 13:32
On Jul 18, 9:10 am, kangax <kan...(a)gmail.com> wrote:
> On 7/18/10 2:18 AM, Garrett Smith wrote:
>
> > On 2010-07-17 06:25 PM, David Mark wrote:
> [...]
>
> >> /**
> >> * Forces the browser to repaint this element
>
> > The name "repaint" has the outward indication of a mystical incantation..
> > We know that there is no way to force a repaint, and so when I see a
> > name like that, I know that the method won't directly cause the browser
> > to repaint and that it will, at best, perform an unrelated action that
> > was associated with addressing a perceived problem.
>
> I'm not sure where you coming from with this.
>
> There certainly are observable ways to trigger both — reflow and
> repaint; at least in WebKit (as that's the layout engine being discussed
> here).

It's certainly fairly predictable.

>
> In Chrome you can use Speed Tracer
> (<http://code.google.com/webtoolkit/speedtracer/>, developed by Google
> themselves, IIRC) to take a peek into when browser reflows the document
> and when it repaints the screen.

It should be noted that is but one flavor of WebKit. It's a desktop
version as well, so may behave in a slightly different manner than
mobile variations.

>
> Try something like this, to see both — reflow and repaing hapenning at
> ~3085ms:

Well sure changing the style rules at 3000ms will cause the browser to
reflow/repaint at some time shortly after 3000ms from current. At
least under normal circumstances.

>
> <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
>    "http://www.w3.org/TR/html4/strict.dtd">
> <html>
>    <head>
>      <title></title>
>      <style type="text/css">
>        #test { width: 100px; background: red; }
>      </style>
>    </head>
>    <body>
>      <div id="test">test</div>
>      <script type="text/javascript">
>        setTimeout(function() {
>          document.styleSheets[0].addRule('#test', 'width: 200px');
>        }, 3000);
>      </script>
>    </body>
> </html>
>
> [...]
>
> Or am I missing something?
>

Yes; the code in question seems to assert that it can make the browser
reflow/repaint at a specific point in the execution. And it doesn't
change the style rules, but does the styling equivalent of a no-op.
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5 6 7 8 9 10 11 12 13
Prev: FAQ Topic - What is JScript? (2010-07-16)
Next: How to find if statusbar is hidden or visible in firefox?