New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checking for ":visible" doesn't return expected value #323

Closed
pamelafox opened this Issue Nov 20, 2011 · 20 comments

Comments

Projects
None yet
5 participants
@pamelafox
Contributor

pamelafox commented Nov 20, 2011

It's a common technique in jQuery to check if a DOM element is visible by doing $(elem).is(':visible'). This check always returns false in Zepto.

@arextar

This comment has been minimized.

Show comment
Hide comment
@arextar

arextar Nov 20, 2011

Contributor

From how I see it, you can either:

  • detect for that standalone pseudo-selector and return the visibility of the element
  • detect for the pseudo-selector, remove it from the selector and then proceed as normal

It all depends on whether or not you think people will check more than just the visibility at a time

Contributor

arextar commented Nov 20, 2011

From how I see it, you can either:

  • detect for that standalone pseudo-selector and return the visibility of the element
  • detect for the pseudo-selector, remove it from the selector and then proceed as normal

It all depends on whether or not you think people will check more than just the visibility at a time

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Nov 22, 2011

Collaborator

Yeah, jQuery- (or Sizzle?) specific pseudo-selectors missing in Zepto are quite a problem because of their widespread use. This shows how it was a bad decision to expand the CSS selectors with features not supported natively by browsers.

We can support simple selectors like ":visible", though. Patches welcome

Collaborator

mislav commented Nov 22, 2011

Yeah, jQuery- (or Sizzle?) specific pseudo-selectors missing in Zepto are quite a problem because of their widespread use. This shows how it was a bad decision to expand the CSS selectors with features not supported natively by browsers.

We can support simple selectors like ":visible", though. Patches welcome

@pamelafox

This comment has been minimized.

Show comment
Hide comment
@pamelafox

pamelafox Nov 24, 2011

Contributor

To add to this issue, I also found the following fake selectors missing: ":first" and ":last" which map to .first() and .last() in Zepto, and ":hidden" (opposite of ":visible").

I'm not sure if Zepto should support them or not -- seems like we should discourage use of fake selectors as a best practice on the web. Checking for visible/hidden is pretty useful though, so maybe those could be functions like .first() and .last().

(I say fake and not pseudo as pseudo selectors are actually defined in CSS and if I recall correctly, they work fine in Zepto.)

Contributor

pamelafox commented Nov 24, 2011

To add to this issue, I also found the following fake selectors missing: ":first" and ":last" which map to .first() and .last() in Zepto, and ":hidden" (opposite of ":visible").

I'm not sure if Zepto should support them or not -- seems like we should discourage use of fake selectors as a best practice on the web. Checking for visible/hidden is pretty useful though, so maybe those could be functions like .first() and .last().

(I say fake and not pseudo as pseudo selectors are actually defined in CSS and if I recall correctly, they work fine in Zepto.)

@patik

This comment has been minimized.

Show comment
Hide comment
@patik

patik Dec 9, 2011

Is it possible to add custom (fake) selectors to Zepto like you can with jQuery? I pulled the :visible and :hidden code from jQuery and put them into standalone functions, but I'd really rather do $("div:hidden") to preserve chaining.

I understand not wanting to include them by default, but a flood of "please add this selector!" requests would be staved off if there was a way to shoehorn them in yourself.

patik commented Dec 9, 2011

Is it possible to add custom (fake) selectors to Zepto like you can with jQuery? I pulled the :visible and :hidden code from jQuery and put them into standalone functions, but I'd really rather do $("div:hidden") to preserve chaining.

I understand not wanting to include them by default, but a flood of "please add this selector!" requests would be staved off if there was a way to shoehorn them in yourself.

@ollym

This comment has been minimized.

Show comment
Hide comment
@ollym

ollym Dec 10, 2011

Contributor

I've been thinking about how to best do this in a sustainable way which will not impact the line count too much. What I'm thinking is something along the lines of:

https://gist.github.com/3e66cc2510504cbbc3d5

Inherently this is a little unsafe as it allows people to all any prototype methods with their query selector but that can be easily fixed by making the regex less-greedy. However I like it this way as it makes it incredibly easy to add your own selectors. Maybe even add support for parameters.

div:color("blue") for example.

Thoughts?

Contributor

ollym commented Dec 10, 2011

I've been thinking about how to best do this in a sustainable way which will not impact the line count too much. What I'm thinking is something along the lines of:

https://gist.github.com/3e66cc2510504cbbc3d5

Inherently this is a little unsafe as it allows people to all any prototype methods with their query selector but that can be easily fixed by making the regex less-greedy. However I like it this way as it makes it incredibly easy to add your own selectors. Maybe even add support for parameters.

div:color("blue") for example.

Thoughts?

@arextar

This comment has been minimized.

Show comment
Hide comment
@arextar

arextar Dec 10, 2011

