Improve UX when entering email addresses for share link #30645

lordelix · 2018-02-28T07:13:52Z

Description

input field is adaptive (auto-scale)
mail addresses become "fixed" by hitting space as in the screenshot
mail addresses can be "unfixed"/deleted letter-wise with backspace and have a "x" to delete them at once

Related Issue

#29695
Fixes: #30645

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2018-02-28T07:19:36Z

Codecov Report

Merging #30645 into master will decrease coverage by <.01%.
The diff coverage is 60%.

@@             Coverage Diff              @@
##             master   #30645      +/-   ##
============================================
- Coverage     62.27%   62.27%   -0.01%     
  Complexity    18233    18233              
============================================
  Files          1141     1141              
  Lines         68127    68148      +21     
  Branches       1230     1231       +1     
============================================
+ Hits          42428    42441      +13     
- Misses        25336    25346      +10     
+ Partials        363      361       -2

Flag	Coverage Δ	Complexity Δ
#javascript	`52.02% <58.33%> (+0.03%)`	`0 <0> (ø)`	⬇️
#phpunit	`63.44% <100%> (-0.01%)`	`18233 <0> (ø)`

Impacted Files	Coverage Δ	Complexity Δ
apps/systemtags/appinfo/app.php	`13.95% <ø> (-0.34%)`	`0 <0> (ø)`
lib/private/legacy/template.php	`49.37% <100%> (+0.31%)`	`39 <0> (ø)`	⬇️
core/js/sharedialogmailview.js	`64.28% <58.33%> (-0.33%)`	`0 <0> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1740027...5430beb. Read the comment docs.

lordelix · 2018-02-28T07:24:31Z

core/js/sharedialogmailview.js

+		afterRender: function () {
+			this.$el.find('.emailPrivateLinkForm--emailField').select2({
+				// data:[{id:0,text:'enhancement'},{id:1,text:'bug'},{id:2,text:'duplicate'},{id:3,text:'invalid'},{id:4,text:'wontfix'}],
+				tags:["hi"],


select2 lib throws Uncaught TypeError: tags is not a function in select2.js line 1210:
https://cdnjs.cloudflare.com/ajax/libs/select2/3.5.3/select2.js

I can't find the problem @ all. :-|
Help me @PVince81 Wan Kenobi you're my only hope.

It collides with the "tags" funcion of the select2 component. You might want to try either setting it via html element with "data-tags" attribute, or try to set it after creating the elements via "data" function.

@jvillafanez no, this syntax is correct. Please read my comments below. It's a select2 bug that was fixed in the newest version

PVince81 · 2018-02-28T10:29:45Z

this is indeed very weird. select2 is using the function tags which I see is defined on line 537 but somehow is not accessible within the scope where the error occurs 😕

PVince81 · 2018-02-28T10:37:20Z

If I add a row var wtf = tags; after the function definition of tags and use wtf(), the function is defined... It almost looks like Javascript somehow refuses to bind this external function into the internal closure... Has anything changed in Javascript recently ??

PVince81 · 2018-02-28T10:42:48Z

This does work correctly:

(function () {
	function inner() {
		console.log('inner called and wants its JS logic back');
	}

	function another() {
		console.log('another');
		inner();
	}

	window.setTimeout(another, 5000);
})();

not sure what is so different in the select2 code that the tags function is suddenly not defined any more.

PVince81 · 2018-02-28T10:53:53Z

I added some console log statements and found out that "tags" are still defined in prepareOpts for multi but then suddenly in prepareOpts in the abstract class it's not defined any more?!

PVince81 · 2018-02-28T10:56:42Z

Maybe the bug was always there but we never bumped into it through our other use cases...

Next up: try and run select2.js completely alone to see if there are side effects from other libraries...

PVince81 · 2018-02-28T11:07:08Z

Put this in the owncloud root and open it through the OC web server:

<html>
<head>
	<script src="core/vendor/jquery/dist/jquery.min.js"></script>
	<script src="core/vendor/select2/select2.js"></script>
</head>
<body>

	<div>Testing how far away from logic we are with select2:</div>

	<input class="public-link-modal--input emailPrivateLinkForm--emailField" id="emailPrivateLinkField-123" value="" type="hidden" />

	<script type="text/javascript">
		function start() {
			console.log('select2 init');
			$('.emailPrivateLinkForm--emailField').select2({
				// data:[{id:0,text:'enhancement'},{id:1,text:'bug'},{id:2,text:'duplicate'},{id:3,text:'invalid'},{id:4,text:'wontfix'}],
				tags:["hi"],
				tokenSeparators:[","]
			});
		}

		$(document).ready(start);
	</script>

</body>
</html>

The same problem happens... maybe we hit a select2 bug.

Still, I don't understand why that "tags" variable is suddenly not set any more.

Instead of trying to understand why this is happening, maybe let's try to properly use select2 by setting more options like in the example.

PVince81 · 2018-02-28T11:11:16Z

Ok, looking at the debugger I understand the error. We're back to the world of logic.

Inside the abstract prepareOpts() function, there is a statement that declares var tags = .... The Javascript interpreter will hoist this variable and make it a local variable, so this will hide the "tags" from the closure. This is a bug in select2. However since we're dealing with an EOL version there is no point in raising a bug upstream...

Now... Tag support does work in other cases, so let's find out whether our usage of select2 is passing the correct arguments.

PVince81 · 2018-02-28T11:19:03Z

I can't seem to make it work with tags even though I'm pretty close to the example in http://select2.github.io/select2/

The only difference I see is that OC has jQuery 2.1.4 while the example page has jQuery 1.7.1...

PVince81 · 2018-02-28T11:34:48Z

So... I deployed the github example page http://select2.github.io/select2/ locally and reduced it to the minimum. It works on the github page of select2 but not in my local HTML page. Then I copied over select2.js and select2.css files from the github repo to the local folder and suddenly it works !

PVince81 · 2018-02-28T11:39:00Z

Ohhhhhhh... it seems the version deployed on the example page contains a fix that uses "tagData" instead of "tags". Sneaky!

Let's try upgrading to 3.5.4...

PVince81 · 2018-02-28T11:42:53Z

raised select2/select2#5239

PVince81 · 2018-02-28T11:44:39Z

pushed select2 update to 3.5.4 which fixes the issue

@felixheidecke please carry on.

lordelix · 2018-03-01T08:06:58Z

@PVince81 … many thanks to you. I knew select2 4.0.5 exists did want to risk the major jump. I really didn't see version lurking in the shadows 3.5.4 there. Thank you for your effort 👍

PVince81 · 2018-03-02T08:56:16Z

It seems the autocomplete ajax request has gone missing on stable10: #30669.

Instead of bringing it back as it was before, please wire select2 to a function that makes the server call. It should be possible to let select2 do an ajax call to retrieve a list of "tags" matching a given pattern.

PVince81 · 2018-03-02T14:32:18Z

@felixheidecke please set the labels as we usually do

lordelix · 2018-03-05T13:16:24Z

add share link to contact email
make mail sending work

I added back the "share link to contact email" functionality. Please have a look if this is what we intended. @PVince81 do you agree with how it works?

PVince81 · 2018-03-05T14:11:05Z

core/js/sharedialogmailview.js

+			this.$el.find('.emailPrivateLinkForm--emailField').select2({
+				containerCssClass: 'emailPrivateLinkForm--dropDown',
+				tags: [],
+				tokenSeparators:[","],


that doesn't seem to work currently, not sure why: try typing email addresses and hitting the comma

Try typing email addresses and hitting enter. :-) There is no more comma to be used.

PVince81 · 2018-03-05T14:11:38Z

core/js/sharedialogmailview.js

+					},
+					results: function (data, page, query) {
+
+						if (data.status != 'success')


did we have a minimum char limit in the old implementation or did we start searching at one char ?

Good point. I'll start @ 3 since this is the minimum username length.

PVince81 · 2018-03-19T15:22:05Z

@felixheidecke correction: please remove the autocomplete completely. No need to waste more time on this. I'll remove the backend part here: #30670

lordelix · 2018-03-21T12:00:37Z

I just rebased the master onto this branch:

@phil-davis If drone ci keeps failing then I don't know whats going on. :-(

phil-davis · 2018-03-21T12:09:09Z

It is waiting to run now, so we will see.

Update: webUI tests pass, so no issue with recent acceptance test refactoring.
You will have to look at the JS and PHP unit fails.

PVince81 · 2018-03-23T14:47:28Z

please confirm if this has been retested and all cases work then we can merge

@phil-davis I wonder if we can get this into an acceptance test ? do you have email parsing there already ?

phil-davis · 2018-03-23T15:57:52Z

@PVince81 there is a step:

@When the user creates a new public link for the file/folder :name using the webUI with

that allows putting in the email address.

Is is not used much currently, but easily could be.

If we want to test the behavior within the field, e.g. for adding a few addresses then deleting one in the middle..., then we will need to look into bonus selenium trickery to be able to "click" the cursor into a particular place in the field before pressing backspace etc. @individual-it - needs a bit of research next week, but in general it would be useful to be able to control cursor-placement in a text field.

PVince81 · 2018-03-23T16:37:43Z

@phil-davis I think for now it might be enough to test with two email address and validating. In general I expect select2 to already work correctly.

phil-davis · 2018-03-24T01:59:35Z

Created issue #30892 and assigned myself, so I do not lose track of adding some UI acceptance test functionality for this.

PVince81 · 2018-03-26T06:57:31Z

please confirm if this has been retested and all cases work then we can merge

@felixheidecke ^

lordelix · 2018-03-27T17:09:20Z

please confirm if this has been retested and all cases work then we can merge

I can confirm that I works on whatever testing env. I have @ my disposal. Latest FF, Latest Chrome and IE11 on my VM.

Update select2 to 3.5.4 Also update yarn.lock Add tags functionality for email input Fix code formatting Start searching @ three chars Validate mails as they are typed allow adding and removing addresses, toggle mail body Move [x] to the right Remove artificial delay Adapt js tests Add new unit tests Remove select2 autocomplete from share dialog Show send message for a minimum of 2 secs Remove delay from sendMail promise return

PVince81 · 2018-03-28T08:35:39Z

stable10: #30945

lock · 2019-07-31T16:02:29Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lordelix requested a review from PVince81 February 28, 2018 07:13

lordelix force-pushed the select2-mail-input branch from 88ca467 to e868728 Compare February 28, 2018 07:20

lordelix commented Feb 28, 2018

View reviewed changes

PVince81 changed the title ~~[WIP] Add Tagging support for email~~ [WIP] Improve UX when entering email addresses for share link Feb 28, 2018

PVince81 mentioned this pull request Feb 28, 2018

Update 3.5.3 github pages to say 3.5.4 select2/select2#5239

Closed

PVince81 mentioned this pull request Mar 2, 2018

Regression: share link to contact email does not work any more #30669

Closed

PVince81 mentioned this pull request Mar 2, 2018

Improve public link share mail address input field #29695

Closed

lordelix added enhancement feature:sharing p1-urgent Critical issue, need to consider hotfix with just that issue labels Mar 5, 2018

lordelix added this to the development milestone Mar 5, 2018

PVince81 reviewed Mar 5, 2018

View reviewed changes

PVince81 added 2 - Developing and removed 4 - To release labels Mar 19, 2018

lordelix force-pushed the select2-mail-input branch 3 times, most recently from 0bdf3b0 to 45e2ac9 Compare March 21, 2018 11:54

PVince81 closed this Mar 23, 2018

PVince81 reopened this Mar 23, 2018

phil-davis mentioned this pull request Mar 24, 2018

Automated UI tests for email-address style fields #30892

Closed

1 task

lordelix closed this Mar 27, 2018

lordelix reopened this Mar 27, 2018

lordelix force-pushed the select2-mail-input branch from 5c4c264 to 5430beb Compare March 27, 2018 17:12

lordelix merged commit 3d3742f into master Mar 28, 2018

lordelix deleted the select2-mail-input branch March 28, 2018 08:24

lordelix removed the 2 - Developing label Mar 28, 2018

lordelix mentioned this pull request Mar 28, 2018

[stable10] select2 mail input #30945

Merged

9 tasks

PVince81 removed the backport-request label Mar 28, 2018

PVince81 mentioned this pull request Apr 3, 2018

Email autocomplete for link shares #30987

Merged

9 tasks

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Improve UX when entering email addresses for share link #30645

Improve UX when entering email addresses for share link #30645

Uh oh!

Conversation

lordelix commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov bot commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

PVince81 commented Feb 28, 2018

Uh oh!

lordelix commented Mar 1, 2018

Uh oh!

PVince81 commented Mar 2, 2018

Uh oh!

PVince81 commented Mar 2, 2018

Uh oh!

lordelix commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Mar 19, 2018

Uh oh!

lordelix commented Mar 21, 2018

Uh oh!

phil-davis commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Mar 23, 2018

Uh oh!

phil-davis commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

lordelix commented Feb 28, 2018 •

edited

Loading

codecov bot commented Feb 28, 2018 •

edited

Loading

PVince81 commented Feb 28, 2018 •

edited

Loading

PVince81 commented Feb 28, 2018 •

edited

Loading

lordelix commented Mar 5, 2018 •

edited

Loading

phil-davis commented Mar 21, 2018 •

edited

Loading

phil-davis commented Mar 23, 2018 •

edited

Loading