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

[BUG] 5.3.2 Exception in text.js #7887

Closed
CodaCodr opened this issue Dec 14, 2023 · 21 comments · Fixed by #7905
Closed

[BUG] 5.3.2 Exception in text.js #7887

CodaCodr opened this issue Dec 14, 2023 · 21 comments · Fixed by #7905

Comments

@CodaCodr
Copy link
Contributor

CodaCodr commented Dec 14, 2023

Describe the bug

The following (very old!) code issues a RSOE in 5.3.2

\define se-popup-menu-btn()
<$set name=tag value="""$(tag)$""">
<$button popup=<<qualify $:/state/rgt-bk/se-selector-popup>> set="$:/state/rgt-bk/state/se-show-sections" setTo="hide" tooltip="Show a list of entries"
  class="sv-popup-section-menu-button tc-btn-invisible tc-tiddlylink">{{$:/core/images/down-arrow}}</$button>
<$reveal type="popup" state=<<qualify $:/state/rgt-bk/se-selector-popup>> Xclass="tc-popup-keep" position=below>
<<se-popout-entries-selector>>
</$reveal>
</$set>
\end

\define se-popout-entries-selector()
<div class="se-entry-selector-panel" style="width:auto;">
 <$list filter="[all[tiddlers]tag<tag>sort[]]" variable="item">
  <$set name=cap value={{{ [<item>get[caption]] }}}>
   <div>
    <$button style.white-space="nowrap" actions=<<se-section-changed-actions>> class="tc-btn-invisible tc-tiddlylink" 
     setTitle=<<cur-item-store>> 
     setField=text 
     setTo=<<item>>
    ><$text text=<<cap>>/></$button>
   </div>
  </$set>
 </$list>
</div>
\end

TiddlyWiki breaks at <$text text=<<cap>>/>

NotFoundError: Node.insertBefore: Child to insert before is not a child of this node

Uncaught DOMException: Node.insertBefore: Child to insert before is not a child of this node

image

TiddlyWiki Configuration

  • Version 5.3.2
  • FF latest
  • Win10
@CodaCodr CodaCodr changed the title [BUG] 5.3.2 [BUG] 5.3.2 text widget Exception Dec 14, 2023
@pmario
Copy link
Member

pmario commented Dec 14, 2023

If the example code is copy / pasted, it does nothing

@Jermolene
Copy link
Member

Hi @CodaCodr is there some missing code that invokes those macros?

Is this a regression? Does the same problem occur in, say, 5.27:

https://tiddlywiki.com/#TiddlyWiki%20Archive

@CodaCodr
Copy link
Contributor Author

Sorry, @pmario @Jermolene:

That code was meant to be informative, not demonstrative. It was 01:30 here at the time I posted, had I been less sleepy, I may have posted on Talk, instead, as it's not easy to make it work on tw .com
I should have made that clear.

Is this a regression?

From my perspective, yes, absolutely. I think that code goes back to 5.1.15 ish.

My concern was, in se-popout-entries-selector, having setTitle, setField, setTo and using actions on the button at the same time <-- I'd left myself a note about it but it has been working fine. And, to be clear, the render is fine. When I click the button, RSOE.

Next, I took out the actions attr: same error. So the issue arises "in" the button, not in the actions transclusion.

Hope that's a little clearer.

@pmario
Copy link
Member

pmario commented Dec 14, 2023

Hope that's a little clearer.

Not really. We need some code, that -- if copy / pasted to tw-com works and allows us to reproduce the problem.

I did create a button with <<se-popup-menu-btn>>
I did set the tag-variable to "HelloThere"

But nothing happens using FireFox Windows 11

@CodaCodr
Copy link
Contributor Author

I'll keep hacking away at it and close this when I have working code that can be compared side-by-side with the non-working code.

@CodaCodr
Copy link
Contributor Author

You don't need to respond unless you see something that grabs your attention. This is just my debugging story.

Latest code is...

\define se-popout-entries-selector()
 \define local-actions()
 <<alog local-actions "cur-item-store item">>
   <$action-setfield $tiddler=<<cur-item-store>>  $field=text $value=<<item>>/>
 \end local-actions
 
<<log se-popout-entries-selector "currentTiddler cur-item-store">>
<div class="se-entry-selector-panel" style="width:auto;">
 <$list filter="[all[tiddlers]tag<tag>sort[]]" variable="item">
  <$set name=cap value={{{ [<item>get[caption]] }}}>
   <div>
    <$button style.white-space="nowrap" actions=<<local-actions>> Xactions=<<se-section-changed-actions>> class="tc-btn-invisible tc-tiddlylink" 
     XsetTitle=<<cur-item-store>> 
     XsetField=text 
     XsetTo=<<item>>
    ><$text text=<<cap>>/></$button>
   </div>
  </$set>
 </$list>
</div>
\end

points:

  • alog is a wrapper over <$action-log>
  • setTitle setField and setTo are X-ed out.
  • button's actions are now in local-actions

The alog call works.
The <$action-setfield> works.
Immediately after, the RSOE appears and this appears in the console

image

So that looks to me that a setTimeout (tick) happens in popup.js and blows up -- which leaves me thinking, there isn't anything I can do directly that might alter that outcome.

Except...

Many moons ago, that list was producing <option>s in a select which I strongly suspect won't have this issue.

@CodaCodr CodaCodr changed the title [BUG] 5.3.2 text widget Exception [BUG] 5.3.2 popup Exception Dec 14, 2023
@CodaCodr
Copy link
Contributor Author

Actions moved to old-skool child elements inside the button...

\define se-popout-entries-selector()
<div class="se-entry-selector-panel" style="width:auto;">
 <$list filter="[all[tiddlers]tag<tag>sort[]]" variable="item">
  <$set name=cap value={{{ [<item>get[caption]] }}}>
   <div>
    <$button style.white-space="nowrap" class="tc-btn-invisible tc-tiddlylink" >
      <<alog local-actions "cur-item-store item">>
      <$action-setfield $tiddler="$:/.rgt/bk-tw/ToolsView-cur-item"  $field=text $value=<<item>>/>
     
    <$text text=<<cap>>/></$button>
   </div>
  </$set>
 </$list>
</div>
\end

Same issue.

@CodaCodr
Copy link
Contributor Author

On a whim, I used Eric's <$action-timeout> widget to delay execution of the actions by 2 seconds. The RSOE took 2 seconds to appear. So it's consistent and doesn't appear to be a timing issue, per se.

I'm guessing, "faulty this" somewhere, and/or a closure not finding what it needs.

@CodaCodr
Copy link
Contributor Author

Having replaced the popup with a $select widget, and found the exact same problem, I pulled up the debugger and set some breakpoints.

I can see that all the obvious code associated with the select element seems to be executing correctly (various renderings and even the change event being dispatched.

Having previously set the debugger to pause on exceptions and then let it run on (clicked play) there's a short pause and then this, in text.js

image

As you can see, textNode is \n, as is nextSibling. parent is a select element (it's properties tell me it's the right select element). And Firefox/gecko is saying "NotFoundError: Node.insertBefore: Child to insert before is not a child of this node"

@CodaCodr
Copy link
Contributor Author

Another late night debugging session. The problem is a select element further along in the node tree. If I remove that select, everything works.

Strangely, the select contains static data... more later.

@CodaCodr
Copy link
Contributor Author

Recall that the original code contained a popup. When the popup (menu) was revealed, and an item from the list was chosen, the RSOE appeared. Very easy to be deceived as to where the problem lies...

The popup was then replaced with a $select. It suffered the same problem.

While I was pondering this bug, and having realized that since the exception was occurring during the refresh cycle (see screenshots above), I decided to bite the bullet and step through it, best I could. Yes. Tedious. But it had to be done.

Eventually I got to the problem select element -- which turned out to be a completely different select element to the select element used as a replacement for the original popup menu.

@Jermolene This is the select element in question.

  <% if [{!!sectionview-select}match[yes]] %>
    Links will target
      <$select tiddler=<<currentTiddler>> field="sectionview-target" class="tc-edit-tags sectionview-select">
        <option></option>
        <option>SectionView</option>
        <option>SectionView2</option>
        <option>SectionView3</option>
      </$select>
<!--    Links will target <$macrocall $name=sectionview-select tiddler=<<currentTiddler>> field=sectionview-target/> -->
  <% endif %>

And keep in mind:

  1. The <option> data is hardwired.
  2. The exception occurs while rendering, during refresh, not while using the select.
  3. Remove the select and the exception is gone.

You can see also, where I commented out a macro that used to transclude that $select code. Made no difference. You can also see I removed <$if> and replaced with <% if %> -- really, I've tried every dumb thing I can think of to nail this down for you.

In the image below, the select (formerly a popup) is marked 1, the select actually causing the problem all along, is marked 2.

image

If I have any stamina left, I may replace that whole UI with a series of radios and see what that does.

@saqimtiaz
Copy link
Member

@CodaCodr As part of other updates to the widget code, the select widget needed to be largely refactored and modernized in v5.3.2 and I suspect these two lines got accidentally swapped around by me while moving code around:

https://github.com/Jermolene/TiddlyWiki5/blob/62bb8affa48aec958fc73104b060f6a90efc8e1b/core/modules/widgets/select.js#L65-L66

Try swapping those two lines around and see if that resolves the issue.

@CodaCodr
Copy link
Contributor Author

select.js locally now has...

image

Same issue. Damn, I thought that had to be it :(

@CodaCodr
Copy link
Contributor Author

The stack as shown in the console:

image

@CodaCodr
Copy link
Contributor Author

@saqimtiaz the exception occurs inside text.js. If I step back through the stack I do indeed reach the select:

image

So, the domNode is the select, and calling renderChildren leads to the exception being thrown in text.js. as per

image

@CodaCodr
Copy link
Contributor Author

parent.parent maybe? Unless widget is a parent too...

@saqimtiaz
Copy link
Member

I'm on the mend from a nasty cold, I can look into this properly in a few days to test if the render and refresh logic has changed due to the refactoring.

In the meantime if you have managed to figure out a minimal test case to recreate the problem, that will be very helpful.

@CodaCodr
Copy link
Contributor Author

CodaCodr commented Dec 15, 2023

I should have mentioned before, this code is part of a bundle of 900+ tiddlers. Working fine in 5.2.x --> 5.3.1

Said another way, you are correct to be looking for a change in 5.3.2.

@saqimtiaz
Copy link
Member

saqimtiaz commented Dec 15, 2023

Ugh. I should have caught this before release.

Try changing this.renderChildren(domNode,nextSibling); to this.renderChildren(domNode,null); in select.js

@CodaCodr
Copy link
Contributor Author

image

Bingo.

Thanks once again, Saq.

@saqimtiaz
Copy link
Member

@CodaCodr could you please leave this open until we get a fix merged? Thank you

@CodaCodr CodaCodr reopened this Dec 15, 2023
@CodaCodr CodaCodr changed the title [BUG] 5.3.2 popup Exception [BUG] 5.3.2 Exception in text.js Dec 17, 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 a pull request may close this issue.

4 participants