chemerisuk / better-dom

Live extension playground
http://chemerisuk.github.io/better-dom/
MIT License
546 stars 37 forks source link

Experience a lot of issues / strange behaviour #29

Closed ghost closed 9 years ago

ghost commented 9 years ago

Hi

Create script, and works for me most of the time, BUT there are issues and flaws here.

  1. mouseenter and mouseleave are not working at all
  2. pointerover and pointerout suffer the same thing as mentioned above. Not working
  3. click can be done in delegated events on disabled elements
  4. No namespaces? Any plans to add this?
  5. Your RAF solution have issues on disabled tabs when you swap browser window, are eating battery
    on mobile devices. Take a look at velocity.js to understand better: https://github.com/julianshapiro/velocity/blob/master/velocity.js#L3299
  6. Checkboxes loose their state in IE after appending elements to DOM
  7. CSS browser issues are not taken care of. Not even cross-browser. Example Chrome 31-36 return text-decoration-line and text-decoration-color which are not expected yet. See https://code.google.com/p/chromium/issues/detail?id=342126

There are other flaws too.

  1. No plans to add feature for different units? Just now I see you are using only 'px'. What is someone want to use 'em' or '%'?

The list are longer, but any plans to fix this?

chemerisuk commented 9 years ago

Hi @justmeandtaken, see my comments below:

1) correct, at present there is no polyfill for mouseenter/mouseleave, because you can achieve similar effect using mouseover/mouseout. The polyfill can be added, though I need to investigate the value. Can you describe your use case?

2) no plans to polyfill pointer events for now because their future is unclear. Chrome and Safari that own mobile market decided not to implement pointer events spec.

3) I don't see why it's a bug. Disabled elements are transparent therefore your click is fired on the parent element. This is a native browser behavior.

4) What do you mean by namespaces? There is only DOM namespace

5) It doesn't drain battery because it uses CSS3 for animations. Declarative animations are very different from what velocity does, and more energy effective, because I don't calculate every frame in JS thread.

6) not sure I got it, could you provide an example?

7) there is no Chrome-specific code in CSS accessor module. If you got those values then they are returned by browser. I'd recommend just use standards-based property names in your code to avoid such problems.

8) you can use any units today: el.css("padding", "1.5em") etc.

ghost commented 9 years ago

Hi

I got short time here, and have to dig up some examples later on. So I only reply short to 1) and 4) now.

1.) As far as I know a lot of users still using this, and when I looked at your code I could get mouseenter to work simply to add a handler for it and return " handler.capturing = true;" but I'm 100% this is not the correct method.

4) Namespace, I was meaning namespacing in events. E.g. 'click.chemerisuk.foo', fn);

chemerisuk commented 9 years ago

1) mouseenter/mouseleave do not bubble according to the spec. jQuery makes them to be bubbling via using mouseover/mouseout behind the scenes. And as a result introduces some mess. That's why I wasn't fast in implementing this kind of change. Need to investigate the best way, but you can use mouseover/mouseout in case of event delegation.

4) Again, this is non-standard. Moreover jQuery uses dot (.) for event namespacing for a some reason, that makes event names confusing. Not sure why they didn't use colon (:) that is a standards-based namespace separator for tag names at least.

Anyway, for situations when you need namespaces for events I'd recommend to trigger custom events and listen to them. It's much clearer, robust, standards-friendly etc.

So the short answer is no, I'm not going to add namespaces for events.

ghost commented 9 years ago

Hi

I understand you. But now I encounter another issue here I guess with your events, and set() method.

Study this:

HTML


<select id="single">
<option>Test</option>
<option>Test2</option>
</select>

<select id="multiple" multiple="multiple">
<option selected="selected">Multiple</option>
<option>Multiple2</option>
<option selected="selected">Multiple3</option>
</select>

Javascript:

// This one will not work now except if you attach it to a live extension. However it's only the ID #single that will work.

DOM.find( "select" ).on('change', function() {
    })

Next you can try this on the ID #Multiple: and nothing get triggered. I also tried to 'fire' it.

If you investigate this further, you will see that for ID #multiple it has two values that are selected. It will only find one of the values, but this seems to create one of the issues.

If you try different scenarios you will notice other strange things. It also seems like that the 'change' are not working if you try this:

DOM.find( "select" ).on('change', function() {
console.log("test");
    })

It's working only for ID #single.

My goal was to have a common event handler for all Selects that get fired with a live extension when I changed some values.

Any solution here?

I investigated further, and this is not only related to the issues above.

Let say you have 2 or more HTML buttons, and this code:

DOM.find( "button" ).on('click', function() {
console.log("clicked!");
    })

In this case only the first button will get activated.

If you try to use your DOM.findALL() method, it will just throw an error and tell me that on() is not a function.

