ibm-js / deliteful

Multichannel (desktop/mobile) UI Custom Elements Library
http://ibm-js.github.io/deliteful
Other
70 stars 36 forks source link

deliteful/list/List: wouldn't it be better that List doesn't change the default value of `selectionMode` defined by delite/Selection? #286

Closed AdrianVasiliu closed 9 years ago

AdrianVasiliu commented 10 years ago

The purpose of entering this issue (labelled "question") is to open a comparative evaluation of the pros and cons.

wkeese commented 10 years ago

The list rarely is "inactive", the list items typically serve for triggering some action upon selection.

"Triggering some action" is different than "selection", for example clicking "Quick definition" in the menu below doesn't select it, it just executes it. The visualization is also different because items that are truly selected (rather than just executed) often have a checkmark next to them, see for example the "Tool Buttons", "Status Bar", "Navigation bar" menu options here:

screenshot 2014-09-13 07 57 24

But, I suppose that usually with lists you are selecting an item, both for a case like selecting which emails to delete (ex: see screenshots in http://www.tothemobile.com/how-to-use-swipe-to-delete-in-gmail-on-android), and for a case like the menu in the left-hand column of "Settings" on an iPad (see screenshots in http://www.macworld.com/article/1150420/ipad_settings.html).

(Likewise with selecting the "Tool Windows" menu option above; it gets "selected" and the submenu opens.)

It seems always better to me to avoid changing the default of the superclass unless there's a strong reason for it.

I agree that's it's usually wrong to change the default value of a superclass's property.

AdrianVasiliu commented 10 years ago

@wkeese

"Triggering some action" is different than "selection"

Yes. My wording wasn't accurate enough. What I meant by "triggering some action" was "allowing the user to trigger some action for specific item(s)", regardless on whether the action is triggered a) when the user touches an item, or b) when he touches a button outside the list which executes an action that holds for a subset of list items indicated by the user (the "selected" ones, typically rendered in a different way than non-selected ones). The item rendering for each situation is typically configurable (with or without checkmarks etc.).

Now, looking a bit more into it:

Anyway, overall, for what matters for the issue discussed in this ticket, it seems we both think most typically list items are selectable.

AdrianVasiliu commented 10 years ago

As a side-remark about how view navigation is implemented in https://github.com/ibm-js/deliteful/blob/master/samples/list/navigationBetweenViews.html, it would seem to me that deliteful should provide some out-of-the-box support for items that trigger ViewStack transitions. The comments in the sample refer to a possible "onAction" on the List, but I wonder if we shouldn't have an even easier to use solution for the pattern of items that trigger view navigation. It is however not obvious to me how the interoperation between List and ViewStack should be designed - we are very far from the system that was in place in dojox/mobile, and in part for good reasons... Might be worth entering an issue for it?

AdrianVasiliu commented 10 years ago

(view navigation) Might be worth entering an issue for it?

I suddenly remembered that long ago it was decided that this feature would be left for dapp, if I understood well. If this still holds, it may still leave the question open about whether we should provide samples with view navigation implemented without dapp.

cjolif commented 10 years ago

@sbrunot @pruzand I would be tempted to change this. I'm pretty sure changing the list selection default is causing more harm than benefit. Do you have an objection?

sbrunot commented 10 years ago

@cjolif: yes, two objections:

  1. The current default doesn't cause any harm
  2. The list widget is not, by default, a selection list. Hence the default value.

Another option, not to change the default value, is to not import the delite/Selection mixin in the List widget and let the user mix it if he needs a selection list.

cjolif commented 10 years ago
  1. It does cause harm as mentioned above both from an app dev perspective (different default value in subclasses is confusing for them) and in the particular case of Adrian. Note that otherwise I would not even bother discussing this...
  2. can you clarify your point here? it sounds like you are saying we should not change the default because it is the default...
sbrunot commented 10 years ago

@cjolif I really don't see the pain point of 1. ? The List clearly states that its default value for selectionMode is "none", so the fact that it inherits from delite/Selection that has a default value of "single" doesn't seem relevant to me here: if you look at the deliteful/list/List jsdoc, it clearly states that the default value for selectionMode is "none"... You don't see any other default value anywhere.

The real pain point for which this issue has been opened, as I understand it, is the last one in Adrian's list:

Also the current situation makes it harder to reuse the List in a widget such as the ComboBox (under development). For the ComboBox, the "none" selectionMode default value is a misfit, in particular in the use-case when the user passes a List instance (instead of the List being created by the combo).

From what I've discussed with Adrian, a developer can define a ComboBox by providing its own List instance. As I explained to Adrian, I think that the ComboBox list should have an aria role of "listbox". That means that a user providing a List instance (or the ComboBox widget itself) must set the property isAriaListbox to true on the list (in this case, the selectionMode is set to "single" by default).

To solve this pain point, the solution is not to change the default value of selectionMode per se, but to change the default aria role of a List. Or to have different list widgets instead of a single multi faced one. My point of view is: provide a widget with simple default values, let the user change this default values if he wants to have more features. If we set the selection mode to something else than "none" on a list, clicking a list item results in a visual feedback of the list item being selected. What is the sense of that, unless you explicitly want to use the list to select things ? For me, this should not be the default behaviour.

For 2., yes, my point is not to change the default because it is the default.

@edurocher you've used the List widget to develop the deliteful sample, what's your feedback on the default values for the widget properties ?

AdrianVasiliu commented 10 years ago

The real pain point for which this issue has been opened, as I understand it, is the last one in Adrian's list [the needs of the ComboBox]

Of course, the needs of the ComboBox are my main personal motivation for raising this issue. However, I think this is not the only concern.

As I explained to Adrian, I think that the ComboBox list should have an aria role of "listbox". That means that a user providing a List instance (or the ComboBox widget itself) must set the property isAriaListbox to true on the list (in this case, the selectionMode is set to "single" by default).

The ComboBox makes sense with both "single" and "multiple" for selectionMode (but not with "none"). Differently, it simply doesn't make sense with List.isAriaListbox at its default value (false). Hence, in my current code, it is forced to true by ComboBox regarless of the initial value on the list instance (and will be documented as such).

My point of view is: provide a widget with simple default values, let the user change this default values if he wants to have more features.

If I'm not mistaken, if we'd apply this principle it would lead to isAriaListbox being true (listbox role) by default, instead of the current default which is false (grid role), isn't it?

AdrianVasiliu commented 10 years ago

And, to explain better the motivation for the ComboBox, we'd like to be able to avoid defining a selectionMode property on the ComboBox itself (and inheriting from delite/Selection too). If List's selectionMode is "single" by default, we can aford to just document that the user shouldn't set "none" on its List instance, and we don't need that the combo sets the selectionMode of the list.

AdrianVasiliu commented 10 years ago

a user providing a List instance (or the ComboBox widget itself) must set the property isAriaListbox to true on the list (in this case, the selectionMode is set to "single" by default).

List's current mechanism for handling this interdependency between List's isAriaListBox and selectionMode (https://github.com/ibm-js/deliteful/blob/master/list/List.js#L183) is such that

new List({selectionMode: "none", isAriaListbox: true});

leads to selectionMode being set to "single" (as side-effect of isAriaListbox being set to true), while reversing the order of the ctor arguments:

new List({isAriaListbox: true, selectionMode: "none"});

leads to TypeError: selectionMode 'none' is invalid for an aria listbox, keeping the previous value of 'none'.

I think the order of arguments shouldn't matter. Maybe the deep issue here is that the aria role shouldn't be coupled with the selection mechanism - the "listbox" aria role would then be usable with selectionMode = "none" and the side-effect of property setters would be avoided.

cjolif commented 10 years ago

I think the order of arguments shouldn't matter.

Right that's basically what computeProperties is supposed to deal with, that is re-conciliating properties in a consistent manner.

Maybe the deep issue here is that the aria role shouldn't be coupled with the selection mechanism

I'm not sure but I suppose some aria roles are or are not compatible with all selection modes, thus the coupling?

AdrianVasiliu commented 10 years ago

I'm not sure but I suppose some aria roles are or are not compatible with all selection modes, thus the coupling?

Right, the listbox role seems to imply single or multiple selection modes. So never mind on this point.

cjolif commented 10 years ago

After more discussions and thoughts about what would be the typical use of List, I still think selectionMode="single" is the right default.

In theory we have 3 types of modes:

  1. plain readable list, no action on the item (I have not seen this a single time in a customer application)
  2. actionable list, you action an item but don't really "select it" (like this triggers a transition to another view typically)
  3. selection enabled list

In practice and considering current implementation which does not really provide an "actionable" mode, I think 2. will usually be implemented using selection (3.) for several reasons:

I think cases 2. and 3. are the vast majority of the cases where a List will be used with deliteful. Based on this and the fact I still did not hear a strong reason why 1. is the right default I suggest we proceed with the change.

sbrunot commented 10 years ago

The current default is 2., as implemented in samples. Hence the default values for List's selection mode. Note that providing an handler at the list level to implement a default "action" was provided in early versions of the List widget, but it was removed considering that this is easy enough to implement using the available methods of the list. It would be really simple to add back the action handler registration method to the list.

AdrianVasiliu commented 10 years ago

The current default is 2., as implemented in samples. Hence the default values for List's selection mode.

It might be my fault, but I must admit I don't understand what you mean here (what is the argument that you bring).

edurocher commented 10 years ago

As a side note, I modified the tutorial to use a selection-change event instead of click to trigger the details view. The tutorial's list is now set in selectionMode="single" mode, so if this becomes the default the tutorial must be updated and the setting removed.

edurocher commented 10 years ago

What about the default value for isAriaListbox then? This is not strictly necessary, just raising the question since both properties are (at least loosely) related.

AdrianVasiliu commented 10 years ago

If you feel the listbox role would be a better default than the grid one, I feel the same ;-)

Now, I wonder if, additionally, it wouldn't be better to replace the current boolean property (isAriaListbox) by a multi-choice string property. This would make clearer the role which holds for the current isAriaListbox == false, and would support the addition of a third role, if this would be later necessary.

cjolif commented 10 years ago

If you feel the listbox role would be a better default than the grid one, I feel the same ;-)

