RTL and offsetLeft in IE 6

[Andrew Poulos]

I have a DIV sized with CSS and its display is inline:

Nerk is the <div id="target0">&nbsp;</div>.

Other DIVs of the same size get positioned exactly over the top of it.

I use code based on offsetLeft and offsetTop (that people here helped me
with) to get its position.

All works well when the text direction is set to 'left to right' (LTR)
but if its 'right to left' (RTL) IE 6 thinks the page is LTR and
offsetLeft for the DIV ends being to a mirror position. That is, its way
off.

How do you get IE 6 to behave?

Andrew Poulos

 

[Garrett Smith]

I have a DIV sized with CSS and its display is inline:

Nerk is the <div id="target0">&nbsp;</div>.

Other DIVs of the same size get positioned exactly over the top of it.

I use code based on offsetLeft and offsetTop (that people here helped me
with) to get its position.

All works well when the text direction is set to 'left to right' (LTR)
but if its 'right to left' (RTL) IE 6 thinks the page is LTR and
offsetLeft for the DIV ends being to a mirror position. That is, its way
off.

I've not tried using RTL before, so have not experienced your problem.
It would be helpful if you could post a simplified example that
demonstrates the problem.

How do you get IE 6 to behave?

Method element.getBoundingClientRect.

See also:
http://msdn.microsoft.com/en-us/library/ms536433(VS.85).aspx
https://developer.mozilla.org/En/DOM/Element.getBoundingClientRect

<aside>
The MDC documentation links to CSSOM draft, replete with misgivings and
errors pointed out in years past. Unfortunately MSIE has included some
of those in Internet Explorer 8.

Making official-looking documentation can seem to have drastic effects,
even when it says "working draft" at the top of it.
</aside>

Garrett

 

[Andrew Poulos]

I've not tried using RTL before, so have not experienced your problem.
It would be helpful if you could post a simplified example that
demonstrates the problem.


Method element.getBoundingClientRect.

I tried it and it also gives incorrect results.

I've uploaded a simple page here:
<url: http://members.iinet.net.au/~apoulos/rtl.htm >
that displays the issue.

You need to view the page with IE 6 or 7 as I'm using innerText to show
the coordinates.

Andrew Poulos

 

[Garrett Smith]

I tried it and it also gives incorrect results.

I've uploaded a simple page here:
<url: http://members.iinet.net.au/~apoulos/rtl.htm >
that displays the issue.

<meta http-quiv="Content-Type" content="text/html; charset=utf-8">
...........^

?

You need to view the page with IE 6 or 7 as I'm using innerText to show
the coordinates.

Don't. Use an alternative to allow the example work in more browsers.
That way, the result might provide useful comparison.

In IE, whitespace is dropped. The "\n" can be replaced with "<br>" for
IE, using innerHTML.

document.getElementById('txt').innerHTML = s.replace(/\n/g, "<br>");

Regardless of making those changes, I don't see the issue you have
described.

I understood the problem you described as this:
IE reports the offsetLeft of the target div as the distance from the
right side.

Did I understand that correctly?


If so, I don't see that problem.

I see is the box with the red border positioned on the right side of the
page (the "oContainer" div).

I see the various "left" properties of the element printed on the page.
The values seem to be roughly the distance from the left side of the
page, by my eyes.

Regarding those numbers, there are discrepancies between versions of IE,
depending on the presence or absence of a vertical scrollbar and
depending on the version of IE and the border of the container of your
target div. The border was not added in your offset* calculation --a
mistake of your calculation.

I hope I described what I see well enough.

Can you provide some numbers and a version of IE? A screenshot would
help illustrate the problem clearly. That should not be absolutely
necessary, if an accurate description is provided.

Garrett

 

[Andrew Poulos]

<meta http-quiv="Content-Type" content="text/html; charset=utf-8">
..........^

?

A typo.

Don't. Use an alternative to allow the example work in more browsers.
That way, the result might provide useful comparison.

In IE, whitespace is dropped. The "\n" can be replaced with "<br>" for
IE, using innerHTML.

document.getElementById('txt').innerHTML = s.replace(/\n/g, "<br>");

Regardless of making those changes, I don't see the issue you have
described.

The problem is only in IE 6 and 7 so I choose to ignore other browsers.
In any case I've made the change you suggested so I can compare the
values returned with other browsers.

I understood the problem you described as this:
IE reports the offsetLeft of the target div as the distance from the
right side.

Did I understand that correctly?

No, offsetLeft is still measured from the left (I believe) but the value
returned is wrong.

If so, I don't see that problem.

I see is the box with the red border positioned on the right side of the
page (the "oContainer" div).

I see the various "left" properties of the element printed on the page.
The values seem to be roughly the distance from the left side of the
page, by my eyes.

Regarding those numbers, there are discrepancies between versions of IE,
depending on the presence or absence of a vertical scrollbar and
depending on the version of IE and the border of the container of your
target div. The border was not added in your offset* calculation --a
mistake of your calculation.

I hope I described what I see well enough.

The displayed values for left which are returned by js do not match the
actual left position of the green rectangle.

Can you provide some numbers and a version of IE? A screenshot would
help illustrate the problem clearly. That should not be absolutely
necessary, if an accurate description is provided.

In IE 6 and 7 offsetLeft and getBoundingClientRect return the wrong
value when the page's text direction is set to RTL.

As a rough test resize the viewport of both Firefox and IE 6 to the same
size and then navigate to the page in question:

getClientRects
FF : Left 518 Top 91.5
IE6: Left 593 Top 91

getBoundingClientRect
FF : Left 518 Top 91.5
IE6: Left 593 Top 91

offSet
FF : Left 517 Top 91
IE6: Left 573 Top 88


With IE 7 and a different sized viewport I get:

getClientRects
FF : Left 529 Top 91.5
IE7: Left 600 Top 91

getBoundingClientRect
FF : Left 529 Top 91.5
IE7: Left 600 Top 91

offSet
FF : Left 528 Top 91
IE7: Left 581 Top 88


Top is, in all cases, more or less the same but left is much further off
than is correct taking into account border and scroll bar widths.

Andrew Poulos

 

[Thomas 'PointedEars' Lahn]

I have a DIV sized with CSS and its display is inline:

Nerk is the <div id="target0">&nbsp;</div>.

Other DIVs of the same size get positioned exactly over the top of it.

I use code based on offsetLeft and offsetTop (that people here helped me
with) to get its position.

Doesn't sound like a good strategy. Use CSS instead.

All works well when the text direction is set to 'left to right' (LTR)
but if its 'right to left' (RTL) IE 6 thinks the page is LTR and
offsetLeft for the DIV ends being to a mirror position. That is, its way
off.

How do you get IE 6 to behave?

Isn't offsetLeft_RTL === offsetParent.offsetWidth_RTL - offsetLeft_LTR?

,--------------------------.
| |
| ,---------------. |
| | LTR | |
| `---------------' |
| |
| ,---------------. |
| | LTR | |
| `---------------' |
| |
`--------------------------'
<--> <------->
offsetLeft_LTR offsetLeft_RTL

<-------------------------->
offsetParent.offsetWidth_LTR


PointedEars

 

[Thomas 'PointedEars' Lahn]

I have a DIV sized with CSS and its display is inline:

Nerk is the <div id="target0">&nbsp;</div>.

Other DIVs of the same size get positioned exactly over the top of it.

I use code based on offsetLeft and offsetTop (that people here helped me
with) to get its position.

Doesn't sound like a good strategy. Use CSS instead.

All works well when the text direction is set to 'left to right' (LTR)
but if its 'right to left' (RTL) IE 6 thinks the page is LTR and
offsetLeft for the DIV ends being to a mirror position. That is, its way
off.

How do you get IE 6 to behave?

Isn't offsetLeft_RTL === offsetParent.offsetWidth_LTR - offsetLeft_LTR?

,--------------------------.
| |
| ,---------------. |
| | LTR | |
| `---------------' |
| |
| ,---------------. |
| | LTR | |
| `---------------' |
| |
`--------------------------'
<--> <------->
offsetLeft_LTR offsetLeft_RTL

<-------------------------->
offsetParent.offsetWidth_LTR


PointedEars

 

[Andrew Poulos]

Doesn't sound like a good strategy. Use CSS instead.


Isn't offsetLeft_RTL === offsetParent.offsetWidth_LTR - offsetLeft_LTR?

I'm confused, it appears that its not as simple as the mirroring I
thought was happening. I'll run some more tests.

Andrew Poulos

 

[Garrett Smith]

I'm confused, it appears that its not as simple as the mirroring I
thought was happening. I'll run some more tests.

I was also confused by that statement.

I made an example based on your example using display: inline for the
target element and display: block for its containing block.

I ran the example in IE7 and measured the distance using JR Screen
Ruler[1] from the right side of the target element to the right edge of
the content area of the element's parent. That distance looked to be
133px. The target's offsetLeft was reported on the page as "e.offsetLeft
= 133".

It appears IE returned what would be "offsetRight" for that inline element.

Next I tried setting position: relative on the target element, but that
resulted in the element becoming visually hidden in IE7. I undid that
change and set zoom: 1. This resulted in no visual change but did change
the numbers returned from offsetLeft, getClientRects, and
getBoundingClientRect.

Upon this discovery, I searched: rtl offsetLeft hasLayout and found
http://trac.dojotoolkit.org/ticket/4349

The example:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">
<html>
<head>
<title>Right to Left</title>
<meta http-quiv="Content-Type" content="text/html; charset=utf-8">