You can work around this things ofc, with settings a CLASS on it, and use a live extension like that. But what if you don't want a CLASS name on it?

And I can also add here that if you set a live extension on a "button" - in this case, and you try to click on the button. Only live extension will work for the first button as well.

ghost commented 9 years ago

Another issue is checkboxes and set() method.

How is the array working in the set method? Didn't find any clear examples, and didn't work when I tried and array

[ "check1", "check2", "radio1" ]

For me this is an array, but not working.

Anyway, I can't set any checkbox values on pageload, and not a value in multiple checkboxes either. Same for radio buttons.

And if I attach a event handler, same will happen as I described earlier. First one are selected.

chemerisuk commented 9 years ago

My goal was to have a common event handler for all Selects that get fired with a live extension when I changed some values.

I guess it's simpler to attach one global listener in such case: http://jsfiddle.net/8hqmttj3/

Not sure what issues did you have - the event fires for both selects on my side. But remember, that unlike jQuery find method in better-dom grabs only the first matched element. If you need to grab all, use findAll instead that returns JS array with elements.

chemerisuk commented 9 years ago

You can't set arrays using $Element#set. Use findAll+forEach instead (if I understood your use case correctly):

DOM.findAll("input[type=checkbox]").forEach(function(el, index) {
  el.set("checked", index % 2 === 0);
});
ghost commented 9 years ago

Well, same code in jsFiddle, but was working locale on my harddrive. But as you see two selects are checked, only one are marked when you load the page. Shouldn't it be 2 if 2 are selected?

I understand the findAll concept, but then again I need to iterate through an array first?

For your latest reply. Ok, I got it, and it make sense but at this stage a simple developer need to become an expert. In jQuery a simply val() / attr() method and case done.

chemerisuk commented 9 years ago

Well, better-dom doesn't touch anything there :) I didn't use select[multiple] a lot, may be it works in this way. Try to use shift to make multiple selections.

Yes, findAll returns true arrays without sugar methods. This is very different from jQuery, but simpler. Just use forEach + appropriate method if you need to invoke a DOM-related method. Other array methods like map, 'filter' etc. are sometimes useful too.

ghost commented 9 years ago

shift will not help here because you in the code are returning a single value, and not an array with multiple values if multiple elements are selected. Therefor It doesn't select correct values when you load the page.

When you select with shift, still single value and not multiple values returned as an array.

chemerisuk commented 9 years ago

Alright, so you are trying to read the value (get, not set). You can get array with selected select[multiple] values using code below:

DOM.find("#multiple").children("[selected]").map(function(el) { return el.get() });
ghost commented 9 years ago

Something else come up here now. For querySelector ( All ) and matchesSelector. How are this working on XML documents? QSA has various cross-browser issues, and IE9 matchesSelector returns false on disconnected nodes, is there a fix for this?

From jQuery source: ' disconnected nodes are said to be in a document fragment in IE 9'

And if I now want to use better-dom with XML documents, this will work right out of the box? SVG too?

chemerisuk commented 9 years ago

I haven't tested better-dom with XHTML yet, because didn't have such a requirement. After HTML5 came to the mainstream the XHTML format is declared as useless I think. No plans to support it.

I'll investigate matches in IE9 for disconnected nodes, thanks.

SVG should work as it's just a node subtree. I'll add a test for it in the test suite as well.

ghost commented 9 years ago

Ok. I spent a few hours now to test this library, and I noticed it's very limited, and still a lot of issues to be solved. So I don't think lib are something I can use right now.

Here are a few things I noticed:

The list goes on....

What I mean with to many restrictions? Well. Example every function you have where you pass in a selector, it's limited to be a typeof string. What if someone need to pass in a object?

Another thing I never figured out was way I never could pass in strings like (document.body) I had to quote it like 'body', but then again I would deal with the document body and not the document.

And I also your documentation seems to be wrong, and errors in the emmet function.

I was reading you could do this:

DOM.create("input[name={name} value={val}]", {name: "myname", val: "myval"});

But this are broken.

Just to get to my point. Try this:


var test = DOM.create("input[name={name} value={val}]", {name: "myname", val: "myval"});

DOM.find('body').append(test )

It's still blank - no values - and if you take a look into the console, you will see this:

<input value="" name="">

I had expected the val to be "mywal" and name to be "myname", but maybe I'm wrong.

Anyways. A great script!

ghost commented 9 years ago

Btw. for the IE9 disconnected nodes issue, take a look at this repo: https://github.com/desandro/matches-selector/blob/master/matches-selector.js

ghost commented 9 years ago

I'm going to abondon this library now, but will first mention that you are not completly following the classList API either. Example classList API have support for multiple arguments, but issue in #IE10.

Anyway. When I tried to add 3 classNames splitted by space, this message I got in the console.log:

InvalidCharacterError: String contains an invalid character
return el[0].classList[nativeMethodName](token);