Then I guess @edurocher you should go for it. But then please be very careful in changing, tests, documentation, samples accordingly.

Same thing for selectionMode, I think we should take that opportunity to clear up samples to leverage it (and do other cleanings if necessary).

it wouldn't be better to replace the current boolean property (isAriaListbox) by a multi-choice string property.

Do we really foresee a third role? If not, I suggest we don't spend time on that.

AdrianVasiliu commented 10 years ago

A "'list" role exists (http://www.w3.org/TR/wai-aria/roles#list), apparently close to selectionMode=="none". That said, I don't know how likely it is that we'd need adding roles (this "list" role or some other).

wkeese commented 10 years ago

Why don't you just get rid of the isAriaListBox property completely? It's redundant with the role property, which is basically what you are already describing with your comment:

wouldn't be better to replace the current boolean property (isAriaListbox) by a multi-choice string property.

AdrianVasiliu commented 10 years ago

Right, except that the user wouldn't do list.role = "grid" (or "listbox") but list.setAttribute("rule", "grid"), isn't it?

AdrianVasiliu commented 10 years ago

(and we'd still need to set a default role, unless we want to count on users setting a role by themselves, without any default - which wouldn't be nice)

cjolif commented 10 years ago

I think we should just stick for now to the changes are that needed to make the list easily usable to the most common use-cases (i.e. changing selectionMode, possibly adjust aria default role) and that are more or less related to the initial ticket. Other changes should be dealt with in a separate issue, in particular to let @sbrunot a chance to comment when he will come back. Thanks.

wkeese commented 10 years ago

Right, except that the user wouldn't do list.role = "grid" (or "listbox") but list.setAttribute("rule", "grid"), isn't it?

I meant for users to do list.role = "grid". I forgot that role needs to be set as an attribute for screen readers, but I would still make a role property and then reflect it to an attribute in refreshRendering().

I think we should just stick for now to the changes are that needed to make the list easily usable to the most common use-cases (i.e. changing selectionMode, possibly adjust aria default role)

We can do that, but the problem is that it will require users to update their code twice, the first time to add isAriaListBox=false and then the second time to change that to say role=grid.

edurocher commented 9 years ago

I am preparing a PR with selectionMode="single" and isAriaListbox=true, and I wanted to share some slight doubts about this:

actionable list, you action an item but don't really "select it" (like this triggers a transition to another view typically) ... I think 2. will usually be implemented using selection

I did that (for example in https://github.com/ibm-js/deliteful/blob/master/samples/list/navigationBetweenViews.html) but I am not so sure this is good. The fact that the last selected item in each list stays selected is a bit disturbing in my opinion. Comparing with the dojox/mobile List, the item was selected but then was deselected after a short time.

I know that one of the arguments for using selection instead of clicks was multi-channel, but let's consider the case of view navigation on a phone, do we really want the list item corresponding to a view to stay selected? It seems more complicated than that in my opinion.

cjolif commented 9 years ago

Ok. So maybe we should sleep on it a bit more as it seems you finally think this is problematic. Do you have an alternative solution on how to move forward on this as it seems neither is ideal? Maybe we are missing something like a "true" action event/state?

edurocher commented 9 years ago

I will look again at what dojox/mobile did exactly. Meanwhile I created PR https://github.com/ibm-js/deliteful/issues/339 that changes selectionMode and isAriaListbox defaults.

edurocher commented 9 years ago

So looking at https://github.com/dojo/dojox/blob/master/mobile/ListItem.js#L299 and https://github.com/dojo/dojox/blob/master/mobile/_ItemBase.js#L249, the Dojo Mobile list could either select items permanently or just keep them selected for a specified duration on click. Maybe we should do the same in the new List?

AdrianVasiliu commented 9 years ago

or just keep them selected for a specified duration on click

Well, a code comment in dojox/mobile's _ItemBase says: "Before transitioning, we want the visual effect of selecting the item.". So it seems to me real purpose is just in terms of visual effect. To get the effect, it actually sets the item in selected state (temporarily). Now, do we want for deliteful's List that the user gets selection-change events for this temporary visual effect? Maybe it wouldn't make much harm, but semantically it would mean the user would be encouraged to trigger an action when the item gets (temporarily) selected, while ignoring the deselection (at the end of the specified duration).

Overall, this would seem confusing to me. Hence I'd rather see real selection distinct from graphic feedback when triggering actions (although the default could be that it provides the same graphic feedback in both cases). Which would bring us to Christophe's point:

Maybe we are missing something like a "true" action event/state?

edurocher commented 9 years ago

Yes, for me what it really missing in the current deliteful/List is a visual effect when selectionMode="none" and the user clicks an item. We could do something similar as dojox/mobile and temporarily set the d-selected class on the item just to get the visual feedback, or another class like d-pressed and by default use the same style as d-selected. This should be completely separate from selection, and we definitely don't want selection events to be fired in that case.

So, I am not sure we miss something functionally, unless you think it is not OK to just use click events?

AdrianVasiliu commented 9 years ago

That said, while "selection" and "action" c/should be distinct concepts, I don't think they should be necessarily exclusive. I mean, we should for instance allow (as a configurable option) the item to become selected (and stay selected) once a click triggers an action. Let's say we'd have basically 2 modes:

  1. Click produces a visual effect on the item (by default, similar to selection, but not real selection), and emits an action event. The visual effect (say, a CSS class corresponding to an "actioned" state of the item) can be, optionally, either temporary (specified delay) or permanent (but see below for cases when a user action can remove it).
  2. Click selects the item, and emits an action event (plus the existing visual effect of selection).

Now, in fact the options that hold for selection: single/radio/multiple seem to also hold for the "visual effect + action event" mode, isn't it? For instance, the user may want to set in "actioned" state more than one item - "actioning" a second item doesn't necessarily imply removing the "actioned" state of the first item - the graphic feedback may need to stay for both items. But once we want to support multiple "actioned" items, it means we need an API such as an array of actionedItems, and an API to put items in "actioned" state programmatically - which would be the exact counter-part of delite/Selection's API... As we may not want to add yet another set of distinct APIs, it would seem we do need to leverage the existing delite/Selection also for covering use-cases such as a click producing a temporary visual effect.

By chance, there's no point in having at the same time multiple "actioned" items and temporary visual effect. Thus, we may be able to go this way:

edurocher commented 9 years ago

I don't really see the purpose of an action event, as we already have click. Or is it for accessibility? But isn't there already a mechanism to fire clicks with the keyboard?

AdrianVasiliu commented 9 years ago

Our comments crossed, I think again at it in the light of yours...

We could do something similar as dojox/mobile and temporarily set the d-selected class on the item just to get the visual feedback, or another class like d-pressed and by default use the same style as d-selected.

So this visual feedback would be necessarily temporary? As soon as the user wants the item to stay "highlighted" he would need to turn selectionMode from "none" into radio/single/multiple? If you think the visual feedback could optionally be permanent, we'd face the issue that I mentioned: radio/single/multiple modes and a delite/Selection-like API would be needed.

I don't really see the purpose of an action event, as we already have click. Or is it for accessibility?

I'm not sure about the accesibility concerns. I'd rather see semantic concerns. Currently, List's view navigation sample sets 2 handlers:

list.on("click", function (event) {
    actionHandler(event, list);
});
list.on("keydown", function (event) {
    if (event.keyCode === keys.SPACE) {
        actionHandler(event, list);
    }
});

Abstracting both into an "action" event would at least avoid this duplication. Whether there would be more advantages... and whether we'd better provide a mechanism to map key events to action events... these are currently open questions in my mind.

edurocher commented 9 years ago

So this visual feedback would be necessarily temporary?

Not necessarily, but most of the time yes. It seems a bit of a corner case to have permanently "actioned"/pressed items... Selection seems the right way to do that.

I really think of this "pressed" state for items as the pressed state of a button.

Regarding the two event handlers (click and keydown), obviously we/users should not have to do that. Is there anything in delite for generating click events using the keyboard already, or does it have to be done by each widget? (@wkeese?)

AdrianVasiliu commented 9 years ago

I really think of this "pressed" state for items as the pressed state of a button.

Okay, so a temporary feedback. So, users who want the "actioned" item to stay highlighted would instead rely on the selection mechanism.

wkeese commented 9 years ago

Is there anything in delite for generating click events using the keyboard already, or does it have to be done by each widget? (@wkeese?)

That's what the delite/a11yclick module is for.

AdrianVasiliu commented 9 years ago

That's what the delite/a11yclick module is for.

Indeed. As far as I can see, it is currently not used by any of our widgets. (If List would start using it, the code at https://github.com/ibm-js/delite/blob/master/KeyNav.js#L421 might matter)

Otherwise, still wondering if a "pressed" mechanism that holds only for selectionMode="none" is globally a good thing, or whether it could also be combined with the single/radio/multiple selection modes.

edurocher commented 9 years ago

Pushed to 0.5.0 as there is no consensus yet.

cjolif commented 9 years ago

As there is still no consensus I propose we close this one at least for now, however I would like the List documentation (and sample?) to better expose the two choices here for navigation (between selection & click).