Contributor

I like it- but I'm worried it could slow down the selection considerably

Contributor

arextar commented Dec 10, 2011

I like it- but I'm worried it could slow down the selection considerably

@patik

This comment has been minimized.

Show comment
Hide comment
@patik

patik Dec 10, 2011

@arexkun Do you mean it could slow down all selections, or only selections involving the custom selector?

patik commented Dec 10, 2011

@arexkun Do you mean it could slow down all selections, or only selections involving the custom selector?

@arextar

This comment has been minimized.

Show comment
Hide comment
@arextar

arextar Dec 10, 2011

Contributor

I think it could slow down all selections a bit, which could compound a lot in an application. I don't think there is any way, though, to implement this in selectors without slowing down element selection. I think that because we're using the native qSA, we can afford to lose some time.

Contributor

arextar commented Dec 10, 2011

I think it could slow down all selections a bit, which could compound a lot in an application. I don't think there is any way, though, to implement this in selectors without slowing down element selection. I think that because we're using the native qSA, we can afford to lose some time.

@ollym

This comment has been minimized.

Show comment
Hide comment
@ollym

ollym Dec 10, 2011

Contributor

If a query doesn't contain a custom selector the only performance hit is 1 regex operation. Hardly a massive problem. I see no better way to do it.

Contributor

ollym commented Dec 10, 2011

If a query doesn't contain a custom selector the only performance hit is 1 regex operation. Hardly a massive problem. I see no better way to do it.

@arextar

This comment has been minimized.

Show comment
Hide comment
@arextar

arextar Dec 10, 2011

Contributor

I don't think there is a better way- I definitely think this is the best
way to do it.

Contributor

arextar commented Dec 10, 2011

I don't think there is a better way- I definitely think this is the best
way to do it.

@patik

This comment has been minimized.

Show comment
Hide comment
@patik

patik Dec 10, 2011

I wrote a quick and dirty implementation of :visible and :hidden to hold me over until Zepto integrates custom selectors. This is definitely NOT the best or most efficient way to do it, but I'm new to Zepto and only making light use of this particular selector.
https://gist.github.com/1456711

patik commented Dec 10, 2011

I wrote a quick and dirty implementation of :visible and :hidden to hold me over until Zepto integrates custom selectors. This is definitely NOT the best or most efficient way to do it, but I'm new to Zepto and only making light use of this particular selector.
https://gist.github.com/1456711

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Dec 11, 2011

Collaborator

Thank you all for your commentary and solutions. I have a feeling this can be done in less code and I'll try to work on a patch for Zepto when I have time. If this turns out to be more code than I would like, we can always make it an optional add-on distributed with Zepto, such as "fx_methods".

Collaborator

mislav commented Dec 11, 2011

Thank you all for your commentary and solutions. I have a feeling this can be done in less code and I'll try to work on a patch for Zepto when I have time. If this turns out to be more code than I would like, we can always make it an optional add-on distributed with Zepto, such as "fx_methods".

@arextar

This comment has been minimized.

Show comment
Hide comment
@arextar

arextar Dec 11, 2011

Contributor

What do you think of this? (needs a lot of cleaning up, but it's pretty concise)

var fakeSelectorRE = /:([-\w]+)(?:\(([^\)]+)\))?/g;
// ...
$$ = $.qsa = function(element, selector){
    var found;
    var filter_fn;
    selector = selector.replace(, function(a, b, c){
        var _f = filter_fn;
        if(b in Zepto.fn) return (filter_fn = function(f){return $(_f ? _f(f) : f)[b](c)}), "";
        return a;
    })
    found = (element === document && idSelectorRE.test(selector)) ?
      ( (found = element.getElementById(RegExp.$1)) ? [found] : emptyArray ) :
      slice.call(
        classSelectorRE.test(selector) ? element.getElementsByClassName(RegExp.$1) :
        tagSelectorRE.test(selector) ? element.getElementsByTagName(selector) :
        element.querySelectorAll(selector)
      );
    if(filter_fn) found = filter_fn(found);
    return found;
  }

This allows you to use methods like .eq when selecting (.visible() and .hidden() would have to be filter functions for this to work, though)

Contributor

arextar commented Dec 11, 2011

What do you think of this? (needs a lot of cleaning up, but it's pretty concise)

var fakeSelectorRE = /:([-\w]+)(?:\(([^\)]+)\))?/g;
// ...
$$ = $.qsa = function(element, selector){
    var found;
    var filter_fn;
    selector = selector.replace(, function(a, b, c){
        var _f = filter_fn;
        if(b in Zepto.fn) return (filter_fn = function(f){return $(_f ? _f(f) : f)[b](c)}), "";
        return a;
    })
    found = (element === document && idSelectorRE.test(selector)) ?
      ( (found = element.getElementById(RegExp.$1)) ? [found] : emptyArray ) :
      slice.call(
        classSelectorRE.test(selector) ? element.getElementsByClassName(RegExp.$1) :
        tagSelectorRE.test(selector) ? element.getElementsByTagName(selector) :
        element.querySelectorAll(selector)
      );
    if(filter_fn) found = filter_fn(found);
    return found;
  }

