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

Fixes data attributes support for Select widget. #7847

Conversation

saqimtiaz
Copy link
Contributor

@saqimtiaz saqimtiaz commented Nov 20, 2023

N.B! Test failing for style support, needs feedback.

This PR fixes issues with the data and style attributes support introduced in #7769

Issues:

  • Fixed: the refresh handling for data attributes in the select widget was short circuiting the code that updated the class of the DOM node in place instead of re-rendering the widget.
  • Attempted fix: there was no support for style.* attributes for widgets that called Widget.prototype.assignAttributesToParseTreeNode since this method does not have the extra handling needed for style.* attributes

Outstanding issue:

  • with the change to assignAttributesToParseTreeNode the style.* widget attributes are correctly applied to the DOM node. However, upon refresh when the style values change, the HTML output of the test shows two duplicate style attributes for the DOM node. Running the same code in the wiki in the browser, the output is as expected without the duplicate style attributes. However, this may be a case of the browser compensating for the duplicate attributes and sanitizing the HTML.

For example when changing the style.color from black to red, this output HTML
<select data-title="Title2" style="color:black;" value="TiddlyWiki">
changes to:
<select data-title="Title2" style="color:black;" value="TiddlyWiki" style="color:red;">

I was working on the Tour plugin when I came back to this as something was bothering my subconscious about this part of the implementation. @Jermolene @pmario any input would be appreciated. I will circle back to this later with fresher eyes.

Copy link

vercel bot commented Nov 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ❌ Failed (Inspect) Nov 20, 2023 1:22pm

@saqimtiaz
Copy link
Contributor Author

Looks like this is due to a nuance of the fakedom implementation.

Details to follow.

@Jermolene
Copy link
Owner

Thanks @saqimtiaz I realised while I was looking at this today that there is no good reason why the select widget delegates DOM node creation to the element widget. It's been like that since 2014, and of course I can't remember why. So I think the optimum fix for this might be to refactor the select widget along more conventional lines.

@saqimtiaz
Copy link
Contributor Author

saqimtiaz commented Nov 20, 2023

In the fakedom, adding attributes via domeNode.style["color"] sets a property of the _style property of the fake DOM node. However, using $tw.utils.addStyleToParseTreeNode() adds/amends the style property of the fake DOM node.

The fakedom outerHTML method uses both properties style and _style leading to the duplication of style attributes in the HTML.

@Jermolene yes refactoring the DOM node creation in $selection is sensible. We also need to determine if Widget.prototype.assignAttributesToParseTreeNode() is still needed and if so, make it consistent under fakeDOM with with directly manipulating the style property of the DOM node.

@Jermolene
Copy link
Owner

In the fakedom, adding attributes via domeNode.style["color"] sets a property of the _style property of the fake DOM node. However, using $tw.utils.addStyleToParseTreeNode() adds/amends the style property of the fake DOM node.

Ah, of course.

The fakedom outerHTML method uses both properties style and _style leading to the duplication of style attributes in the HTML.

@Jermolene yes refactoring the DOM node creation in $selection is sensible. We also need to determine if Widget.prototype.assignAttributesToParseTreeNode() is still needed and if so, make it consistent under fakeDOM with with directly manipulating the style property of the DOM node.

I'm pretty sure assignAttributesToParseTreeNode would no longer be needed.

@saqimtiaz
Copy link
Contributor Author

Closing in favour of #7848

@saqimtiaz saqimtiaz closed this Nov 20, 2023
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.

None yet

2 participants