Bug 21709 - Addbiblio shows clickable tag editor icons which do nothing
Summary: Addbiblio shows clickable tag editor icons which do nothing
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-29 07:19 UTC by paxed
Modified: 2019-05-30 15:12 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06


Attachments
Bug 21709: Replace 'no popup' plugin links with a span (1.54 KB, patch)
2018-11-01 16:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21709: Replace 'no popup' plugin links with a span (1.86 KB, patch)
2019-05-06 17:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21709: Replace 'no popup' plugin links with a span (1.90 KB, patch)
2019-05-06 18:15 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21709: Replace 'no popup' plugin links with a span (2.00 KB, patch)
2019-05-10 07:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2018-10-29 07:19:01 UTC
Addbiblio can show a tag editor icon after the field which does nothing. The icon will have the "No popup" title and clicking on it does nothing.

There should be no icon shown, if there is no editor attached to it.
Comment 1 Marcel de Rooy 2018-10-29 07:47:02 UTC
Please add more details how to reproduce.
Comment 2 paxed 2018-10-29 08:58:13 UTC
(In reply to Marcel de Rooy from comment #1)
> Please add more details how to reproduce.

Install master with defaults, go to Cataloging > New record > Default framework, try to click on the 003 or 005 tag editor icon. Contrast that with the tag editor icons of 000, 007, or 008.
Comment 3 Katrin Fischer 2018-10-29 11:05:36 UTC
I think they work when clicking in the field, not all have a pop-up. Similar to the one behind dateacessioned or autobarode.
Comment 4 Katrin Fischer 2018-10-29 11:06:05 UTC
So they icon can indicate extra functionality that is linked to the field.
Comment 5 paxed 2018-10-29 11:37:29 UTC
But in these cases, the icon is clickable, it just does nothing.

If it did the same as clicking on the field (autofilling the data), then it would be OK.

If the icon wasn't clickable, it would be OK. (Although I'd still want a visual indication without needing to move the mouse over it)
Comment 6 Jonathan Druart 2018-11-01 16:31:51 UTC
Created attachment 81832 [details] [review]
Bug 21709: Replace 'no popup' plugin links with a span
Comment 7 Jonathan Druart 2018-11-01 16:32:34 UTC
Like that? Also I am wondering if we should not remove some classes, I do not know what is the purpose of .buttonDot
Comment 8 Marcel de Rooy 2018-11-07 12:28:15 UTC
<span class="buttonDot tag_editor disabled" tabindex="-1" title="Field autofilled by plugin"></a>

This seems to be invalid HTML :)
Comment 9 Marcel de Rooy 2018-11-07 13:03:28 UTC
(In reply to Jonathan Druart from comment #7)
> Like that? Also I am wondering if we should not remove some classes, I do
> not know what is the purpose of .buttonDot

This makes no actual difference. I still see a button that does not respond to clicks. But this is the actual design: if you do not explicitly assign a click event, it should not respond to click. (The only problem is that we have been used to some buttons that translated click to focus events.)
I agree with paxed that we should see the difference right away: have a visual distinction between a real popup and just a focus action (there may be a report already for that). Only removing the button is not the solution, since we still want to alert on the focus action.
It should not be that hard to define a separate class and add some css associated to it to achieve our goal.

On the other hand, when we really want click to translate to focus, add three lines in the popup script. For instance for 003 in marc21_orgcode.pl:
function Click$params->{id}(event) {
    \$('#'+event.data.id).focus();
}
And now it responds to click and triggers focus.. (I still think though that it should not be the default.)
Comment 10 Marcel de Rooy 2018-11-07 13:04:46 UTC
The buttondot class indeed seems to be unneeded ballast. Note that we do need the buttonDot_ id.
Comment 11 Jonathan Druart 2019-05-06 17:43:26 UTC
Created attachment 89379 [details] [review]
Bug 21709: Replace 'no popup' plugin links with a span

Test plan:
Edit a bibliographic record
Notice that the icons next to fields linked with a plugin can be a link
or not. The browser displays a pointer or not.

It will remove the ambiguity to display an icon that is clickable but
does nothing when clicked.
We could also have another icon to disociate both behaviors.
Comment 12 Jonathan Druart 2019-05-06 17:44:33 UTC
(In reply to Marcel de Rooy from comment #9)
> (In reply to Jonathan Druart from comment #7)
> > Like that? Also I am wondering if we should not remove some classes, I do
> > not know what is the purpose of .buttonDot
> 
> This makes no actual difference. I still see a button that does not respond
> to clicks.

To me it's an improvement, the link is removed if no popup will be opened. So there is an icon but nothing to tell you it's clickable (pointer will be different).
Comment 13 Owen Leonard 2019-05-06 18:15:22 UTC
Created attachment 89390 [details] [review]
Bug 21709: Replace 'no popup' plugin links with a span

Test plan:
Edit a bibliographic record
Notice that the icons next to fields linked with a plugin can be a link
or not. The browser displays a pointer or not.

It will remove the ambiguity to display an icon that is clickable but
does nothing when clicked.
We could also have another icon to disociate both behaviors.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Marcel de Rooy 2019-05-10 07:12:46 UTC
QA: Looking here
Comment 15 Marcel de Rooy 2019-05-10 07:18:35 UTC
Created attachment 89524 [details] [review]
Bug 21709: Replace 'no popup' plugin links with a span

Test plan:
Edit a bibliographic record
Notice that the icons next to fields linked with a plugin can be a link
or not. The browser displays a pointer or not.

It will remove the ambiguity to display an icon that is clickable but
does nothing when clicked.
We could also have another icon to disociate both behaviors.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Nick Clemens 2019-05-10 14:52:09 UTC
Awesome work all!

Pushed to master for 19.05
Comment 17 Martin Renvoize 2019-05-17 13:45:02 UTC
Pushed to 18.11.x for 18.11.06
Comment 18 Lucas Gass 2019-05-30 15:12:22 UTC
backported to 18.05.x for 18.05.13