Thanks! That's interesting.
If they keyboard plugin only needs each tr to have an id, without paying attention to the value/meaning of the id, we could simply put a value somewhat unique like the hashKey of the Job object. No non-HTML characters in there. Or if there's a unique id for each job already in the data model we could use it too. I'll have a look. I'll also install the plugin to play with it.
Asked jieryn on Github for his opinion on how to proceed. It's clear that the current approach is broken.
My favorite solution would be to extend the KS plugin to not require these (shouldn't be too difficult to add them in a Javascript by the plugin) and remove from core, but haven't tried whether it's feasible.
html id are useful, for instance when using automated UI validation with framework like Selenium. Or, when they're used by plugin
Let's keep it simple: keep the ids so that the Keyboard plugin works, and add a space-stripping static method in the Functions class to avoid polluting the object model for that purpose. This function can be revisited later if other issues arise in the html ids.
I got it working, i.e. I can navigate to a job from a job list page using my keyboard (start with g-j then use arrow). However my setup is quite simple: Jenkins 1.578-SNAPSHOT and Keyboard Shortcuts Plugin 1.2, nothing else.
Is it not working for you?
Checking the plugin code, these actually seem to be used, however only for the function `ks_view_job_next`, `ks_view_job_prev`, which are triggered by j/k/n/p.
I just don't see how this could not achieved in the plugin by adding the classes in JS...
Please see my PR : https://github.com/jenkinsci/jenkins/pull/1383.
Can I get people's opinion on one of these 3:
1.use the job name as html id by stripping space characters (initial commit in the PR)
2.use Functions#generateId() - and do not actually touch Functions and FunctionsTest
3.Remove the htmlid altogether from projectViewRow.jelly? In that case I'll have to defer to a Javascript specialist to implement this in the KeyboardPlugin
Daniel, I noticed that the plugin works well to navigate to jobs in Chrome, but behaves weirdly to do the same in FireFox. I will check the reported bugs for the plugin.
As noted in the PR, solution 1 and 2 breaks a feature of the keyboard plugin (selecting a job on a view page using the 'n' key for instance). Some code change is required in the Keyboard plugin in conjunction with JENKINS-23330.
We don't use the Keyboard Plugin either, but as noted above, IDs may be useful for Scrapping/Browser-Automation or for future Plugins. I vote for stripping spaces or replacing them by "-", "_" or even "%20" (html5 only). Latter solution has the advantage of generic URL encoding being possible to apply on all special chars, and URL fragments being the same as paths for the jobs. So jenkins/#My%20Job would jump to the "My Job" row, while jenkins/My%20Job would jump to the job's dashboard.
IDs may be useful for Scrapping/Browser-Automation or for future Plugins
Sounds like YAGNI to me.
1. The "feature" is broken.
2. Nobody* uses it.
The best fix would be to just get rid of it in core. It's just something that needs to be maintained for no real reason. It could always be added again later once someone actually needs it, has a good reason for it, and can provide an implementation that's not trivially broken.
*400 stagnating installs is close enough to nobody – and so far no other implemented use cases of this have been mentioned.
In every project list table there are HTML table rows (
Jumping to the relevant html element using the ID as URL fragment should work in most browsers, however.
Originally reported by huuugo, imported from: Spaces in project names are not encoded on IDs
- status: Open
- priority: Trivial
- resolution: Unresolved
- imported: 2022/01/10
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
elements in this table as suggested by Daniel ?
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
timja
commented
10 years ago
- © Githubissues.
- Githubissues is a development platform for aggregating issues.
danielbeck:
Is this ID even used anywhere?
huuugo:
No, it isn't used by Jenkins directly. It only is used by users or their browsers.
For this the bug is rated trivial.
dominiquebrice:
What about removing the id altogether form the
danielbeck:
I dug a bit.
Original commit: https://github.com/jenkinsci/jenkins/commit/580d8f2db50cce1256166cb044305f47e8b088eb
Original issue: https://issues.jenkins-ci.org/browse/JENKINS-12518
Plugin mentioned: https://wiki.jenkins-ci.org/display/JENKINS/Keyboard+Shortcuts+Plugin
dominiquebrice:
Thanks! That's interesting.
If they keyboard plugin only needs each tr to have an id, without paying attention to the value/meaning of the id, we could simply put a value somewhat unique like the hashKey of the Job object. No non-HTML characters in there. Or if there's a unique id for each job already in the data model we could use it too. I'll have a look. I'll also install the plugin to play with it.
danielbeck:
Asked jieryn on Github for his opinion on how to proceed. It's clear that the current approach is broken.
My favorite solution would be to extend the KS plugin to not require these (shouldn't be too difficult to add them in a Javascript by the plugin) and remove from core, but haven't tried whether it's feasible.
dominiquebrice:
html id are useful, for instance when using automated UI validation with framework like Selenium. Or, when they're used by plugin
Let's keep it simple: keep the ids so that the Keyboard plugin works, and add a space-stripping static method in the Functions class to avoid polluting the object model for that purpose. This function can be revisited later if other issues arise in the html ids.
danielbeck:
Does the Keyboard Plugin actually work with "valid" IDs?
dominiquebrice:
I got it working, i.e. I can navigate to a job from a job list page using my keyboard (start with g-j then use arrow). However my setup is quite simple: Jenkins 1.578-SNAPSHOT and Keyboard Shortcuts Plugin 1.2, nothing else.
Is it not working for you?
danielbeck:
Just asked, never used the plugin.
Checking the plugin code, these actually seem to be used, however only for the function `ks_view_job_next`, `ks_view_job_prev`, which are triggered by j/k/n/p.
I just don't see how this could not achieved in the plugin by adding the classes in JS...
dominiquebrice:
Please see my PR : https://github.com/jenkinsci/jenkins/pull/1383.
Can I get people's opinion on one of these 3:
1.use the job name as html id by stripping space characters (initial commit in the PR)
2.use Functions#generateId() - and do not actually touch Functions and FunctionsTest
3.Remove the htmlid altogether from projectViewRow.jelly? In that case I'll have to defer to a Javascript specialist to implement this in the KeyboardPlugin
Daniel, I noticed that the plugin works well to navigate to jobs in Chrome, but behaves weirdly to do the same in FireFox. I will check the reported bugs for the plugin.
dominiquebrice:
As noted in the PR, solution 1 and 2 breaks a feature of the keyboard plugin (selecting a job on a view page using the 'n' key for instance). Some code change is required in the Keyboard plugin in conjunction with JENKINS-23330.
huuugo:
We don't use the Keyboard Plugin either, but as noted above, IDs may be useful for Scrapping/Browser-Automation or for future Plugins. I vote for stripping spaces or replacing them by "-", "_" or even "%20" (html5 only). Latter solution has the advantage of generic URL encoding being possible to apply on all special chars, and URL fragments being the same as paths for the jobs. So jenkins/#My%20Job would jump to the "My Job" row, while jenkins/My%20Job would jump to the job's dashboard.
danielbeck:
Sounds like YAGNI to me.
1. The "feature" is broken.
2. Nobody* uses it.
The best fix would be to just get rid of it in core. It's just something that needs to be maintained for no real reason. It could always be added again later once someone actually needs it, has a good reason for it, and can provide an implementation that's not trivially broken.
*400 stagnating installs is close enough to nobody – and so far no other implemented use cases of this have been mentioned.
huuugo:
Fair point and ok for me.