Ticket #7092 (closed bug: fixed)

Opened 4 years ago

Last modified 4 years ago

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:
Blocking: Blocked by:

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)

Change History

comment:1 Changed 4 years ago by ddstreet

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.

comment:2 follow-up: ↓ 3 Changed 4 years ago by 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.

comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 4 years ago by ddstreet

Replying to 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?

comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 4 years ago by rdworth

  • Status changed from new to closed
  • Resolution set to wontfix

Replying to ddstreet:

Replying to 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.

Last edited 4 years ago by rdworth (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 4 years ago by ddstreet

Replying to rdworth:

Replying to ddstreet:

Replying to 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)?

comment:6 Changed 4 years ago by scott.gonzalez

  • Status changed from closed to reopened
  • Resolution wontfix deleted

comment:7 Changed 4 years ago by scott.gonzalez

  • Status changed from reopened to open

comment:9 Changed 4 years ago by ddstreet

  • Status changed from open to closed
  • Resolution set to fixed

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

comment:10 Changed 4 years ago by ddstreet

Button: find associated label even without common ancestor. Fixes #7092 - button creation that requires a matching label does not find label in all cases (cherry picked from commit 0b30a1d450f6b7d529732d6a39c5f4057efaaaf4)

Changeset: 2d6ad068733a18e5b69815192e7c0f5614410c56

comment:11 Changed 4 years ago by scott.gonzalez

  • Milestone changed from 1.9 to 1.8.11
Note: See TracTickets for help on using tickets.