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

Allow tab content to be opened using middle click #17156

Merged

Conversation

AdrienClairembault
Copy link
Contributor

Issue

Opening an item tab using the middle click lead to a broken "raw" page:

image

This is because our tabs href properties are the ajax endpoinst that will fetch the data.
So, opening the link in a new tab will only fetch the tab content and nothing else.

Fix

This is easy to fix as we have no requirement to use this url directly in the href property.
We can use a valid href and move the ajax endpoints URL into dedicated data attributes.

Tests

2E tests can't be added to validate this as cypress does not support multiple browsers tabs.

Branch

I'm doing this changes in main because the href property may possibly be parsed by some plugins which would break with this change.
If you think this possible issue is too "far fetched", I can move the PR to target the bugfix branch instead.

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets !33040

@cconard96
Copy link
Contributor

Full page tabs are an old debug feature. Not sure they serve much of a purpose anymore and the warning message doesn't seem to trigger anymore.
See ajax/common.tabs.php.
Maybe it can be removed altogether.

@cedric-anne
Copy link
Member

Full page tabs are an old debug feature. Not sure they serve much of a purpose anymore and the warning message doesn't seem to trigger anymore. See ajax/common.tabs.php. Maybe it can be removed altogether.

Done in #17157 .

Comment on lines +354 to +356
// TODO: ctrl+click should have the same behavior but it seems
// to be caught by the tabs events handler and does not trigger
// a new browser tab.
Copy link
Member

Choose a reason for hiding this comment

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

Clicks default behaviour are prevented by the bootstrap tabs logic. The only way to handle this is to remove the data-bs-toggle='tab' attribute and add a click handler that either let the default behaviour continue when e.ctrlKey === true, either trigger the tab display manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I am not sure it is worth dealing with right now as it doesn't break anything.

@cedric-anne cedric-anne merged commit 1606886 into glpi-project:main May 22, 2024
8 checks passed
@AdrienClairembault AdrienClairembault deleted the fix/tab-middle-click branch May 22, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants