Skip to content

WMenu: enforce segment boundaries in internal path matching#246

Open
saddamr3e wants to merge 1 commit into
emweb:masterfrom
saddamr3e:fix/wt-investigation
Open

WMenu: enforce segment boundaries in internal path matching#246
saddamr3e wants to merge 1 commit into
emweb:masterfrom
saddamr3e:fix/wt-investigation

Conversation

@saddamr3e

Copy link
Copy Markdown

Problem

WMenu uses the internal match(path, component) helper function to determine how well the current internal path matches a menu item's path component.

The current implementation incorrectly treats prefix matches as valid even when they do not end on a path segment boundary.

For example:

If the current path is /contact-us and the menu contains a "contact" item, match("contact-us", "contact") returns 7, causing the "contact" menu item to be incorrectly selected even though /contact-us represents a different route.
Likewise, match("a/ab", "a/ac") returns 1 instead of -1, which contradicts the documented behavior of the helper function.
Solution
Update match() in src/Wt/WMenu.C to enforce path-segment boundary checks.
Return a positive match only when the component is an exact match or when the following character in the path is '/'.
Add the regression test WMenu_internal_path_matching_segment_boundary in test/widgets/WMenuTest.C to prevent future regressions.

@RomainMard

Copy link
Copy Markdown

Thanks for the pull request.

This change will be added to Wt soon.

Note that the test you added was incorrect and had to be fixed. This was because:

  1. The first element added to the WMenu will be automaticaly selected, even if it does not match the internal path.
  2. WApplication::setInternalPath() needs its second parameter to be set to true in order for the WMenu to react to the internal path change.

@saddamr3e

Copy link
Copy Markdown
Author

Thank you for the review and for fixing the test. I appreciate the feedback and the explanation. I'll keep this in mind for future contributions.

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.

2 participants