So then I tried to separate the class names with comma, but same error.

Any plans to fix / support this?

A simple check for multiple arguments, can be done like this:

(function() {
        // #IE 10 - 11 - Detect if the classList API supports multiple arguments
        var div = document.createElement('div');
        div.classList.add('a', 'b');
        supportsMultipleArgs = /(^| )a( |$)/.test(div.className) && /(^| )b( |$)/.test(div.className);
        // release memory in IE
        div = null;
    }());
ghost commented 9 years ago

My mistake, because you are actually not using the classList API at all because of a bug in your code.

In your code, you have this:

            if (HTML.classList) {

but this will not give a boolean value. Try to alert() that value and you get nothing. The correct code would be:

            if (!!HTML.classList) {

All I was doing was adding !! and then you are using the classList API, but still I encounter the error I mentioned.

ghost commented 9 years ago

A last question is regarding your trim() ( IE9+ ) method. I see you are using it different places. Is there any polyfill in your code for this for IE8? And how you solve the trim() method for Android v. 4.1 and lower that gives this issues and not support a polyfill?

chemerisuk commented 9 years ago

@justmeandtaken

ghost commented 9 years ago

Problems with trim() for Android 4.1 and lower are related to BOM and NBSP.

A workaround / polyfill around this are found here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/Trim

The emmet example I mentioned, how did you manage it to work? Witch build of your script? I tried in all latest builds of all browsers available. Not working in my end.

I could have sent you a screenshot of the console log, and you will see the same as I mentioned. I "deep" checked that part of the code, and in your DOM.format() function it works in the beginning of the emmet() function.

BUT in the main emmet() function if you do some magic console loging or alerts(), you will notice it's not working. At lest in my end.

For classList, be aware that there are browser bugs in iOS 5.1, Opera 11.50 and lower. But resolved in newer browsers.

IE still not supporting the toggle methods second argument. I think MS are going to solve it now in the upcoming IE 12, but still not solved in IE 11. Multiple arguments not supported at all.

I see there is a lot of things here that are left behind to the end-developer, but be aware that he / she first will think that something are wrong in your code if something goes wrong, before checking it's own code. ( Same as happen to me when I checked this library).

At least you have to avoid the script from throwing, and do it gentle :)

ghost commented 9 years ago

I give you a tip for the future. Most likely your script will get issues on Android 5.0 / 5.1. Be aware of this when you continue developing. A live example on the issues is simple: try to use Facebook app on Android 5.0. I also heard rumors saying there are issues with touch related methods.

But mostly this issues are now solved, but for Galaxy S5 Android 5 that just got shipped in Poland and Belarus can't run Facebook on the new stock ROM. Some Nexus devices experience the same.

chemerisuk commented 9 years ago

Ok, I'll investigate if it's possible to avoid using of trim then. This function is used only in several places, so it should not be a big deal.

For emmet - check demo: http://jsfiddle.net/apx86Lb6/. The only idea I have is that you are trying to pass variables that have spaces. In such case you have to use back ticks to achieve a desired result:

DOM.emmet("input[name=`{name}` value=`{val}`]", {name: "my name", val: "my val"});

I'm aware of bugs in classList implementations, therefore don't use multiple arguments for it. Same for the second argument in $Element#toggle.

This is ok if a young library is judged as a buggy by junior developers. Without a good documentation only seniors that don't afraid to look at the source code can recognize the value that it can give. I'm aware of lacking a good manuals, and I'll work on improving that as soon as complete adding new cool stuff like $Element#context :)

ghost commented 9 years ago

I understand now how you got it working, but I was only following this: https://github.com/chemerisuk/better-dom/wiki/Microtemplating Section: Abbreviation variables

In that example you was given there, there are no ticks.

I got it working now when I use ticks

Can I ask how many years you have developed? When I looked at the code, I was more or less not thinking Vanilja JS and mostly people will not either because mostly people are used to jQuery. But with a good documentation, I think you are on the right path here.

But I didn't deep dive into your script completly, but I understand how it works, a tiny layer on top of DOM that following the natural way of doing it. And that is positive.

When it come to me, all I can say it's hard to judge people's skill on Github. Mostly this is a playground for noobs, and therefor you have to speak and write in a way noobs understand.

That is not the case with you, you have the talent. I have done development since 1985 with Assambly language in old DOS and up to day, and I have seen a lot :)

chemerisuk commented 9 years ago

Alright, I updated the wiki to reflect back ticks moment.

My professional carrier started in 2005 from languages like C++, C#, Java. Only recently (~3 years ago) I switched to JavaScript.

Github is a good thing. I'd encourage people try Open Source because it can teach a lot.

mrzafod commented 9 years ago

Максим, это тебя видимо тролят по заказу jquery

chemerisuk commented 9 years ago

Надеюсь, что нет :)