Admin, Orders list: when capturing an order, fix an issue that makes the tooltip invisible (+ errors in the console) #11577

jibees · 2023-09-21T12:25:01Z

What? Why?

Closes [Orders page] Uncaught Error: Missing target element "tooltip" for "tooltip" controller - missing Ship order Icon #10956

We should use cable_ready.inner_html instead of morph to be sure that every stimulus controllers attached to the DOM that is replaced is will connected. Actually, I don't know why, this is empirical. What I saw is that when using morph, the stimulus controller (specially tooltip_controller) were not attached to the DOM (and not disconnected as well) that triggered some errors in the console.

This explains nicely what is happening here: stimulusreflex/stimulus_reflex#314 (comment)

In the context of SR (Stimulus Reflex), the expectation is that you would specify the UI you want to exist in the browser on the server. There's no need for a render stage because the markup is already perfect before it is sent down the wire. There is no build stage, no rehydration. We're not taking over the task of redrawing the UI from the browser.

The tooltip controller was adding some html element to the page on connect, which doesn't play well with StimulusReflex. So we refactored the controller and use a partial to populate tooltip content.

What should we test?

As a hub manager, go to /admin/orders
Capture a payment with the small icon on the right
Once the order has been captured, see that you can display the tooltips for this order (on each action icons)

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

mkllnk · 2023-09-22T05:03:14Z

app/reflexes/admin/orders_reflex.rb

-        morph dom_id(@order), render(partial: "spree/admin/orders/table_row",
-                                     locals: { order: @order.reload, success: true })
+        cable_ready.inner_html(
+          selector: dom_id(@order),
+          html: render(partial: "spree/admin/orders/table_row",
+                       locals: { order: @order.reload, success: true })
+        ).broadcast
+
+        morph :nothing


That's an interesting observation. It sounds like a bug in StimulusReflex to me. I don't like this new pattern but if it works then it's good.

I tried to find any information about why this is happening. And I found an interesting section in the docs:

inner_html completely wipes out any Stimulus controllers present in the replaced DOM hierarchy and doesn't respect the data-reflex-permanent attribute. How can we adapt this so that both morph operations are performed by the morphdom library?
https://docs.stimulusreflex.com/guide/morph-modes.html#real-world-example-pagy-refactoring

So there's a difference between inner_html and morphdom and morphdom seems to keep as much as possible, therefore not disconnecting and reconnecting controllers. I think it tries to update the existing tooltip elements without replacing them.

Maybe a problem of the tooltip controller is that it renders the tooltip on connect and is not aware of data attribute changes after that.

Another thing I found is:

You cannot change the attributes of your morph target.
https://docs.stimulusreflex.com/appendices/troubleshooting.html#you-cannot-change-the-attributes-of-your-morph-target

So either solution means that the tr attribute for order state never updates. This is probably a pre-existing bug. But looking at morphdom it seems to be able to update attributes on the target. So, I'm not sure where that limitation comes from or if the warning is only there in case StimulusReflex decides to replace inner_html only (which it seems to do in some cases).

Maybe we should test if the state change is a problem and then we need to use outer_html. Otherwise we are good.

I think this describe quite neatly the issue and the recommended solution here :
stimulusreflex/stimulus_reflex#314 (comment)

I am wondering if we should rewrite the tooltip stimulus controller to use stimulus-reflex:after callback as described above, it seems to me to be the better approach. It would prevent the issue in any other page that uses the tooltip controller.As it stands, the onus is on the reflex to do the correct render, and I feel like it's not its job to have to worry about any stimulus controller it's not interacting directly with.

That said the stimulus docs do describe the pattern used here : https://docs.stimulusreflex.com/guide/cableready.html#using-cableready-inside-a-reflex-action

What do you guys think ?

Even after adding this.element.addEventListener("stimulus-reflex:after", this.insertToolTipMarkup); into the connect() method, I get a

Uncaught TypeError: this.elementTarget is undefined insertToolTipMarkup tooltip_controller.js:78

Thank you for the links, Gaetan, very interesting. I didn't know that you could also queue cable_ready events without broadcasting them immediately and that they get then broadcasted after the StimulusReflex morph.

I guess the idealistic solution for the tooltip is to render the tooltip server-side and include it in the morphing partial. Hm, that's actually already done in other places. Do we just need to get rid of insertToolTipMarkup and replace it with a partial?

I believe original intend was to use a partial : b76d904 but then Matt did some refactoring here : d64a9af which led to the current behaviour.

I had a go at using stimulus-reflex callbacks, I can trigger a re render but for some reason it's not finding the data attributes linked to the element with the tooltip controller 😕

From the linked I shared above :

In the context of SR (Stimulus Reflex), the expectation is that you would specify the UI you want to exist in the browser on the server. There's no need for a render stage because the markup is already perfect before it is sent down the wire. There is no build stage, no rehydration. We're not taking over the task of redrawing the UI from the browser.

I'll look at moving back to a partial.

mkllnk

Nice, this is great.

dacook

Looks good! 👍 I just have a query about the need for sanitize.

dacook · 2023-10-03T05:19:25Z

app/views/admin/shared/_tooltip_button.html.haml

+  %button{class: button_class, "data-reflex": button_reflex, "data-id": reflex_data_id, "data-tooltip-target": "element" }
+  .tooltip-container
+    .tooltip{"data-tooltip-target": "tooltip"}
+      = sanitize tooltip_text


Would it not be sanitised automatically?

I believe rails escape html automatically but does not sanitize unless you do it manually.

spec/system/admin/orders_spec.rb

@filipefurtad0

To be sure that every stimulus controllers attached to the DOM that is replaced is will connected. What I saw is that when using `morph`, the stimulus controller (specially `tooltip_controller`) were not attached to the DOM (and not disconnected as well) that triggered some errors in the console. Adds test case for payment capture thanks to @filipefurtad0

The issue is with with the stimilus tooltip controller, it add some element to the DOM which create issue when it's modified by StimulusReflex. See here for a more detailed explanation: stimulusreflex/stimulus_reflex#314 (comment)

It's not great to have Stimulus controller rendering markup on `connect` Stimulus is intended to add behavior to existing markup. Plus add some documentation

dacook

Argh, looking back I'm sorry I stalled this while you were on leave.

Thanks for responding. Looking at this again, I can see tooltip_text comes from the locale, so it shouldn't require any further sanitising.
But I can also see that it was copy/pasted from existing code, so all good 👍

I don't want to stall it anymore, so will move to test ready when specs pass!

drummer83 · 2023-10-20T09:56:21Z

Hi @rioug,
Thanks for finishing this PR! 💪

Before staging

I could see the error message in the browser console.
I could see the missing tooltip after capturing a payment. After reloading the page, the tooltip was back.
I could see the missing 'Ship' icon after capturing a payment. However I think the icon is there but it has very little contrast compared to the background (the icon is not changing its color back to blue when removing the mouse hovering). After clicking anywhere else on the page to release the focus, the icon is back.

Peek.2023-10-19.21-11.mp4

After staging

The error in the browser console is gone. ✔️
The tooltip remains visible at all times. ✔️
The 'Ship' icon is stll 'missing' (invisible) after capturing a payment. ❌

Peek.2023-10-18.11-20.mp4

Summary

I will merge this one because it's fixing two out of three issues.
For the remaining invisible 'Ship' icon I will create a new issue.

Thanks again! 🎉

jibees self-assigned this Sep 21, 2023

jibees force-pushed the 10956-use-cable_ready-instead-of-morph branch from 5e81d61 to 986be46 Compare September 21, 2023 12:42

jibees mentioned this pull request Sep 21, 2023

Adds pending test case for payment capture, and JS error #11132

Closed

mkllnk approved these changes Sep 22, 2023

View reviewed changes

rioug force-pushed the 10956-use-cable_ready-instead-of-morph branch from a9c15b4 to 501d182 Compare September 25, 2023 13:28

rioug requested a review from mkllnk September 25, 2023 14:01

mkllnk approved these changes Sep 27, 2023

View reviewed changes

dacook reviewed Oct 3, 2023

View reviewed changes

rioug assigned rioug and unassigned jibees Oct 17, 2023

jibees and others added 7 commits October 17, 2023 13:33

Revert to using morph instead of cable_ready

31c537c

The issue is with with the stimilus tooltip controller, it add some element to the DOM which create issue when it's modified by StimulusReflex. See here for a more detailed explanation: stimulusreflex/stimulus_reflex#314 (comment)

Remove insertToolTipMarkup

4639e53

It's not great to have Stimulus controller rendering markup on `connect` Stimulus is intended to add behavior to existing markup. Plus add some documentation

Improve tooltip partial

01a13a8

Refactor "What's this" tooltip

6a1664d

Refactor tooltip on the backoffice orders page

7ce3fea

Add the highlighted version of the "ship" icon

d2952d4

rioug force-pushed the 10956-use-cable_ready-instead-of-morph branch from 501d182 to d2952d4 Compare October 17, 2023 03:05

dacook approved these changes Oct 17, 2023

View reviewed changes

drummer83 self-assigned this Oct 18, 2023

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Oct 18, 2023

drummer83 merged commit 2ba5ab7 into openfoodfoundation:master Oct 20, 2023

drummer83 mentioned this pull request Oct 20, 2023

[BUU][Orders] Ship icon is invisible on hovering #11692

Open

drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin, Orders list: when capturing an order, fix an issue that makes the tooltip invisible (+ errors in the console) #11577

Admin, Orders list: when capturing an order, fix an issue that makes the tooltip invisible (+ errors in the console) #11577

jibees commented Sep 21, 2023 •

edited by rioug

Loading

mkllnk Sep 22, 2023

rioug Sep 22, 2023

jibees Sep 22, 2023

mkllnk Sep 25, 2023

rioug Sep 25, 2023

mkllnk left a comment

dacook left a comment

dacook Oct 3, 2023

rioug Oct 17, 2023

dacook left a comment

drummer83 commented Oct 20, 2023

Admin, Orders list: when capturing an order, fix an issue that makes the tooltip invisible (+ errors in the console) #11577

Admin, Orders list: when capturing an order, fix an issue that makes the tooltip invisible (+ errors in the console) #11577

Conversation

jibees commented Sep 21, 2023 • edited by rioug Loading

What? Why?

What should we test?

Release notes

mkllnk Sep 22, 2023

Choose a reason for hiding this comment

rioug Sep 22, 2023

Choose a reason for hiding this comment

jibees Sep 22, 2023

Choose a reason for hiding this comment

mkllnk Sep 25, 2023

Choose a reason for hiding this comment

rioug Sep 25, 2023

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook Oct 3, 2023

Choose a reason for hiding this comment

rioug Oct 17, 2023

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

drummer83 commented Oct 20, 2023

Before staging

After staging

Summary

jibees commented Sep 21, 2023 •

edited by rioug

Loading