Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Field flow: Handle HTML tags in unlinked list items #2291

Conversation

EricDunsworth
Copy link
Member

@EricDunsworth EricDunsworth commented Nov 29, 2023

Previously, when the plugin encountered list items that didn't start off with an anchor, it used each item's first child node as a label/value. That was fine if the only child was a text node. But not when nested elements were in play...

For example:

  • If a list item's first child node was an element node, wb.escapeAttribute() raised the following JS exception (since it expects strings - not element nodes):
    • Uncaught TypeError: can't access property "replace", str is null
  • If a list item started off with a text node and was followed by any element nodes, only the text node's content would be retained... resulting in the rest of the content getting truncated

This fixes it by deriving list item labels from their inner HTML code (like what's already done for anchors), as well as stripping HTML tags out of option elements and value attributes. HTML formatting is retained for checkbox/radio button labels.

Nested structure that isn't meant to be shown in grouping (ul) and nesting (.wb-fieldflow-sub) scenarios is excluded.

Also updated examples to demonstrate affected scenarios:

  • Redirection example:
    • Bold a link in a list item
  • Other examples:
    • Add italics/bolding in all "(Set 5)"/"(Ensemble 5)" list items

@EricDunsworth EricDunsworth changed the title Fieldflow: Handle nested label markup in "plain" list items Fieldflow: Handle HTML tags in unlinked list items Nov 30, 2023
@EricDunsworth EricDunsworth changed the title Fieldflow: Handle HTML tags in unlinked list items Field flow: Handle HTML tags in unlinked list items Nov 30, 2023
@EricDunsworth EricDunsworth force-pushed the fieldflow-handle-nested-label-markup-plain-lis branch from 0682933 to 3610fc7 Compare November 30, 2023 21:09
@EricDunsworth EricDunsworth marked this pull request as ready for review November 30, 2023 21:09
@EricDunsworth EricDunsworth force-pushed the fieldflow-handle-nested-label-markup-plain-lis branch from 3610fc7 to 7d78891 Compare December 1, 2023 23:14
@EricDunsworth EricDunsworth force-pushed the fieldflow-handle-nested-label-markup-plain-lis branch from 7d78891 to 962229c Compare December 1, 2023 23:18
@EricDunsworth EricDunsworth marked this pull request as draft December 8, 2023 22:56
@EricDunsworth
Copy link
Member Author

Gonna shift this back into a draft since what I currently have is probably unable to cope with named/numeric entities. So I think it'll need a bit more work.

@EricDunsworth
Copy link
Member Author

I think I'll put this back into ready for review "as-is".

I did more research/testing earlier. Looks like modern browser rendering engines (i.e. Gecko/Blink/etc) are already designed to automatically convert entities into their "native" characters. So there's no need for special handling of entities in my logic. Browser engines already do what I had in mind "for free" :).

@EricDunsworth EricDunsworth marked this pull request as ready for review January 18, 2024 19:10
@duboisp
Copy link
Member

duboisp commented Jan 22, 2024

Pre-approved upon review and testing

@GormFrank GormFrank requested a review from duboisp January 29, 2024 16:24
Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a partial review since that I found that 2 working example are going to be broken.

See the "Grouping" and the "Nesting" working example. Extra text is added.

Ex:
image

components/wb-fieldflow/fieldflow-en.html Outdated Show resolved Hide resolved
components/wb-fieldflow/fieldflow-en.html Outdated Show resolved Hide resolved
@EricDunsworth EricDunsworth force-pushed the fieldflow-handle-nested-label-markup-plain-lis branch from 962229c to 6c54ded Compare February 23, 2024 19:11
Previously, when the plugin encountered list items that didn't start off with an anchor, it used each item's first child node as a label/value. That was fine if the only child was a text node. But not when nested elements were in play...

For example:
* If a list item's first child node was an element node, wb.escapeAttribute() raised the following JS exception (since it expects strings - not element nodes):
  * Uncaught TypeError: can't access property "replace", str is null
* If a list item started off with a text node and was followed by any element nodes, only the text node's content would be retained... resulting in the rest of the content getting truncated

This fixes it by deriving list item labels from their inner HTML code (like what's already done for anchors), as well as stripping HTML tags out of option elements and value attributes. HTML formatting is retained for checkbox/radio button labels.

Nested structure that isn't meant to be shown in grouping (ul) and nesting (.wb-fieldflow-sub) scenarios is excluded.

Also updated examples to demonstrate affected scenarios:
* Redirection example:
  * Bold a link in a list item
* Other examples:
  * Add italics/bolding in all "(Set 5)"/"(Ensemble 5)" list items
@EricDunsworth EricDunsworth force-pushed the fieldflow-handle-nested-label-markup-plain-lis branch from 6c54ded to 23d0439 Compare February 23, 2024 19:13
@EricDunsworth
Copy link
Member Author

EricDunsworth commented Feb 23, 2024

Just force-pushed an update to fix the grouping/nesting issues + rebased + updated the commit message/OP :).

@duboisp Thanks for catching those! I probably didn't think of testing them initially since my eyes were glued to the examples I modified :P.

Btw the nesting example's labels/values will now end with an extra line break and a couple of tab characters (since the text node that contains them is now being included). That isn't a problem and is in line with this PR's intended purpose. But it might be worthwhile to add some trimming logic in the future...

@duboisp
Copy link
Member

duboisp commented Feb 26, 2024

We will do a review + local testing.

@Garneauma
Copy link
Contributor

Pre-approved upon successful testing.

@Garneauma Garneauma self-requested a review March 4, 2024 15:23
Copy link
Contributor

@Garneauma Garneauma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through the working examples and they all are working as expected with the value and labels fixed.

@Garneauma Garneauma dismissed duboisp’s stale review March 4, 2024 21:06

Contributor made the necessary changes.

@Garneauma Garneauma merged commit 6c327e4 into wet-boew:master Mar 4, 2024
1 check passed
EricDunsworth added a commit to EricDunsworth/GCWeb that referenced this pull request Mar 6, 2024
Also adds a strong element to the grouping example to demonstrate it in action.

Overlooked leftover from wet-boew#2291.
EricDunsworth added a commit to EricDunsworth/GCWeb that referenced this pull request Mar 6, 2024
Removes HTML tags and escapes quotes from optgroup label attributes.

Also adds a strong element to the grouping example to demonstrate it in action.

Overlooked leftover from wet-boew#2291.
EricDunsworth added a commit to EricDunsworth/GCWeb that referenced this pull request Mar 11, 2024
Leading/Trailing spaces are usually undesirable, so let's strip them out.

Doesn't affect labels that are explicitly-declared via the "wb-fieldflow-label" class. Those will be kept as-is so as not to interfere with an implementer's desire.

Affects grouping and nesting scenarios. Directly impacts the demo page's nesting example.

Spinoff of wet-boew#2291.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants