:bug: N°5396 - Fix BrowseBrick tree "opening_target" mode for "self" and "new" values

[vbkunin]

Hi! I faced with a small bug in the Browse Brick tree more. The level actions didn't work when <opening_target> was set in self or new value. The reason is that the href attribute is attached to the span element and there are no listeners on it to handle click events. Modal mode works fine because it's based on the bootstrap modal functionality. I swapped the span and a elements and made some changes to the scss/css to keep the current look.

[piRGoif]

Hello Vladimir, thanks for this contribution !

I applied this XML to a standard iTop on the develop branch :

<?xml version="1.0" encoding="UTF-8"?>
<itop_design xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" version="1.7">
  <module_designs>
    <module_design xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" id="itop-portal" xsi:type="portal" _delta="must_exist">
      <bricks>
        <brick id="services" xsi:type="Combodo\iTop\Portal\Brick\BrowseBrick" _delta="must_exist">
          <levels>
            <level id="1" _delta="must_exist">
              <levels>
                <level id="1" _delta="must_exist">
                  <levels>
                    <level id="1" _delta="must_exist">
                      <actions>
                        <action id="create_from_this" xsi:type="create_from_this" _delta="must_exist">
                          <opening_target _delta="define">self</opening_target>
                        </action>
                      </actions>
                    </level>
                  </levels>
                </level>
              </levels>
            </level>
          </levels>
          <browse_modes>
            <default _delta="force">tree</default>
          </browse_modes>
        </brick>
      </bricks>
    </module_design>
  </module_designs>
</itop_design>

And easily reproduced the problem, thanks ! Indeed with your fix the link now works as expected, but I'm not sure if this is the best fix, and won't cause regressions (changing the DOM structure). We'll see during the technical review next week !

[Molkobain]

Discussed during technical review, @Molkobain will review it for next time.

[Molkobain]

Nested anchors are illegal in HTML (see here and here).

With your proposal, in the case the Service description is in HTML and contains an hyperlink (which we encountered in several occasions), we will have those nested anchors.

That being said, even though nested anchors were not working in previous versions of browsers, this seems to work on both Chrome and Firefox (to date versions) now.

With that in mind and the fact that the portal option is indeed not working as-is, we think that this should be considered. The PR will be discussed further more in functional review next week.

In the mean time I'll create the corresponding bug in our bug tracker.

[Molkobain]

Hello Vladimir,

We discussed this during today's review; we don't really want to nest anchors as it is still prohibited by the HTML specs as mentioned on the previous post.

What we would agree to in order to restore that feature is to add an onclick="" attribute on the element with the corresponding JS code to open the href in either the current window or a new tab.

Cheers, Guillaume

[vbkunin]

Hi!

It seems that nested anchors are not only a problem of the proposed solution, since they also exist in the Mosaic view even at several levels (service/subcategory description and the hamburger menu with its items).

I figured that the code of the helper functions SetActionUrl() and SetActionOpeningTarget() assumes that the element should be passed as an argument. And in all cases, this is true, except for the Tree view where is passed. I'm not sure if the RFCs allow the use of href and target attributes for the element, I've never seen this, and it's confusing in my opinion.

As a workaround to the current problem, I suggest adding href and target to and hanging an onclick handler on to delegate these events to . I think a more complete solution requires more complex refactoring outside of this PR.

[Molkobain]

Hello Vladimir,

Thanks for the changes, your new approach seems good regarding the whole refactoring that it would require. Once suggestions are applied it will good for me.

Cheers, Guillaume

[Molkobain]

Hello @vbkunin , any update?

[vbkunin]

Hello @Molkobain ! Sorry for the long wait, I made changes.

[Molkobain]

No worries Vladimir, thanks for the changes!