<style type="text/css">
body {
margin:0;
direction:rtl;
unicode-bidi:embed;
}
#oContainer {
position:absolute;
top:50px;
right:10px;
height:100px;
width:300px;
background-color: #FFCCAA;
}
#id116 {
display:inline;
height:18px;
padding:0 50px;
background:green;
/** trigger a layout */
zoom: 1;
}
</style>
<script type="text/javascript">
window.onload = function () {
var e = document.getElementById('id116');
var o = document.getElementById('oContainer');
var s = "";

s = "getClientRects:\n";
var col = e.getClientRects();
s += " Left " + col[0].left + " Top " + col[0].top + "\n\n";

s += "getBoundingClientRect:\n";
var bndrct = e.getBoundingClientRect();
s += " Left " + bndrct.left + " Top " + bndrct.top + "\n";
s += " Right " + bndrct.right + " Bottom " + bndrct.bottom +
"\n\n";

s += "offset*:\n";
s += " Left " + (o.offsetLeft + e.offsetLeft)
+ " Top " + (o.offsetTop + e.offsetTop)
+ "\ne.offsetLeft: " + e.offsetLeft;

document.getElementById('txt').innerHTML = s.replace(/\n/g, "<br>");
}
</script>
</head>

<body>
<div id="oContainer">

I'm trying to get the position of this green rectangle with the text
direction set to RTL in IE 6 and 7. &nbsp;<div
id="id116">&nbsp;</div>&nbsp;
I get "correct" values with LTR but not RTL.
</div>
<pre style=" margin-top:300px; margin-left:100px; text-align:left;"
id="txt">&nbsp;</pre>
</body>
</html>

End example.

<aside>
JR Screen Ruler should be added to the FAQ, somewhere, but where?

IE Tester should also be added to the FAQ.

I think a section on debugging is in order.
</aside>

Garrett
[1]http://www.spadixbd.com/freetools/jruler.htm

 

[David Mark]

Garrett Smith wrote:

[...]

It appears IE returned what would be "offsetRight" for that inline element.

Next I tried setting position: relative on the target element, but that
resulted in the element becoming visually hidden in IE7. I undid that
change and set zoom: 1. This resulted in no visual change but did change
the numbers returned from offsetLeft, getClientRects, and
getBoundingClientRect.

Interesting. It doesn't happen in IE8 standards mode, but happens in IE8
  compat. one (although it returns "correct" values in compat. mode when
giving element a "layout").

This reminds me of a bug when IE returns wrong offset* values on a
static element inside positioned one when an element is preceded by
another block element
<http://github.com/kangax/prototype/blob/95f5f56ba1724cfc3574dc3d5fb3f...>

Interesting giant black box at the bottom the document (in FF3.)
Additionally, the document caused my PC's fans to sound like a jet
engine on takeoff. Git hub indeed.