This allows you to use methods like .eq when selecting (.visible() and .hidden() would have to be filter functions for this to work, though)

@ollym

This comment has been minimized.

Show comment
Hide comment
@ollym

ollym Dec 11, 2011

Contributor

Altered my regex statement a little to include parameters and be more consistant.

^([^:]+)?([^\(\s]*)(?:\(([^\)]+)\))?\s*(.+)?$

Will match all css selectors with the following captures (undefined if they do not exist):

$1 = The selector before the first operator. div a:color(white) span:nth-child(3n+3)
$2 = The first operator name including colon. div a__:color__(white) span:nth-child(3n+3)
$3 = The first operator's parameters. div a:color(white) span:nth-child(3n+3)
$4 = The remaining selector text after the first operator. div a:color(white) span:nth-child(3n+3)

Hope this helps. Btw I now don't think my idea is a good idea as if/when you decide to switch to closure compiler w/ advanced mode, my method will fail to work.

Contributor

ollym commented Dec 11, 2011

Altered my regex statement a little to include parameters and be more consistant.

^([^:]+)?([^\(\s]*)(?:\(([^\)]+)\))?\s*(.+)?$

Will match all css selectors with the following captures (undefined if they do not exist):

$1 = The selector before the first operator. div a:color(white) span:nth-child(3n+3)
$2 = The first operator name including colon. div a__:color__(white) span:nth-child(3n+3)
$3 = The first operator's parameters. div a:color(white) span:nth-child(3n+3)
$4 = The remaining selector text after the first operator. div a:color(white) span:nth-child(3n+3)

Hope this helps. Btw I now don't think my idea is a good idea as if/when you decide to switch to closure compiler w/ advanced mode, my method will fail to work.

@ollym

This comment has been minimized.

Show comment
Hide comment
@ollym

ollym Dec 11, 2011

Contributor

PS. @arexkun your regular expression is faulty. It won't match anything without parentheses, maybe you missed a ? at the end of your expression.

Contributor

ollym commented Dec 11, 2011

PS. @arexkun your regular expression is faulty. It won't match anything without parentheses, maybe you missed a ? at the end of your expression.

@arextar

This comment has been minimized.

Show comment
Hide comment
@arextar

arextar Dec 11, 2011

Contributor

Thanks for catching that

Contributor

arextar commented Dec 11, 2011

Thanks for catching that

@arextar

This comment has been minimized.

Show comment
Hide comment
@arextar

arextar Apr 7, 2012

Contributor

A simple plugin implementation for using .is and .filter for ":visible" and ":hidden"
https://gist.github.com/2324454

Contributor

arextar commented Apr 7, 2012

A simple plugin implementation for using .is and .filter for ":visible" and ":hidden"
https://gist.github.com/2324454

@mislav mislav closed this in f716bad Apr 8, 2012

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Apr 8, 2012

Collaborator

Ladies and gentlemen. I've added the experimental "selector" module. Include it by building your distro:

$ rake concat[selector]
$ cp dist/zepto.js /path/to/your/project

It's limited to a subset of jQuery CSS extensions, and only if the pseudo-selector appears in the end of the selector.

// this is supported:
$('div:first')
el.is(':visible')

// NOT supported
$('div:first p')

Implementation here. @arexkun, I'd love if you had a look.

Collaborator

mislav commented Apr 8, 2012

Ladies and gentlemen. I've added the experimental "selector" module. Include it by building your distro:

$ rake concat[selector]
$ cp dist/zepto.js /path/to/your/project

It's limited to a subset of jQuery CSS extensions, and only if the pseudo-selector appears in the end of the selector.

// this is supported:
$('div:first')
el.is(':visible')

// NOT supported
$('div:first p')

Implementation here. @arexkun, I'd love if you had a look.

@arextar

This comment has been minimized.

Show comment
Hide comment
@arextar

arextar Apr 8, 2012

Contributor

I think it looks great. I would just consider exposing the filters object so people can extend it if they need more of jQuery's extensions.

Contributor

arextar commented Apr 8, 2012

I think it looks great. I would just consider exposing the filters object so people can extend it if they need more of jQuery's extensions.

@mislav

This comment has been minimized.

Show comment
Hide comment
@mislav

mislav Apr 9, 2012

Collaborator

Done and done.

Collaborator

mislav commented Apr 9, 2012

Done and done.

lopper added a commit to buddydvd/zepto that referenced this issue Apr 24, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment