Setting target release to current version for consideration and review. please
do not push non-RFE bugs to an undefined target release to make sure bugs are
reviewed for relevancy, fix, closure, etc.

This is actually a common problem in many dialogs in the project, specifically with the "cancel" button (but also other fields that are malordered, tab-wise). Some infrastructure should probably be implemented to force consistent tab ordering. This is possibly an RFE.

This is an automated message.
Re-targeting all non-blocker bugs still open on 3.4.0 to 3.4.1.

This is an automated message.

oVirt 3.4.1 has been released.
This issue has been retargeted to 3.5.0 since it has not been marked as high priority or severity issue, please retarget if needed.

from bug 903558, Comment #11 [Vojtech]:

"""
...
- AbstractModelBoundPopupWidget has overridable setTabIndexes method, specific (PresenterWidget) dialogs must override this method and take care of assigning tab indexes to focusable (input) elements
- if not done properly, can result in "jumpy" focus change behavior ...
- notes: [this needs to be a] *** per-dialog-level fix ***, since it deals with specific elements of specific dialogs
...
"""

@Vojtech: Lior's comment #2 ("Some infrastructure should probably be implemented to force consistent tab ordering") contradicts what you said in bug 903558, Comment #11 (quoted above). 
Is there *any* infrastructure, at *any* level, that we can/should introduce in order to eliminate, or at least ease applying the correct tab-behavior per dialog?

(In reply to Einav Cohen from comment #5)
> @Vojtech: Lior's comment #2 ("Some infrastructure should probably be
> implemented to force consistent tab ordering") contradicts what you said in
> bug 903558, Comment #11 (quoted above). 
> Is there *any* infrastructure, at *any* level, that we can/should introduce
> in order to eliminate, or at least ease applying the correct tab-behavior
> per dialog?

(CC'ing Lior)

We already have infra "to force consistent tab ordering" in form of following overridable methods:

1, org.ovirt.engine.ui.common.view.popup.AbstractModelBoundPopupView#setTabIndexes (views which embed content UI directly, i.e. UI is part of the view)

2, org.ovirt.engine.ui.common.widget.uicommon.popup.AbstractModelBoundPopupWidget#setTabIndexes (separate reusable widgets which are accepted by views as their content UI)

So for any dialog (whose view class extends either 1, AbstractModelBoundPopupView or 2, AbstractModelBoundWidgetPopupView), developer should override appropriate setTabIndexes method and configure tab indexes on specific input elements.

Infra logic to assign tab indexes [1] first asks view to configure tab indexes on specific input elements (passing 0 as the initial value), then configures tab indexes on dialog (footer) buttons. If the view doesn't implement setTabIndexes() properly, behavior such as "pressing TAB in middle of dialog jumps to dialog button" occurs (as mentioned in BZ description).

[1] org.ovirt.engine.ui.common.view.popup.AbstractModelBoundPopupView#updateTabIndexes

For an example of how to override setTabIndexes() see: org.ovirt.engine.ui.webadmin.section.main.view.popup.host.HostPopupView#setTabIndexes

I have an idea, though. We could develop small, annotation-based infra (utilizing GWT deferred binding feature) to make configuring tab indexes a lot easier. For example, in HostPopupView:

  // existing annotations

  @WithTabIndex    // tab index value reflecting field declaration order
  // -or-
  @WithTabIndex(3) // tab index value specified directly

  ListModelListBoxEditor<StoragePool> dataCenterEditor;

@Lior/Einav, what do you think?

Discussed this idea with Alex, API (usage) & implementation would be very similar to our existing "ID handler" framework (which assigns DOM element IDs).

Declarative approach (annotation) is more readable/maintainable than imperative approach (method/code) so I think we should aim for this improvement.

What I meant by infrastructure was different. I don't see any reason why a developer would need to override the tab indexing manually - there is always exactly one predictable order, which is traversing fields primarily left to right and then top to bottom. There likely won't be a dialog where this isn't right.

There is a question about left tabs, I feel that they should follow all the widgets in the main section of the dialog - but either way this behavior should be consistent across the system (so again no need to implement it on a per-dialog basis).

Moreover, I don't even see why annotation should be needed - all the layout information already exists in the DOM hierarchy. In this respect, I would expect a solution similar to what I suggested in Bug 1123620, that would require zero manual intervention.

And lastly, such a solution should take into account dynamic addition/removal of widgets (e.g. when adding/removing rows in a custom properties widget) while maintaining predictable tab ordering.

regardless if this is some magic fancy framework or just a guideline how to use existing stuff, we need to unify the approach and make sure all new changes are following that

(In reply to Lior Vernia from comment #8)
> What I meant by infrastructure was different. I don't see any reason why a
> developer would need to override the tab indexing manually - there is always
> exactly one predictable order, which is traversing fields primarily left to
> right and then top to bottom. There likely won't be a dialog where this
> isn't right.

IMHO, that's quite narrow definition of the problem. IIUC, you're suggesting to traverse dialog's content pane by means of its DOM tree structure. What about input fields that are dynamically added to / removed from dialog's content pane? What about input fields for which we don't want tab index assigned?

In general, I believe that auto-magical "do it for me" logic is. in many ways, inferior to developer's intervention. This is why I proposed a simplification of the current (setTabIndexes method override) infra -> using annotations to simply mark relevant input fields (widgets). It still allows developer to control which fields have tab index assigned, and could also support dynamic ones as well (this wasn't part of my proposal, though).

> There is a question about left tabs, I feel that they should follow all the
> widgets in the main section of the dialog - but either way this behavior
> should be consistent across the system (so again no need to implement it on
> a per-dialog basis).

Hm, you mean that pressing TAB while on last input field within left-tab A should switch into left-tab B and focus its first input field, correct? (In other words, cycling focus not only within a single left-tab content, but across all left-tabs.)

If so, I agree, it would be a nice thing to have, but technically I believe that this is out of scope for this RFE.

> Moreover, I don't even see why annotation should be needed - all the layout
> information already exists in the DOM hierarchy.

When utilizing some infra, developer's intervention (essentially infra use customization) promotes flexibility, whereas a "do it for me" logic generalizes the whole process and therefore goes against flexibility. As I wrote above: what about input fields that are dynamically added to / removed from dialog's content pane, what about input fields for which we don't want tab index assigned? Solving this on a level too high results in bloated generalized logic. Solving this on lower level allows a balance between infra complexity and infra usage burden (how hard it is to use the infra).

> In this respect, I would
> expect a solution similar to what I suggested in Bug 1123620, that would
> require zero manual intervention.
> 
> And lastly, such a solution should take into account dynamic
> addition/removal of widgets (e.g. when adding/removing rows in a custom
> properties widget) while maintaining predictable tab ordering.

The only way I see on how to handle dynamic addition / removal of fields, in case of using "do it for me" general logic, is using some DOM structure change detection [1], which detects any structural change in dialog's content and re-scans it again to assign new tab indexes. IMHO this is far from being both optimal and appropriate. (If you believe otherwise, or know a solution that I have missed, please elaborate.)

[1] this is generally tricky to do, some references:
http://stackoverflow.com/questions/7692730/dom-mutation-event-in-jquery-or-vanilla-javascript
https://github.com/snesin/jcade
http://www.w3.org/TR/DOM-Level-3-Events/#event-type-DOMSubtreeModified

(In reply to Michal Skrivanek from comment #9)
> regardless if this is some magic fancy framework or just a guideline how to
> use existing stuff, we need to unify the approach and make sure all new
> changes are following that

Agreed, I'll work on this and will update you guys.

1. Please give an example where a UI field is editable, visible and enabled, but shouldn't be reachable by pressing tab.

2. DOM hierarchy is updated even when new fields are added dynamically, isn't it? So traversing DOM elements should solve that problem - whenever TAB is pressed, find the next element that is Focusable, visible and enabled. The only problem remaining is how to map DOM elements to GWT classes (e.g. to find out if something is Focusable). Maybe this isn't needed and everything can be checked at the level of a DOM element, otherwise we can discuss where this map can be implemented.

3. Concerning left tab - I meant that pressing TAB when focused on the last element of a dialog will switch focus to the left tab panel, where hopefully SPACEBAR can be used to "click" a different left tab. I didn't mean to simulate a click and change tabs just by pressing TAB. I think this will take care of itself if something along the lines of (2) is implemented (together with a small fix to take care of cyclical TAB ordering, i.e. taking care of what happens when reaching the last element).

(In reply to Lior Vernia from comment #11)
> 1. Please give an example where a UI field is editable, visible and enabled,
> but shouldn't be reachable by pressing tab.

For this combination, there is no such example. Editable and enabled are logically equivalent for an input field, visible means attached to active DOM. Under these conditions, UI input field should always have a tabindex assigned.

What I tried to point out are cases when:
* visible input field(s) change status from editable to non-editable or vice versa (readonly fields shouldn't have a tabindex assigned)
* previously invisible input field(s) now become visible (imagine these appear in the middle of dialog's content)

In other words, single "traverse DOM hierarchy" call is not enough, we still need to adapt tabindexes according to dialog's (actively visible) content.

> 2. DOM hierarchy is updated even when new fields are added dynamically,
> isn't it? So traversing DOM elements should solve that problem - whenever
> TAB is pressed, find the next element that is Focusable, visible and
> enabled. The only problem remaining is how to map DOM elements to GWT
> classes (e.g. to find out if something is Focusable). Maybe this isn't
> needed and everything can be checked at the level of a DOM element,
> otherwise we can discuss where this map can be implemented.

Aha, so you mean something like blur handler attached on each DOM input element - once called, locate "next" logical DOM input element and focus it via DOM API. Is my understanding correct?

If so, we don't even need tabindex HTML attribute, what I described above is programmatic (callback-based) focus handling. I am not sure this is a good thing to do in general, since relying on tabindex HTML attribute is much easier than any kind of programmatic approach.

(Once you go down to DOM element level, I'm not aware of any way to go up to GWT widget level.)

> 3. Concerning left tab - I meant that pressing TAB when focused on the last
> element of a dialog will switch focus to the left tab panel, where hopefully
> SPACEBAR can be used to "click" a different left tab.

Should be possible, DialogTab uses FocusPanel.

> I didn't mean to
> simulate a click and change tabs just by pressing TAB. I think this will
> take care of itself if something along the lines of (2) is implemented
> (together with a small fix to take care of cyclical TAB ordering, i.e.
> taking care of what happens when reaching the last element).

Sure, but for starters, just solving tab cycling within a single dialog tab (including focus on left-handed tabs) should be enough.

OK, I'll try to play with some code and get back to you.

Yes, your understanding is correct, though I'm not familiar with what happens with blur.

Relying on enumerated tab indexes is what adds complexity (including computational) to any reasonable ("do it for me") solution - as a UI element is added you need to re-enumerate everything. So I suggest not to rely on it. Tab indexing is only "easier" in a static case; when elements can be added dynamically (e.g. as with AddRemoveRowWidget) this approach goes bankrupt, in my opinion.

I'm not sure we need to "go up to GWT widget level", can't we tell whether an element is editable, visible and enabled at the DOM level? If we can't, I'm not sure it's that difficult to get back the GWT widget. I Googled it and found one hacky solution, and we also might incorporate a Map<Element, Widget> in our dialogs (this might also be used by what I suggest in Bug 1123620).

(In reply to Lior Vernia from comment #13)
> Yes, your understanding is correct, though I'm not familiar with what
> happens with blur.

By blur I just meant "focus lost on input element", i.e. browser pressed TAB while given input element was focused.

> Relying on enumerated tab indexes is what adds complexity (including
> computational) to any reasonable ("do it for me") solution - as a UI element
> is added you need to re-enumerate everything. So I suggest not to rely on
> it.

Well, enumerating fields eligible for tabindex assignment could be as simple as marking fields with some annotation, infra could provide a way to "register" new input elements as well as re-assign tabindex values. Implementation could be auto-generated (via GWT deferred binding) and infra could expose simple interface for tabindex handling. Well, this was my idea.

> Tab indexing is only "easier" in a static case; when elements can be
> added dynamically (e.g. as with AddRemoveRowWidget) this approach goes
> bankrupt, in my opinion.

I'm not that much skeptical about it, but I agree, with dynamic content, handling HTML tabindex attribute on input elements is more complicated (more than just static assignment of tabindex values).

> I'm not sure we need to "go up to GWT widget level", can't we tell whether
> an element is editable, visible and enabled at the DOM level?

I think we can, we can have a way to detect different kinds of input elements on DOM API level.

visible = style.display != 'none' && document.contains(element)
enabled = presence of some standard HTML attribute (readonly)

> If we can't,
> I'm not sure it's that difficult to get back the GWT widget. I Googled it
> and found one hacky solution, and we also might incorporate a Map<Element,
> Widget> in our dialogs (this might also be used by what I suggest in Bug
> 1123620).

The disadvantage is to keep such Map<Element,Widget> in sync with actual dialog content.

Anyway, I have one more concern: absolutely-positioned elements (using CSS position:absolute).

For example, see this fiddle: http://jsfiddle.net/gp5zbg1s/

First <input> is rendered at the bottom, yet it's the first one in DOM structure. In this case, we'd have to use element.offsetHeight which is further complication.

What do you think? (A way to solve this is to forbid absolute positioning within dialog content, but I don't like this restrictive approach too much.)

Overall, we could use jQuery to do the heavy lifting here, if we decide to follow Lior's idea (jQuery is already provided due to PatternFly integration):
* use jQuery.on to attach blur handler for selector matching all input elements in scope of dialog content
* in that handler, we can use jQuery selector to obtain all (currently visible) input elements, locate current element and just focus the next logical element after it

Still, there's this issue with absolutely-positioned elements. I'm not aware of any - if we detect one, we can exclude it from tabindex handling.

Lior, let me know what you think.

I would have no problem with a solution based on tab indexing, if it won't be the developer having to implement logic to re-assign tab indices whenever a UI field is added dynamically to a dialog.

Anyway, I think the DOM-based solution would require less "special" code per-dialog, so seems preferable to me.

I wouldn't worry so much about absolutely-positioned elements - it sounds very uncommon to me and I don't know of any. Worse case scenario, we can wait for bugs to be filed on such UI fields (while solving all the currently open bugs).

> Still, there's this issue with absolutely-positioned elements.

I just realized that elements can be sorted left-to-right (offsetWidth) and top-to-bottom (offsetHeight) i.e. according to their visual positioning.

So it seems that automatic tabindex handling implemented on DOM level is possible and not too complex after all.

>  I just realized that elements can be sorted left-to-right (offsetWidth) and top-to-bottom (offsetHeight) i.e. according to their visual positioning.

Sorry, I meant: offsetLeft + offsetTop :-)

OK, I played with jQuery and updated my fiddle:

http://jsfiddle.net/gp5zbg1s/1/

This basically shows what I mean by "automatic" tabindex handling.

Shouldn't be too hard to port this into WebAdmin.

PS: the only issue I found is when you press TAB on input3, input1 does NOT receive focus, despite that focus() is called on input1 element. I assume this is due to jsfiddle using iframes to render different parts (CSS, JS, etc.) so this should not happen in our GUI.

Lior, can you review above fiddle?

Basically looks okay, but:

1. I think you need to sort first by top-to-bottom, then left-to-right (to have left-to-right the primary criterion).

2. If you're going according to tab index, then I would expect the initialization code (allocating tab indices) to be separate from the code handling TAB presses. Re-assigning indices should only happen following GWT binding and dynamic addition of widgets, not on TAB press.

3. If you want all the logic to happen on TAB press, then I would go with the DOM traversal logic rather than tab index allocation. I.e. look for the next sibling element that is visible+editable+enabled, if there isn't any go the parent and then its siblings, and so forth.

Thoughts?

> 1. I think you need to sort first by top-to-bottom, then left-to-right (to
> have left-to-right the primary criterion).

Yes, makes sense, agreed.

> 2. If you're going according to tab index, then I would expect the
> initialization code (allocating tab indices) to be separate from the code
> handling TAB presses. Re-assigning indices should only happen following GWT
> binding and dynamic addition of widgets, not on TAB press.

Not sure what you mean.

In my fiddle (see comment #18) I added focusout handler to all (current and possibly future) input elements via jQuery.on function. This gets called whenever some input element loses focus - we fetch all currently visible input elements, sort them (left-to-right, top-to-bottom), compute index of current element (one which just lost focus), shift index to next element and focus next element.

There is no tabindex initialization code, there's just focusout handler that scans all currently visible input elements and does the focus shift to next element. This ensures that even if some new input element was added in the meantime, it will be detected inside focusout handler.

If I misunderstood, feel free to fork my fiddle and update it.

> 3. If you want all the logic to happen on TAB press, then I would go with
> the DOM traversal logic rather than tab index allocation. I.e. look for the
> next sibling element that is visible+editable+enabled, if there isn't any go
> the parent and then its siblings, and so forth.

This is already solved by using jQuery selector.

Assume we have something like:

  <div> <!-- dialog content pane root element -->
    <input />
    <div> <input /> </div>
    <div> <div> <input /> </div> </div>
  </div>

Then we can still select *all* input elements via jQuery selector (rooted under dialog content pane root element), regardless of their position in DOM structure.

In other words, in my fiddle, you can change this:

  <input type="text" id="input1" value="input1" />

to this:

  <div> <input type="text" id="input1" value="input1" /> </div>

and it should still work, so I don't see any problem here.

> Re-assigning indices should only happen following GWT
> binding and dynamic addition of widgets, not on TAB press.

Forgot to mention - there's no "re-assigning" logic, there's just logic to detect all currently visible input elements and shift focus to next "logical" element. This logic happens only when focus was lost due to TAB press, so mouse click still works (remember that element loses focus also when you click other element).

What I meant in article (2) was that what you're doing is an overkill - you only need to re-compute tab indices whenever the elements in the document change, not on each TAB press. TAB press can just shift focus to the next element by tab index, without re-computing tab indices.

Re-assign tab indices == sorting the elements. I meant that you could sort only when the content of the document changes, remember this order, and then you don't have to sort each time TAB is pressed.

OK, I see what you mean.

> when the content of the document changes

This is the tricky part.

We could use [1] but that's not supported in IE < 11 [2]. Note that [3] is officially deprecated and should be avoided.

[1] https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
[2] http://caniuse.com/mutationobserver
[3] https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Mutation_events

We could use hacks like setup repeated timer to check root node's innerHTML length, however this is obviously far from being efficient.

[Lior, if you know any good way of detecting DOM changes, please share. As I see it, there's no reliable/standard way to do this (taking IE8+ into account) and I don't want to resort to using ugly/inefficient hacks. If you have some better solution to detecting dynamic DOM changes, feel free to update my fiddle from comment #18.]

And this is why I implemented focus switch logic inside focusout handler. It may seem as overkill, I agree with that, but it works and the code is relatively simple.

I think we can override onLoad() (or alternatively add an onAttach handler) in our common widgets (e.g. AbstractValidatedWidget) to trigger this re-computation method (which might be declared and implemented at the level of a dialog view).

However, first I would run this computation following GWT's binding, and only then enable it for triggering by dynamically-added widgets (as most dialogs contain only static elements and shouldn't suffer from the computational cost of sorting on addition of each element).

This is an automated message.
This Bugzilla report has been opened on a version which is not maintained anymore.
Please check if this bug is still relevant in oVirt 3.5.4.
If it's not relevant anymore, please close it (you may use EOL or CURRENT RELEASE resolution)
If it's an RFE please update the version to 4.0 if still relevant.

Target release should be placed once a package build is known to fix a issue. Since this bug is not modified, the target version has been reset. Please use target milestone to plan a fix for a oVirt release.

Closing old tickets, in medium/low severity. If you believe it should be re-opened, please do so and add justification.