Fix li/td click targets to avoid nested li/td's

[fvgoto]

Fixes bug of multiple events triggered if the model views contain 'li' or 'td' of their own. [See first commit for detailed description]

[dgbeck]

Hello @fvgoto !

Thanks for the PR!

I believe for the table case we need a tr in there, since the collection view's el is the table element, so we'd have:

            "mousedown > li, tr > td" : "_listItem_onMousedown",
            "dblclick > li, tr > td" : "_listItem_onDoubleClick",

If that seems right to you go ahead and update and I'll merge.

Thx!

[fvgoto]

Oh, indeed! (Didn't test the table version ;-) ).

But: it's rather > tr > td (leading >).

cf. your selector in _removeEmptyListCaption at src/backbone.collectionView.js line 392.

BTW: you don't wrap in additional <tbody>, thus it's fine. (src/backbone.collectionView.js line 381).

[dgbeck]

Uff, you bring up a good point about the tbody element. I'm quite certain that element is inserted automatically by some browsers (like chrome), so I don't think > tr > td will work.

Another way to tackle this issue would be in line 537, in _getClickedItemId, where we do the check to make sure the click corresponds to an element in the collection view. I think we could generalize that check, which now looks like this,

if( clickedItemEl.closest( ".collection-list" ).get(0) !== this.$el.get(0) ) return;

to consider any ul (or table) element:

if( clickedItemEl.closest( this._isRenderedAsList() ? 'ul' : 'table' ).get(0) !== this.$el.get(0) ) return;

Does that work for your case?

[fvgoto]

Nope, code analysis says and tests also fail:

var clickedItemId = this._getClickedItemId( theEvent );   //==> *call to the code you mention*
if( clickedItemId )
{
   //...
}
else
   // the blank area of the list was clicked
   this.setSelectedModels( [] );   //===> *WRONG RESULT*

(up to src/backbone.collectionView.js#L935)

So no, our click was not on the "blank area of the list" and thus should not emptify selection.

---

UPDATE

I did some (uncommitted) solution, but reverted as I found that

        _listBackground_onClick : function( theEvent ) {
            if( ! this.selectable ) return;
            if( ! $( theEvent.target ).is( ".collection-list" ) ) return;

            this.setSelectedModels( [] );
        }

with crucial line at backbone.collectionView.js#L955 did already the same as #L935 described above. I don't see why an event "mousedown li, td" should correspond to // blank area of the list anyway.

Thus, I also removed the else tree, which

• is offending to the nested-li/td resolving
• lacks justification, since for background, selection gets already emptied by _listBackground_onClick Tested with ul and table structure and works for me. (But at the same time, I fixed also some other bugs, which I will commit/comment separately).

So finally, you were closer than what I thought of in a first step ;-)

---

NB: tbody vs not: You've got it for _getContainerEl at backbone.collectionView.js#L521...

[fvgoto]

2 commits with other fixes/cleaning ups in regard to the more global tbody issue (left on this branch/PR since rather connected/follow-up).

---

Further: In _setupSortable: sortableOptions:, an alternative to items : this._isRenderedAsTable() ? "> tr:not(.not-sortable)" : "> li:not(.not-sortable)", could be: items : "> [data-model-cid]:not(.not-sortable)",

This would be more similar to _getVisibleItemEls's: itemElements = this._getContainerEl().find( "> [data-model-cid]:not(.not-visible)" );.

Both selector parts could even be extracted to a jointly used separate method (maybe also for other locations)... NB: I currently left the first alternative because it is a little safer against the issue of ongoing-work branch patch-2-initialize-with-parent (no pull-request yet)...

(Detail-2: the latter .find("> [data... would be clearer with .children("[data...)

[fvgoto]

[NB: dist/ is not yet updated...]

[dgbeck]

HI @fvgoto ! This is looking good to me. However let's please pull out the changes to the wrapping of modelView elements as that is a separate issue altogether. (I'm also inclined to keep that the way it is, since there is no reason to use a table collection view unless your modelView elements are trs.) Thanks!

[fvgoto]

Thank you for all your comments! Let's keep this PR to finish debating and once we agree, I'll do a fresh, clean PR.

For the wrapping thingy, I'm not yet sure if I'll do a (separate) PR - since you're hesitant... I just saw, that it might have an impact on _removeModelView() on L460. (NB: that one is likely buggy as it is - we don't know if we wrapped in li or not...)

[dgbeck]

Hi @fvgoto ,

I'm loosing track of what this PR is about. Let's hit the reset button.. I think your original mini PR was actually spot on to fix the issue. It looks like all browsers insert that tbody element

http://stackoverflow.com/questions/938083/why-do-browsers-insert-tbody-element-into-table-elements

So we can just change the events hash to

"mousedown > li, tbody > tr > td" : "_listItem_onMousedown",
"dblclick > li, tbody > tr > td" : "_listItem_onDoubleClick",

And that should solve the issue described in this ticket.

The clean PR would be helpful.

Thanks!

[dgbeck]

fixed in v1.0.3. Thank you @fvgoto for finding and solving this issue!