#7092 closed bug (fixed)

Opened March 10, 2011 04:09AM UTC

Closed March 11, 2011 03:53PM UTC

Last modified March 11, 2011 03:54PM UTC

button creation that requires a matching label does not find label in all cases

Reported by:	ddstreet	Owned by:
Priority:	minor	Milestone:	1.8.11
Component:	ui.button	Version:	1.8.10
Keywords:		Cc:
Blocked by:		Blocking:

Description

When creating a button() for a checkbox or radio box, where an associated label is required, the code does not correctly search for the associated label; it misses several cases as illustrated here:

http://jsbin.com/akuqu4/15

Specifically, the current code only finds the label if both the checkbox and label have a common ancestor; it misses cases where:

the label is a sibling of the checkbox, but neither have a parent
the label is a uncle of the checkbox (and do not share a common ancestor)
the label is a cousin of the checkbox (and do not share a common ancestor)
the label is a nephew of the checkbox (and do not share a common ancestor)

Attachments (0)

Change History (11)

Changed March 10, 2011 04:38AM UTC by ddstreet comment:1

Sent a pull request for this:

https://github.com/jquery/jquery-ui/pull/147

Ideally, it would be nice if jQuery itself added a function for this, as I'm sure other places in the code may need to do something like this.

Changed March 10, 2011 12:53PM UTC by scottgonzalez comment:2

I really don't think we should handle this. The problem in every one of these cases is that there's no common ancestor.

Changed March 10, 2011 01:49PM UTC by ddstreet comment:3

Replying to [comment:2 scott.gonzalez]:

I really don't think we should handle this. The problem in every one of these cases is that there's no common ancestor.

Yes, exactly, as I said in the description. But IMHO jQuery-ui *should* handle cases where there is not (yet) a common ancestor. The code *already* handles the case where the checkbox and label haven't been added to the DOM yet, so I'm not sure why you're drawing the line between that and this case.

Do you think there should be a core jQuery function for this? Or do you just think nobody should be doing this, so jQuery-ui shouldn't change? Or do you just object to the specific way I handled it in my pull request?

Changed March 10, 2011 02:09PM UTC by rdworth comment:4

_comment0:	Replying to [comment:3 ddstreet]: \ > Replying to [comment:2 scott.gonzalez]: \ > > I really don't think we should handle this. The problem in every one of these cases is that there's no common ancestor. \ > \ > Yes, exactly, as I said in the description. But IMHO jQuery-ui should handle cases where there is not (yet) a common ancestor. The code already handles the case where the checkbox and label haven't been added to the DOM yet, so I'm not sure why you're drawing the line between that and this case. \ > \ > Do you think there should be a core jQuery function for this? Or do you just think nobody should be doing this, so jQuery-ui shouldn't change? Or do you just object to the specific way I handled it in my pull request? \ \ We intentionally don't handle this, not because no one will ever have this markup, but because if someone does, they know better how to deal with it than we do. The only we could do it is choose one of several possible combinations of moving both elements and wrapping one or more of them. That might seriously break someone's form if they're not expecting it, or they might want it done a different way, and we can only choose 1, whereas they can choose many. So, for this reason we don't feel the fix should be built-in. If such a fix is needed, it needs to be done by the user before calling this widget. \ → 1299766174003589
_comment1:	Replying to [comment:3 ddstreet]: \ > Replying to [comment:2 scott.gonzalez]: \ > > I really don't think we should handle this. The problem in every one of these cases is that there's no common ancestor. \ > \ > Yes, exactly, as I said in the description. But IMHO jQuery-ui should handle cases where there is not (yet) a common ancestor. The code already handles the case where the checkbox and label haven't been added to the DOM yet, so I'm not sure why you're drawing the line between that and this case. \ > \ > Do you think there should be a core jQuery function for this? Or do you just think nobody should be doing this, so jQuery-ui shouldn't change? Or do you just object to the specific way I handled it in my pull request? \ \ We intentionally don't handle this, not because no one will ever have this markup, but because if someone does, they know better how to deal with it than we do. The only thing we could do it is choose one of several possible combinations of moving both elements and wrapping one or more of them. That might seriously break someone's form if they're not expecting it, or they might want it done a different way, and we can only choose 1, whereas they can choose many. So, for this reason we don't feel the fix should be built-in. If such a fix is needed, it needs to be done by the user before calling this widget. \ → 1299766205950205
_comment2:	Replying to [comment:3 ddstreet]: \ > Replying to [comment:2 scott.gonzalez]: \ > > I really don't think we should handle this. The problem in every one of these cases is that there's no common ancestor. \ > \ > Yes, exactly, as I said in the description. But IMHO jQuery-ui should handle cases where there is not (yet) a common ancestor. The code already handles the case where the checkbox and label haven't been added to the DOM yet, so I'm not sure why you're drawing the line between that and this case. \ > \ > Do you think there should be a core jQuery function for this? Or do you just think nobody should be doing this, so jQuery-ui shouldn't change? Or do you just object to the specific way I handled it in my pull request? \ \ We intentionally don't handle this, not because no one will ever have this markup, but because if someone does, they know better how to deal with it than we do. The only thing we could do it is choose one of several possible combinations of moving one or both elements and wrapping one or both of them. That might seriously break someone's form if they're not expecting it, or they might want it done a different way, and we can only choose 1, whereas they can choose many. So, for this reason we don't feel the fix should be built-in. If such a fix is needed, it needs to be done by the user before calling this widget. \ → 1299766291221828
_comment3:	Replying to [comment:3 ddstreet]: \ > Replying to [comment:2 scott.gonzalez]: \ > > I really don't think we should handle this. The problem in every one of these cases is that there's no common ancestor. \ > \ > Yes, exactly, as I said in the description. But IMHO jQuery-ui should handle cases where there is not (yet) a common ancestor. The code already handles the case where the checkbox and label haven't been added to the DOM yet, so I'm not sure why you're drawing the line between that and this case. \ > \ > Do you think there should be a core jQuery function for this? Or do you just think nobody should be doing this, so jQuery-ui shouldn't change? Or do you just object to the specific way I handled it in my pull request? \ \ We intentionally don't handle this, not because no one will ever have this markup, but because if someone does, they know better how to deal with it than we do. The only thing we could do is choose one of several possible combinations of moving one or both elements and wrapping one or both of them. That might seriously break someone's form if they're not expecting it, or they might want it done a different way, and we can only choose 1, whereas they can choose many. So, for this reason we don't feel the fix should be built-in. If such a fix is needed, it needs to be done by the user before calling this widget. \ → 1299766403517393
resolution:	→ wontfix
status:	new → closed

