Bug #7669
closedSome examples with WSuggestionPopup produce duplicate DOM ids and exhibit incorrect behavior
0%
Description
This issue can be observed in the composer and planner examples with github wt 4.4.0-rc1-1-gea5f938d --- although I don't believe it is a recent regression. For the composer, click on "Add Cc", then type a "b". A suggestion box will appear with one email address. However, it will not match additional characters or handle backspaces as would be expected. It will accept enter, but only once, and then the suggestion box will not reappear. The planner exhibits similar behavior for the time fields.
The issue appears to be due to the use of addWidget() rather than addChild() for the WSuggestionPopup subclasses in these examples. Since these classes are derived from WPopupWidget, the use of addWidget() causes them to be added to the DOM twice, once via createStubElement() and a second time via addGlobalWidget(). The duplicate ids can cause problems for the JavaScript code (handling the keyup event), as seen in the examples.
There is a similar issue with the widgetgallery's Autocomplete example, where addNew() is used to add a WSuggestionPopup. This example appears to function correctly, although it also generates a duplicate id. The difference in behavior is likely due to the ordering of the duplicate elements.
I originally encountered this issue when coding a test program to include with #7666. I made the mistake of using addNew() for a WSuggestionPopup and spent some time debugging the error. Since it is always an error (?) to use addWidget with a subclass of WPopupWidget, I tried adding some static_asserts to Wt to catch this case. That's what revealed the problems with the three examples.
Attached for your review are two patches that have been lightly tested. The first is intended to fix the planner and composer examples. It also adds some static_asserts based on the assumption that it is an error to call the addWidget() related functions with an instance derived from WPopupWidget. The second patch is intended to fix the Autocomplete example and adds support for a addChildNew() method, as well as an additional static_assert. There are more comments on the patches.
Files
Updated by Roel Standaert over 4 years ago
It seems to me that the static assert is right. The addChildNew is kind of an awkward name, addNewChild makes a bit more sense.
I think I'll add the fixes to the examples to Wt 4.4.0, but keep the static assert and addNewChild for the next release.
Updated by Roel Standaert over 4 years ago
Another (maybe slightly crazy) idea would be to use type traits and enable_if to select the implementation of addNew to use:
-
addNewfor aWObjectthat is not aWWidget: usesaddChild -
addNewfor aWWidgetthat is not aWPopupWidget: usesaddWidget -
addNewfor aWPopupWidget: usesaddChild
Updated by Bruce Toll over 4 years ago
Thanks for the quick follow-up. Your idea for enhancing addNew() could actually prevent these subtle bugs in user code, rather than just having the compiler exit with an error.
Updated by Roel Standaert about 4 years ago
- Description updated (diff)
- Status changed from New to Resolved
I ended up going with a simpler at-runtime approach: any global widget (a widget is automatically flagged as global when WApplication::addGlobalWidget is called) only has its ownership transferred (addChild) instead of being added as a child in the widget tree (addWidget).
I decided against adding addNew to WObject, since it would have limited use in the end (most non-widget WObjects are managed as shared ptrs anyway), and might just confuse users when using addNew on some other widget than WContainerWidget. The main use is really with WPopupWidget and the like.
Updated by Bruce Toll about 4 years ago
- File 0003-Extra-tests-for-WContainerWidget-addwidget-family.patch 0003-Extra-tests-for-WContainerWidget-addwidget-family.patch added
Thanks for the upgrade to addNew(). I'm sure that it will prevent a lot of usage errors.
I did notice that some of the related member functions, e.g. addWidget, insertWidget, and insertBefore have not been updated and wanted to double-check that this was intentional.
For clarity, I've attached a patch that complements your new containerwidget_addnew test with tests for addWidget, insertWidget, and insertBefore. These tests do not currently pass and I'm not sure which, if any, should.
For consistency with addNew, it might make sense for addWidget to call addChild if called with a global widget (similar to the logic you added to addNew).
It's a subtle distinction, but I suspect that passing a global widget directly to insertWidget or insertBefore probably indicates a programming error that should be flagged (statically or with an exception?), since the user is being more explicit about what they are trying to accomplish and it doesn't make sense. You can't insert an item before another specified item in a different list.
In any case, the existing commits are a big step forward and very much appreciated!
Updated by Roel Standaert about 4 years ago
Yeah, I might rather indicate to the user that that's not appropriate for global widgets.
Updated by Bruce Toll about 4 years ago
I totally missed this, but another thing to consider is the behavior of WContainerWidget::removeWidget().
There could be existing applications that previously called WContainerWidget::addNew() incorrectly with a global widget and subsequently called removeWidget(). With the new behavior of WContainerWidget::addNew(), the widget will no longer be added to the container, so I believe the subsequent removeWidget() will log an error -- I haven't actually tested it.
If WContainerWidget::addWidget() is modified to have the same behavior as addNew(), it would share the same issue.
I'm not sure of the best way to address this, but thought I'd at least raise the issue for your consideration.
Updated by Roel Standaert about 4 years ago
- Status changed from Resolved to Feedback
How about a remove() that does a removeWidget(), and if that returns nullptr, it does removeChild()?
Updated by Bruce Toll about 4 years ago
Taking your idea one step further, I wonder if WContainerWidget::removeWidget() itself could be modified to use the new isGlobalWidget() test and invoke removeChild() for global widgets. That way, existing code could benefit, as with your recent update to addNew(). Moreover, there would still be an error logged if removeWidget() was incorrectly called with a non-global object that was not in the container ("removeWidget(): widget not in container").
One concern, though, is maintaining consistent behavior across the many contexts where removeWidget() currently exists, e.g. WStackedWidget, WCompositeWidget, WLayout, etc. Perhaps, it would make things simpler to throw an exception on attempting to add a global widget to any container-like objects other than WContainerWidget? There is no need to special case removeWidget for global widgets, if they can't be added.
Summarizing:
- In general, it should not be possible to add global widgets to container-like widgets. Attempts to do so will result in an exception.
- As a narrow exception to the above (to maintain compatibility with existing code and simplify a common use case), the addNew(), addwidget(), and removeWidget() methods of WContainerWidget will accept global widgets without error and treat them as child objects.
What do you think?
Updated by Roel Standaert about 4 years ago
- Target version changed from 4.5.0 to 4.6.0
Updated by Roel Standaert over 3 years ago
- Status changed from Feedback to Closed
- Target version deleted (
4.6.0)
I'm closing this since the issue was fixed in an old release already. Creating a new issue for further future consideration.
Updated by Roel Standaert over 3 years ago
- Related to Feature #8620: Consider improvements to the API to prevent bugs like issue #7669 added