Giving element "position:relative" fixes it (as might "zoom:1", but I
haven't checked yet)

Z wouldn't bother as both Prototype and GP position calculators are
antiquated notions.

[snip]

 

[David Mark]

Strange. I don't see any block box in FF3. Here's a plain text version
(under the name
BUGGY_OFFSET_VALUES_FOR_STATIC_ELEMENTS_INSIDE_POSITIONED_ONES)

<http://yura.thinkweb2.com/cft/feature_tests.js>

Have you ever stumbled upon this bug?

I don't think so. And you are falling into the same trap as the
jQuery project, mixing the unknown (and unneeded) variable of
innerHTML into basic feature testing equations.

What's a GP position calculator?

GP = general purpose.

 

[David Mark]

David Mark wrote:
[...]
This reminds me of a bug when IE returns wrong offset* values on a
static element inside positioned one when an element is preceded by
another block element
<http://github.com/kangax/prototype/blob/95f5f56ba1724cfc3574dc3d5fb3f...>
Interesting giant black box at the bottom the document (in FF3.)
Additionally, the document caused my PC's fans to sound like a jet
engine on takeoff.  Git hub indeed.
Strange. I don't see any block box in FF3. Here's a plain text version
(under the name
BUGGY_OFFSET_VALUES_FOR_STATIC_ELEMENTS_INSIDE_POSITIONED_ONES)
<http://yura.thinkweb2.com/cft/feature_tests.js>
Have you ever stumbled upon this bug?

I don't think so.  And you are falling into the same trap as the
jQuery project, mixing the unknown (and unneeded) variable of
innerHTML into basic feature testing equations.

I can see the danger in using non-standard and known-to-be-buggy
property. It's just that the increase in amount of code for this
seemingly simple test is a bit disturbing (if we were to stop using
innerHTML). It's also not clear to me when and which properties should
be tested and which can be assumed to work.

Could you, please, clarify this?

Yes, feature tests should be as simple and direct as possible. The
last thing you want to do is involve unknown variables like innerHTML.

var BUGGY_OFFSET_VALUES_FOR_STATIC_ELEMENTS_INSIDE_POSITIONED_ONES =
(function(){
   function clearStyles(el) {
     var s = el.style;
     s.margin = '0';
     s.padding = '0';
     s.border = '0';
     s.visibility = 'hidden';
   }
   var body = document.body,
       isBuggy = null;

   // let's assume that ideally these are tested with
   // `isHost*` methods
   if (body &&
       body.insertBefore &&
       body.removeChild &&
       document.createElement &&
       document.getElementById) {

You don't normally go through all of that for every function (lots of
tests need these methods.) I don't see why you need gEBI for this
one.

     var id = 'x' + (Math.random() + '').slice(2);
     var wrapper = document.createElement('div');

     if (wrapper && wrapper.style && wrapper.appendChild) {

Don't need to test wrapper.

       var elAbs = document.createElement('div');

       // should we check for existence of `elAbs` element at this point?
       // what about its style property?

No and no.

       elAbs.style.position = 'absolute';
       elAbs.style.top = '10px';
       clearStyles(elAbs);

The name of that function is confusing. Could be re-used for other
tests as well.

       var elRel = document.createElement('div');

       // should we perform the same check here again?
No.

       elRel.style.position = 'relative';
       elRel.style.top = '10px';
       clearStyles(elRel);

       var elStatic = document.createElement('div');

       // what about this one?
No.

       elStatic.style.height = '10px';
       elStatic.style.fontSize = '1px';
       clearStyles(elStatic);

       var el = document.createElement('div');

       // do we test appendChild on every of these elements
       // before calling it?
No.

       elRel.appendChild(elStatic);
       elRel.appendChild(el);
       elAbs.appendChild(elRel);
       wrapper.appendChild(elAbs);

       // should firstChild existence be tested as well?

Sure, why not. Should test it once in the "gateway" to tests that
require it.

       body.insertBefore(wrapper, body.firstChild);

       if (el && el.style) {

Don't need to test el or el.style at this point.

         // does el.offsetTop need to be checked for typeof "number"?
         if (el.offsetTop !== 10) {

Yes, but again this should be a one-time test.

           // buggy, set position to relative and check if itfixes it
           el.style.position = 'relative';
           if (el.offsetTop === 10) {
             isBuggy = true;
           }
         }
         else {
           isBuggy = false;
         }
       }
       body.removeChild(wrapper);
     }
     wrapper = elAbs = elRel = elStatic = el = null;
   }
   return isBuggy;

})();

I'm still not sure I understand this workaround (for IE?) Probably a
situation to avoid (design out of the system.)

(I haven't tested it after taking innerHTML out)

It will tell you something more specific than the test that relies on
the unrelated innerHTML property.

 

[David Mark]

[...]

Could you, please, clarify this?

Yes, feature tests should be as simple and direct as possible.  The
last thing you want to do is involve unknown variables like innerHTML.

Makes sense. Ironically, I used innerHTML to simplify things (as it
seemed to me); Apparently, its non-standard nature makes things only worse.

You don't normally go through all of that for every function (lots of
tests need these methods.)  I don't see why you need gEBI for this
one.

Forgot to take gEBI out after removing innerHTML. I was just thinking
that mixing tests for existence (i.e. isHost* ones) and tests for actual
deficiencies (i.e. buggy behavior, etc.) makes quite a mess. The code is
harder to understand and navigate through; it also leads to duplication.

Returning early in case any of required components is missing makes much
more sense.

Don't need to test wrapper.

So we should only test `document.createElement` once and its ability to
produce elements once? Or do we not test ability to create at all? I
always thought that a client might not have enough memory to create a
new element, yet it could have a fully functional `document.createElement`.

[snip test]

No.

Sure, why not.  Should test it once in the "gateway" to tests that
require it.

Ok.

Btw, I just looked at "My Library" again and noticed an inconsistency in
testing for exactly this property - firstChild. `getElementText` method
does - if (isRealObjectProperty(html, 'firstChild')) { - as a final
fallback when branching its implementation, but its counterpart that
follows next - `setElementText` goes straight ahead and uses
`firstChild` in a second branch without a test similar to the one in
`getElementText`. It does test for `removeChild` and `createTextNode`
but not for `firstChild`. If this test is not needed, then I don't see a
reason to perform it in `getElementText`.

It is an oversight.

On a side note, it looks like `getElementText` tries proprietary
`innerText` before standard `textContent`. Shouldn't those be switched?

Almost certainly. No idea why I would have switched them. ISTM that
those two methods are very old and were not part of CWR (just stuff I
had on the shelf.)

Don't need to test el or el.style at this point.
Ok.

Yes, but again this should be a one-time test.
Ok.

[...]



I'm still not sure I understand this workaround (for IE?)  Probably a
situation to avoid (design out of the system.)

Yes, for IE. Here's a minimal test case to illustrate this issue
(reports 10 in IE, when element is not given layout to, and 0 - on other
browsers)

I'll bet it evens out on the next measurement in either case. In
other words, the offsetParent will have the opposite discrepancy (or
the offsetParent will be a different element.)

In any event, it seems like a measurement to avoid. Likely you can
measure to the outermost DIV without a discrepancy.

[snip]

 

[David Mark]

[...]

Btw, I just looked at "My Library" again and noticed an inconsistency in
testing for exactly this property - firstChild. `getElementText` method
does - if (isRealObjectProperty(html, 'firstChild')) { - as a final
fallback when branching its implementation, but its counterpart that
follows next - `setElementText` goes straight ahead and uses
`firstChild` in a second branch without a test similar to the one in
`getElementText`. It does test for `removeChild` and `createTextNode`
but not for `firstChild`. If this test is not needed, then I don't seea
reason to perform it in `getElementText`.

It is an oversight.

Ok. I don't have much time to looks into things deeply, but I do see few
more, if you're interested. Some of them look like an oversight, others
- like an edge case.

I'm always interested. Likely I know about most of them, but it is
good to document any inconsistencies.

There's more inconsistency with testing properties;
for example, when looking at the very same `getElementText`, I was
trying to see which properties you test and which - not. I noticed that
`nodeType` was not tested, but when glancing a bit further, found that
`constructElementHtml` does indeed test for `typeof html.nodeType ==
'number'`.

Inconsistency. Today, in most contexts, I wouldn't bother testing
nodeType.

Speaking of `constructElementHtml`, it seems to declare "<", ">", "&"
and "\"" regexes and then use them to escape attributes values.

I don't even remember what that function does. It's internal IIRC.
Modules that rely on it (very few) should be considered suspect.

Same

thing happens in `setElementText` (except that "\"" is not encoded
there). This seems like a lot of duplication. Perhaps it was  meant to
make things modular, but I would probably create `escapeHtmlEntities` or
similar helper.

It illustrates the fact that the library was thrown together very
quickly.

Actually now that I look at that replacement, the order seems to be wrong..

Which replacement?

var el = document.createElement('div');
el.title = 'foo > bar & baz';
var wrapper = document.createElement('div');
wrapper.appendChild(el);

API.getElementHtml(wrapper);
// "<div title="foo &amp;gt; bar &amp; baz"></div>"

Yes, that is FUBAR. So whatever I did in constructElementHtml, which
was apparently duplication, was wrong. I'm not particularly surprised
as nobody ever used those inner/outerHTML replacement methods (at
least not that I know of.) They were just an academic exercise for me
at the time (and apparently I failed in at least one respect!)

Shouldn't it be "<div title="foo &gt; bar &amp; baz"></div>"?

I remember the same bug was in Prototype.js for ages.

That was an asymmetry between the get and set element text methods. I
certainly don't think mine is afflicted with that, but apparently I
botched a duplication of the same logic in the HTML functions.

Then there's another regex used in `constructElementHtml` -

reSelfClosing = new
RegExp('^br|hr|img|meta|link|input|base|param|col|area$');

There's a pretty common bug which gets overlooked quite often - a regexp
as it is will match values such as - "bridge" and "my-area", since
alternation "bounds" are only limited by parenthesis and /^foo|bar$/
does *not* only match "foo" or "bar"; /^(foo|bar)$/ is what matches only
"foo" or "bar". As a result, it looks like `reSelfClosing` will
erroneously match a quite common not "self-closing" tag - "textarea"
(since it ends with "area" and satisfies a regex).

Yes, the parenthesis are missing.

There are also insignificant regex deficiencies like:

new RegExp('<object[^>]*>(.*)</object>', 'i');

which will not work reliably with perfectly valid values such as -

<object title="foo > bar"></object>

and also performs a greedy match (e.g. matching "a</object><object>b" in
"<object>a</object><object>b</object>)

Yes, I had to look up where that came from. The Flash module has at
least one other silly mistake related to setting the class attribute
(of all things.)

Ok. I thought maybe `innerText` had wider supported back in the days and
you decided to test for it first

No, it is just old code. The whole idea a year and half back was to
start a community project (sort of like what you are doing now.) Too
late for those who want to build the perfect monolith as I am no
longer interested (i.e. I won't be fixing these issues any time soon.)

[...]

I'll bet it evens out on the next measurement in either case.  In
other words, the offsetParent will have the opposite discrepancy (or
the offsetParent will be a different element.)

Perhaps. In any case, it seems like giving an element "layout" seems
like a reasonable thing to do before attempting to calculate positions,
dimensions, etc. "direction: rtl" is a good example of another
discrepancy and `zoom` being able to "fix" it (just like it "fixes" an
example I gave).

I would avoid any design that attempts to measure the offset of a
statically positioned element. I imagine that includes both of those
cases.

 

[David Mark]

[...]

Btw, I just looked at "My Library" again and noticed an inconsistency in
testing for exactly this property - firstChild. `getElementText` method
does - if (isRealObjectProperty(html, 'firstChild')) { - as a final
fallback when branching its implementation, but its counterpart that
follows next - `setElementText` goes straight ahead and uses
`firstChild` in a second branch without a test similar to the one in
`getElementText`. It does test for `removeChild` and `createTextNode`
but not for `firstChild`. If this test is not needed, then I don't see a
reason to perform it in `getElementText`.
It is an oversight.

Ok. I don't have much time to looks into things deeply, but I do see few
more, if you're interested. Some of them look like an oversight, others
- like an edge case.

I'm always interested.  Likely I know about most of them, but it is
good to document any inconsistencies.

There's more inconsistency with testing properties;
for example, when looking at the very same `getElementText`, I was
trying to see which properties you test and which - not. I noticed that
`nodeType` was not tested, but when glancing a bit further, found that
`constructElementHtml` does indeed test for `typeof html.nodeType ==
'number'`.

Inconsistency.  Today, in most contexts, I wouldn't bother testing
nodeType.

Speaking of `constructElementHtml`, it seems to declare "<", ">", "&"
and "\"" regexes and then use them to escape attributes values.

I don't even remember what that function does.  It's internal IIRC.
Modules that rely on it (very few) should be considered suspect.

Same

thing happens in `setElementText` (except that "\"" is not encoded
there). This seems like a lot of duplication. Perhaps it was  meant to
make things modular, but I would probably create `escapeHtmlEntities` or
similar helper.

It illustrates the fact that the library was thrown together very
quickly.

Actually now that I look at that replacement, the order seems to be wrong.

Which replacement?

var el = document.createElement('div');
el.title = 'foo > bar & baz';
var wrapper = document.createElement('div');
wrapper.appendChild(el);

API.getElementHtml(wrapper);
// "<div title="foo &amp;gt; bar &amp; baz"></div>"

Yes, that is FUBAR.  So whatever I did in constructElementHtml, which
was apparently duplication, was wrong.  I'm not particularly surprised
as nobody ever used those inner/outerHTML replacement methods (at
least not that I know of.)  They were just an academic exercise for me
at the time (and apparently I failed in at least one respect!)

Shouldn't it be "<div title="foo &gt; bar &amp; baz"></div>"?

I remember the same bug was in Prototype.js for ages.

That was an asymmetry between the get and set element text methods.  I
certainly don't think mine is afflicted with that, but apparently I
botched a duplication of the same logic in the HTML functions.

Then there's another regex used in `constructElementHtml` -

reSelfClosing = new
RegExp('^br|hr|img|meta|link|input|base|param|col|area$');

There's a pretty common bug which gets overlooked quite often - a regexp
as it is will match values such as - "bridge" and "my-area", since
alternation "bounds" are only limited by parenthesis and /^foo|bar$/
does *not* only match "foo" or "bar"; /^(foo|bar)$/ is what matches only
"foo" or "bar". As a result, it looks like `reSelfClosing` will
erroneously match a quite common not "self-closing" tag - "textarea"
(since it ends with "area" and satisfies a regex).

Yes, the parenthesis are missing.

There are also insignificant regex deficiencies like:

new RegExp('<object[^>]*>(.*)</object>', 'i');

which will not work reliably with perfectly valid values such as -

<object title="foo > bar"></object>

and also performs a greedy match (e.g. matching "a</object><object>b" in
"<object>a</object><object>b</object>)

Yes, I had to look up where that came from.  The Flash module has at
least one other silly mistake related to setting the class attribute
(of all things.)

Ok. I thought maybe `innerText` had wider supported back in the days and
you decided to test for it first

No, it is just old code.  The whole idea a year and half back was to
start a community project (sort of like what you are doing now.)  Too
late for those who want to build the perfect monolith as I am no
longer interested (i.e. I won't be fixing these issues any time soon.)

[snip]

Well, since I had it open, I did go ahead and fix those three issues
(two in constructElementHtml and the innerText detection order in
setElementText) locally. Will re-build and upload whenever I get a
chance. Likely will fix that Flash class name issue as well (I think
I had already fixed that locally.)

I doubt anyone wants to use the Get HTML module (I certainly can't
think of a practical use), which is the only one to call
constructElementHtml.

I'll have to look at that other regex (also in the Flash module.)
IIRC, that code was for replacing a static Flash movie with its
fallback content (in case the required Flash version was
unavailable.) Why it is using innerHTML at all is a good question and
I suspect the answer is that the code in question is in an IE-only
branch (IE and OBJECT elements have a quirky history.)

Regardless of that, I do recommend the Flash module. I don't think
there is another out there that works without browser sniffing. I've
deployed it on several projects (usually to replace SWFObject or the
like) and it has always worked flawlessly.

 

[David Mark]

Well, since I had it open, I did go ahead and fix those three issues
(two in constructElementHtml and the innerText detection order in
setElementText) locally.  Will re-build and upload whenever I get a
chance.  Likely will fix that Flash class name issue as well (I think
I had already fixed that locally.)

Turns out the Flash stuff was fixed, along with some other
miscellaneous improvements. JSLints latest nitpicks have been
deflected (except for a dozen or so I didn't agree with or couldn't
comprehend.) So I went ahead and re-built and uploaded. Additional
comments are in the My Library discussion group.

[snip]

 

[David Mark]

[...]

There's more inconsistency with testing properties;
for example, when looking at the very same `getElementText`, I was
trying to see which properties you test and which - not. I noticed that
`nodeType` was not tested, but when glancing a bit further, found that
`constructElementHtml` does indeed test for `typeof html.nodeType ==
'number'`.

Inconsistency.  Today, in most contexts, I wouldn't bother testing
nodeType.

It would be interesting to know which properties you consider safe to
use without test (e.g. nodeType; what about nodeName, btw?). AIUI,
"blacklist" (what *not to test*) would make more sense than "whitelist"
(what *to test*).

I also realize that there's no one rule to follow, and that everything
depends on a context and a range of supported browsers for a given
script. In this case, I am talking about a general script for a general web.

[...]

Which replacement?

I meant - .replace(reLt, '&lt;').replace(reGt, '&gt;') ...

[...]

Yes, the parenthesis are missing.

Maybe it would makes sense to add parenthesis to other regexes as well -
`reCollapsibleAttributes`, `rePixelConvert`, etc. just to be technically
precise. `reFeaturedMethod`, for example, will not only match "function"
and "object" but also - "functional", although such value as a typeof
for host object seems extremely unlikely.

Yes, it is the same mistake over and over.

Speaking of regexes, in flash section there is -

var reStartComment = new RegExp('<!--\\[if !IE\\]>-->', 'g');

Why is it restricted to "!IE" only? What about, say, "!(IE 7)" (which is
a valid syntax, IIRC)?

No. That is for non-IE content.

Cookie section uses -

var reEncodeRegExp = new RegExp('([\\.])', 'g');

You can probably see that this won't escape all of the special
characters. Was such loose matching intentional?

Probably not, but I would have to look at the context (no more time
for My Library at the moment.)

A regex used in `setAttributeByReplacement` seems unnecessary verbose -

re = new RegExp(name + '=[\'"]{0,1}[a-zA-Z0-9_]+[\'"]{0,1}', 'i');

Is redundant for sure.

I don't see a reason to include both a-z and A-Z if "i" flag is present,
also the whole range can be replaced with \w and comma/parenthesis could
be matched properly via backreferences (they are not "synced" in the
above regex) -
Right.


re = new RegExp(name + '=([\'"])?\\w+\\1?', 'i');

Yes, that is much better. Of course, you won't normally need the get/
setAttribute wrappers. I'll update it at a later date.

There are also insignificant regex deficiencies like:
new RegExp('<object[^>]*>(.*)</object>', 'i');
which will not work reliably with perfectly valid values such as -
<object title="foo > bar"></object>
and also performs a greedy match (e.g. matching "a</object><object>b" in
"<object>a</object><object>b</object>)

Yes, I had to look up where that came from.  The Flash module has at
least one other silly mistake related to setting the class attribute
(of all things.)
Ok.

No, it is just old code.  The whole idea a year and half back was to
start a community project (sort of like what you are doing now.)  Too
late for those who want to build the perfect monolith as I am no
longer interested (i.e. I won't be fixing these issues any time soon.)

I wish CWR didn't die off. I really don't see anything wrong with a set
of fine-grained low-level methods that employ best practices recommended
here. It would certainly be better than 95% of what you can find on the
web. "My Library" seems like a great foundation, but the lack of unit
tests, inline documentation and, perhaps, code reviews (correct me if
I'm wrong on any of these points) is something that would prevent me
from using it in my projects.

Very little inline documentation and certainly no unit tests (just a
test page.) The code is like an archeological dig (some modules are
new, some ancient, most in-between.) Code reviews have been spotty,
but the CWR stuff (mostly low-level) had been discussed to death
here. The whole library was tested to death at the start of 2008. No
formal testing has occurred since, but then there haven't been a lot
of changes (and all were minor.)

That being said, I don't recommend the project as anything but a
learning experience. My ideas about browser scripting in late 2007
don't translate very well into 2009 (at least not IMO) and I was
obviously prone to some silly mistakes during its preparation. Of
course, the ideas make a lot more sense than the current "state of the
art" iterations of Prototype and jQuery and the mistakes are less
numerous (and generally less silly.)

I am currently collaborating with Garrett on his APE library, and it
works out great so far. APE is much less cautious with host objects of
course and wouldn't work on such range of browsers as My Library would.

I haven't looked at that in a while. What is the goal for it?

 

[David Mark]

[snip]

I am currently collaborating with Garrett on his APE library, and it
works out great so far. APE is much less cautious with host objects of
course and wouldn't work on such range of browsers as My Library would.

Unit testing is nice, but I didn't get very far with the interactive
tests. I think you guys must be messing with it at the moment. (?)

Ajax GET throws this:

formConfig is undefined

I got an error on the slider test too.

 

[David Mark]

[...]

There's more inconsistency with testing properties;
for example, when looking at the very same `getElementText`, I was
trying to see which properties you test and which - not. I noticed that
`nodeType` was not tested, but when glancing a bit further, found that
`constructElementHtml` does indeed test for `typeof html.nodeType ==
'number'`.

Inconsistency.  Today, in most contexts, I wouldn't bother testing
nodeType.

It would be interesting to know which properties you consider safe to
use without test (e.g. nodeType; what about nodeName, btw?). AIUI,
"blacklist" (what *not to test*) would make more sense than "whitelist"
(what *to test*).

I also realize that there's no one rule to follow, and that everything
depends on a context and a range of supported browsers for a given
script. In this case, I am talking about a general script for a general web.

[...]

Which replacement?

I meant - .replace(reLt, '&lt;').replace(reGt, '&gt;') ...

[...]

Yes, the parenthesis are missing.

Maybe it would makes sense to add parenthesis to other regexes as well -
`reCollapsibleAttributes`, `rePixelConvert`, etc. just to be technically
precise. `reFeaturedMethod`, for example, will not only match "function"
and "object" but also - "functional", although such value as a typeof
for host object seems extremely unlikely.

Speaking of regexes, in flash section there is -

var reStartComment = new RegExp('<!--\\[if !IE\\]>-->', 'g');

Why is it restricted to "!IE" only? What about, say, "!(IE 7)" (which is
a valid syntax, IIRC)?

Cookie section uses -

var reEncodeRegExp = new RegExp('([\\.])', 'g');

You can probably see that this won't escape all of the special
characters. Was such loose matching intentional?

It occurs to me that is only for the cookie names. I'm not even sure
what is allowed for those (I stick to alphanumeric characters.)
Evidently I wanted to allow for the period at the time I wrote the
cookie code (1999 IIRC.)

 

[Thomas 'PointedEars' Lahn]

You don't normally go through all of that for every function (lots of
tests need these methods.)

This may be true because `document.body' could be considered to refer
to the same object on every call. However, that does not need to be so.
The method may be reused in another global context, for example.

I don't see why you need gEBI for this one.

Add me.

Don't need to test wrapper.

Not testing the return value would be at least unwise. Per W3C DOM Level 2
Core the createElement() method may throw a DOMException
(INVALID_CHARACTER_ERR) "if the specified name contains an illegal
character"; implementations vary, of course. For example, it is reasonable
to assume that if there is any error creating the object (that is, at least
reserving heap memory for it), the method call will fail in some way. So
the return value may as well not be a reference to a host object. However,
we can safely assume (based on empirical data) that if it is not a reference
to a host object it is a false-value, and (employing simple logic) that if
it is a reference to a host object that the result of the expression is a
true-value, and the property access does not throw an exception (else
`wrapper.style' aso. would not work, too).

No and no.

The wise answer is "Yes and yes." `elAbs' is used as the base object of a
property access below, so it needs to be tested or the property access could
throw a ReferenceError (or worse). And since it does not make sense to
continue if creation of the object failed, that object reference should be
tested as early as possible, which would be exactly here.

var elRel = document.createElement('div');

// should we perform the same check here again?
No.

Yes.

[...]
var elStatic = document.createElement('div');

// what about this one?
No.

Yes.

elStatic.style.height = '10px';
elStatic.style.fontSize = '1px';
clearStyles(elStatic);

var el = document.createElement('div');

// do we test appendChild on every of these elements
// before calling it?

No.

The wise answer is "Definitely yes."; everything else would be a type II
object inference, known to be error-prone. Never rely on that if an object
implements one method specified for an API or is created by another
implemented method specified for the same API, it would support a third
method specified for that API.

Don't need to test el or el.style at this point.

Correct, that test should happen directly after the assignment to `el'.
Because currently we continue with body.insertBefore() despite the
possibility of `el' being not an object reference (which does not make
sense), and `el' is used as an argument to a host method without being
tested first.

Logic suggests that it SHOULD NOT be tested this way because the result of
the `typeof' operation may not indicate the type of the result of the plain
expression.

Yes, but again this should be a one-time test.

That test is fine as it is . It is already a one-time test for property of
the host object previously created, because that object is a different one
on every call.

Please trim your quotes to the necessary minimum required to retain the
context of your replies.


PointedEars