Replying to [comment:3 ddstreet]:

Replying to [comment:2 scott.gonzalez]: > I really don't think we should handle this. The problem in every one of these cases is that there's no common ancestor. Yes, exactly, as I said in the description. But IMHO jQuery-ui *should* handle cases where there is not (yet) a common ancestor. The code *already* handles the case where the checkbox and label haven't been added to the DOM yet, so I'm not sure why you're drawing the line between that and this case. Do you think there should be a core jQuery function for this? Or do you just think nobody should be doing this, so jQuery-ui shouldn't change? Or do you just object to the specific way I handled it in my pull request?

We intentionally don't handle this, not because no one will ever have this markup, but because if someone does, they know better how to deal with it than we do. The only thing we could do is choose one of several possible combinations of moving one or both elements and wrapping one or both of them. That might seriously break someone's form if they're not expecting it, or they might want it done a different way. We can only choose one, whereas they can choose one of many. So, for this reason we don't feel the fix should be built-in. If such a fix is needed, it needs to be done by the user before calling this widget.

Changed March 10, 2011 02:30PM UTC by ddstreet comment:5

Replying to [comment:4 rdworth]:

Replying to [comment:3 ddstreet]: > Replying to [comment:2 scott.gonzalez]: > > I really don't think we should handle this. The problem in every one of these cases is that there's no common ancestor. > > Yes, exactly, as I said in the description. But IMHO jQuery-ui *should* handle cases where there is not (yet) a common ancestor. The code *already* handles the case where the checkbox and label haven't been added to the DOM yet, so I'm not sure why you're drawing the line between that and this case. > > Do you think there should be a core jQuery function for this? Or do you just think nobody should be doing this, so jQuery-ui shouldn't change? Or do you just object to the specific way I handled it in my pull request? We intentionally don't handle this, not because no one will ever have this markup, but because if someone does, they know better how to deal with it than we do. The only thing we could do is choose one of several possible combinations of moving one or both elements and wrapping one or both of them. That might seriously break someone's form if they're not expecting it, or they might want it done a different way. We can only choose one, whereas they can choose one of many. So, for this reason we don't feel the fix should be built-in. If such a fix is needed, it needs to be done by the user before calling this widget.

My pull request worked fine in my tests, so I'm not sure why you are saying you would have to do any strange stuff. Can you at least let me know why you think the patch in my pull request wouldn't work (even though it appeared to for me)?

Changed March 10, 2011 08:35PM UTC by scottgonzalez comment:6

resolution:	wontfix
status:	closed → reopened

Changed March 10, 2011 08:40PM UTC by scottgonzalez comment:7

status:	reopened → open

Shorter patch: https://gist.github.com/864880

Changed March 11, 2011 03:50PM UTC by ddstreet comment:8

Possible test case patch https://github.com/ddstreet/jquery-ui/commit/893c54adf59ff3b13740e4a263fb5ba30f2953aa

Changed March 11, 2011 03:53PM UTC by ddstreet comment:9

resolution:	→ fixed
status:	open → closed

Button: find associated label even without common ancestor. Fixes #7092 - button creation that requires a matching label does not find label in all cases

Changeset: 0b30a1d450f6b7d529732d6a39c5f4057efaaaf4

Changed March 11, 2011 03:54PM UTC by ddstreet comment:10