Replace Fancytree with Vue component in Entity Selector

[cconard96]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-

Sort of a proof of concept for replacing the Fancytree library since the entity selector was the most complex case with DOM recycling (only the visible options are in the DOM and the data is swapped out as the user scrolls). Since Vue already handles this functionality natively, it is not a difficult thing to migrate. The only difficulty I had was minor and involved handling auto-expand of the entity "folders" to display the entities that matched the search while also keeping good performance. In the end, performance seems at least as good as Fancytree when I tested with tens of thousands of entities.

The remaining uses for this library are the Browse view on various search pages and the Browse view in the knowledgebase. Neither case restricts the elements added to the DOM to only what is visible to the user at the time.

• DOM Recycling to maintain good performance on very large entity trees
• Fake scrollbar to reflect the current position in the tree even though only a small number of the "visible" nodes are actually in the DOM
• Loading tree data over AJAX to prevent blocking the main page request
• Search/filter with the parent folders of matches automatically expanding

[orthagh]

knowledgebase

Should be redone entirely anyway but that's a topic for another day.

[cconard96]

knowledgebase

Should be redone entirely anyway but that's a topic for another day.

In what way should the KB be redone? Probably relevant now since I just started the KB UI migration to Twig today. One thing I already had in mind is streamlining the view and edit views depending on complexity. I seem to remember a very old issue or note somewhere about that.

[orthagh]

current:

vs 10.0 :

• having dedicated icon for expanding/collpasing is mandatory
• file icon for entity name is not the better choice (and due to the tree lib) but having none feel empty for the list. Maybe we can reuse stack icon (icon of Entity). Folder icon could maybe use stack-2 icon.
• folders was displayed bold, not mandatory but it helps identifying them

I broke scrolling directly in 5s (just scrolling down a medium list):

I will probably have more to say, but a first pass is required.

[orthagh]

In what way should the KB be redone? Probably relevant now since I just started the KB UI migration to Twig today. One thing I already had in mind is streamlining the view and edit views depending on complexity. I seem to remember a very old issue or note somewhere about that.

A lot of things and difficult to be exhaustive but here is a list of issues:
a big UX review is required. Navigation and usage should be like new tools like Notion, outline, obsidian. Barier between edition and viz should be almost transparent. Browse and Search should not be separate tabs. Comments and revisions system should be integrated inline and not a tab. We should have nested document (and so index/toc page where you can write actual content).
Anyway, this requires specs and probably the rewrite should be probably planned for another version, we have a lot of things to finish before the next release.

[orthagh]

It's better but I still have some issues with virtual dom

Capture.video.du.07-03-2024.15.46.16.webm

[cconard96]

It's better but I still have some issues with virtual dom
Capture.video.du.07-03-2024.15.46.16.webm

I'm not seeing that issue and I'm not sure why the rendering would be so slow for you.

[orthagh]

The repaint matches my mouse scroll speed. I try to redo a test soon.

[orthagh]

After a checkout right now:

Tried to logout/login, same.

[cconard96]

After a checkout right now: ![image](https://private-user-images.githubusercontent.com/418844/317752032-
Tried to logout/login, same.

I can only recreate if I don't do a composer build after checking out the branch and prevent the outdated dependencies message from showing. Something seems to have prevented the component from loading or it failed to be built. There maybe more information in the browser console.

[cedric-anne]

Seems OK. I rebased to ensure that it passes an up-to-date test suite.

[cedric-anne]

E2E tests are failing.

[orthagh]

I tried to test again the pr right now: but I'm blocked because the entity menu doesn't show in the top-right menu

[orthagh]

Seems cache related.
It's fixed on my side, but maybe we should investigate the cache invalidation of Vue components.

[orthagh]

Ok I have another annoying special case .

Within your tree, you must have an entity:

• with children (to permits unfold action)
• also present as direct assignment in user profiles

After unfolding the entity under the root section, the scroll start to behave wrongly (see my previous comment: #16617 (comment))
I suspect you use some HTML id attributes and there's some dom conflicts, but the case above is valid, an entity can be referenced twice (or more) in the tree. This is not something encontred often, but I know some old customers of mine using this direct assignation as a shortcut for some entities. Maybe the unicity of an entry in the dom tree should not only rely on the id of the entity (prefix/suffix with id of glpi_user_profiles).

[orthagh]

Not sure if it is cache or not, but I have a wrap issue:

Additionally: could we move from an HTML table design to a div with flex ?
AFAIK, there is no requirement to continue using table cells.

[orthagh]

There are a few remaining quircks:

• top margin is a bit small (compared to 10.x)
  

• Active entity is not highlighted anymore in the tree

• more complex but everyone misses this feature of active entity. You may have several entities in your rights. One could be a subset of another:
  

  In this case, interacting firstly within the Root tree on the duplicated children and then in the children tree lead to very strange behavior
  
  This is working in 10.0.x

[cconard96]

* [ ]  more complex but everyone misses this feature of active entity. You may have several entities in your rights. One could be a subset of another

The weird behavior is because there are nodes with duplicate keys in this case. Expanding the node for an Entity "fill entity" for example will expand every instance of it in the tree display.

Is the ideal display still how it was before or should we take the opportunity to change it? For example, if I have rights on the root entity recursively and a sub entity, is anything really gained by having that sub entity listed twice?

[orthagh]

It was before

Probably that. The case I showed you is not that useful; it's just typical and clear. A real case could be someone without rights on the root entity but several on different entities. I don't know if it triggers strange behavior in this case, but I think it's safer to consider unique, identical nodes present in different branches.

[cedric-anne]

@orthagh

Should it be included in version 11.0 or postponed?

[orthagh]

I will not push for it myself.
It's rather a critical component for GLPI, and it's currently working fine.

If we can fix the remaining issues and making some extensive functional tests (by hand I mean); I would be OK.
For the moment, it's pretty in favor of "DO NOT merge for 11.0"

[cconard96]

I'm OK with delaying this PR. I don't see a huge need to rush it for 11.0.

[cconard96]

This PR has been restarted nearly from scratch.

• The TreeView component was inlined with the EntitySelector component. I don't want to encourage the reuse of a custom TreeView in other areas over a proper UI component library at this time.
• A lot of code was not really done the "Vue" way, and apparently I've learned a lot in the last 2 years, so it was cleaned up. This includes using jQuery to select elements and also to add event listeners instead of templateRefs and 'v-on/@' directives.

If there are any desired design changes to this feature, now may be the time to discuss them/tell me.

[orthagh]

I will a busy week , but I'll try to make a checkout asap and give my opinion about the current state

[cconard96]

I will a busy week , but I'll try to make a checkout asap and give my opinion about the current state

Current state is UI/performance work only. I may work on the form actions later today.

[orthagh]

The below code should probably be in getEntitySelectorTree method

[cconard96]

Leaving that for last. If this PR is OK, getEntitySelectorTree will be rewritten and the code for the API endpoint can be changed to call it. The temporary code in the AJAX script is the same as what the API uses currently.

[orthagh]

It's difficult to check raw client performance on my side, but here are a few things noted:

• on the same data, 11.0.x grabbed 1.45Mo, while this PR state took only 45Ko 👍 (for 750 entities)
• It's seem a bit faster, but again difficult to say.
• I don't see entries duplication like in the previous version

On UI, even you said it's currently still in progress, another set of remarks:

• Please implement in priority the entity selection, It's hard to check the feature without its main action 😬
• please keep icons (folder at least) for identifying the tree
• “Select this entity and all its children” icons should be .btn-sm
• Those with no children still have the above button but without icon.

[cconard96]

* on the same data, 11.0.x grabbed 1.45Mo, while this PR state took only 45Ko 👍 (for 750 entities)

* It's seem a bit faster, but again difficult to say.

32x less data being sent is huge for mobile/wifi users.

* I don't see entries duplication like in the previous version

Someone literally just commented about in on the forum yesterday, calling it "weird" and I tend to agree. The way a user is granted permission on an entity shouldn't matter to them. If they are given direct permissions on Entity B and Entity C or granted permissions indirectly on B and directly on C, they should still show in the same way:
Entity A

• Entity B
• Entity C

The only reason for it to be different is if permissions are granted directly on both B and C, making them both "roots".

• Entity B
• Entity C

Anyways, I'll check different entity assignments later.

* Please implement in priority the entity selection, It's hard to check the feature without its main action 😬

done

* please keep icons (folder at least) for identifying the tree

This is redundant with the arrow icons, but I restored the stack icons that were decided on as replacements for the folder icon the last time.

* “Select this entity and all its children” icons should be `.btn-sm`

done

* Those with no children still have the above button but without icon.

done

[orthagh]

A last few things;

• please remove .p1 on sub element selection button.
• space a bit the icon in the label (add .me-1)

Regarding icons, it's ok now, but I find it a bit busy. Maybe we can remove icons from no-children entries.

I don't see entries duplication like in the previous version

It was more about a weird bug appearing previously, it's OK now for me.

making them both "roots"

Note, I find it odd also, but this is kind of changes I'm not willing to make.
People impacted by a change on this place will probably be the ones that like this behavior.
It triggers not so often, so I'm pretty in favor to not change anything.

[cconard96]

* please remove `.p1` on sub element selection button.

Be aware that these buttons would no longer meet WCAG AA requirements for minimum target size. The button would have a clickable area of 17x21 pixels instead of 25x25. The minimum for WCAG AA is 24x24 pixels. Especially because it is right next to another interactable element, I don't believe it q2ualifies under any of the exemptions for the guideline.
https://www.w3.org/WAI/WCAG22/Understanding/target-size-minimum.html

* space a bit the icon in the label (add `.me-1`)

done

Regarding icons, it's ok now, but I find it a bit busy. Maybe we can remove icons from no-children entries.

done

Just FYI, I was thinking yesterday about a favorites feature for entities. That would entail having a star icon button next to each entity in the list which would add/remove it from the favorites. Favorite entities would show up above the normal tree in a flat list. Another button would show in the selector to favorite/unfavorite the current entity. In cases where a user has access to hundreds or thousands of entities, odds are they only need to change to a few for 90% of their work.

We could also have it show only the favorites by default if they are available and there are a lot of entities to avoid needing to load the entire entity tree for the user if they just want to use their favorites list.

[orthagh]

I did a few tests regarding the current state, here is the diff:

diff --git a/js/src/vue/EntitySelector/EntitySelector.vue b/js/src/vue/EntitySelector/EntitySelector.vue
index 83f4875898..cebb4398c7 100644
--- a/js/src/vue/EntitySelector/EntitySelector.vue
+++ b/js/src/vue/EntitySelector/EntitySelector.vue
@@ -306,21 +306,21 @@
 
             <div v-if="!loading" class="flexbox-item-grow mt-2 position-relative" :style="`height: calc(30px + ${32 * max_items}px)`">
                 <div class="w-100 h-100 overflow-x-auto overflow-y-hidden">
-                    <ul class="w-100 list-group" @wheel.prevent.stop="onListScroll">
+                    <ul class="w-100 list-group rounded-0" @wheel.prevent.stop="onListScroll">
                         <li v-for="node in visible_in_dom" :key="node.key" :class="`list-group-item p-0 border-0 cursor-pointer`" :style="`${node.selected ? 'background-color: var(--tblr-primary)' : ''}`">
-                            <div :style="{paddingLeft: node.level * indent_size + 'px'}" :data-node-id="node.key" class="text-nowrap d-flex align-items-center">
+                            <div :style="{paddingLeft: node.level * indent_size + 'px'}" :data-node-id="node.key" class="text-nowrap d-flex align-items-center pt-1">
                                 <button v-if="node.children.length" :title="node.expanded ? __('Collapse') : __('Expand')"
                                         :aria-label="node.expanded ? __('Collapse') : __('Expand')"
-                                        class="btn btn-ghost-secondary btn-sm btn-icon p-1 me-1 cursor-pointer collapse-item"
+                                        class="btn btn-ghost-secondary btn-sm btn-icon p-1 cursor-pointer collapse-item"
                                         @click.prevent.stop="onExpandToggleClick(node)">
                                     <i :class="node.expanded ? 'ti ti-chevron-down' : 'ti ti-chevron-right'"></i>
                                 </button>
                                 <div v-else style="width: 25px"></div>
                                 <div role="button" :class="selected_nodes.includes(node.key) ? 'fw-bold' : ''" @click.prevent.stop="changeEntity(node.key, false)" class="d-flex align-items-center">
-                                    <i :hidden="node.children.length === 0" class="ti ti-stack-2 me-1" aria-hidden="true"></i>
+                                    <i :hidden="node.children.length === 0" :class="node.expanded ? 'ti ti-folder-open me-1' : 'ti ti-folder me-1'" aria-hidden="true"></i>
                                     {{ node.label }}
                                 </div>
-                                <button v-if="node.children.length" class="btn btn-outline-secondary btn-sm btn-icon p-1 ms-1"
+                                <button v-if="node.children.length" class="btn btn-ghost-secondary btn-sm btn-icon p-1"
                                         :title="__('Select this entity and all its children')"
                                         :aria-label="__('Select this entity and all its children')"
                                         @click.prevent.stop="changeEntity(node.key, true)" data-bs-toggle="tooltip" data-bs-placement="top">

summary:

• replace stack icon by folder icon (with an opened variation), I found it clearer regarding a tree exploration. It also fixes the vertical alignment of labels. I found the stack icon a bit too much in the component and it doesn't help much with the prettiness of this part, nor the UX. I didn't test with the file icon like currently in 11.0/bf, it may be superflu to have this icon.
• remove the rounded corner when the root entity is selected
• the align-center on icon+label+icon doesn't to work, add a pt-1 to better align the colored selection with the content
• removed again the border on the right btn, I know it's a recent change, but I still found the thing ugly. I remember the discussion about discoverability of the feature, but it still seems odd

[orthagh]

An important point I would like to discuss, if the tree traversal (parent nodes opening) doesn't trigger ajax calls, it's useless imo.
We should either:

• cache entity tree if not already done and provide it directly to the component
• extension of the previous, we may load the full vue component when it's parent dropdown is called, if we want to defer a bit.
• or make ajax requests for each node opening.

[cconard96]

An important point I would like to discuss, if the tree traversal (parent nodes opening) doesn't trigger ajax calls, it's useless imo. We should either:

* cache entity tree if not already done and provide it directly to the component
* extension of the previous, we may load the full vue component when it's parent dropdown is called, if we want to defer a bit.

* or make ajax requests for each node opening.

With 100,000 entities and GLPI in production mode (no debug mode or xdebug either), the AJAX call when the selector is opened takes 800-1000ms for me and the tree is displayed shortly after (half a second maybe). No data is fetched until the selector is opened.

If we add caching so that the clients with huge numbers of entities don't need to transfer so much over the network or have better performance, we need to think about it a little more than just dumping the data in the sessionStorage/localStorage as there is a 5MB limit and the uncompressed entity tree data from GLPI for my 100k entities is 5.7MB. To cache it locally, across pages would mean using indexDB. I don't think there is any reason we couldn't use it though.

The reason I didn't have it load data when each entity node is expanded is because of the added complexity on the server and the search functionality. Additionally, the data is cached on the server in the getSonsOf function which stores direct and indirect child entities so using that cached data only for direct children every time a node is expanded seems to not be efficient.

[cconard96]

I looked more at using IndexedDB for caching the entity tree client-side but I don't think there is enough information on the client-side currently to know when the cache data can be used or if it needs invalidated.