File dialog Enhancements #8748

kenneth-marut-work · 2020-11-12T22:14:23Z

Signed-off-by: Kenneth Marut kenneth.marut@ericsson.com

What it does

Addresses #2815 by improving the browser file dialog to allow for absolute path text input (with directory auto suggestion) in addition to the existing select input , also adds a new icon to "Navigate Upward" and fixes minor styling issue with navigation icon focus on enabled/disabled states. Tested on Linux and Windows.

How to test

Pull this branch and build, start in Browser Mode
Use the file picker, you should see 2 new icons, an up arrow and a 'edit' icon inside the select input
Test that the up arrow behaves as it would on a native file picker, also observe that the existing icons' appearance remain disabled even when they are focused (this is not the case on master)
Use the file picker as you normally would, navigate to various directories
Switch to the text-input method by clicking the 'edit' icon, you should be brought to the absolute path of where you left off
Navigate using text input (the input only allows absolute paths), the first matched (alphabetical order) directory will be auto suggested as you type. Press 'Tab' to accept the suggestion and press 'Enter' to navigate to the specified directory, 'Escape' will cancel your input .
Observe the backward/forward/up/home behavior, does it behave as expected?
Switch back to the select input by blurring the text input, clicking the icon, or by pressing escape. The input should revert to its original state.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

packages/filesystem/src/browser/file-dialog/file-dialog-model.ts

packages/filesystem/src/browser/style/file-dialog.css

colin-grant-work · 2020-11-17T19:27:01Z

Very nice work, and a vast improvement over the current dialog. Some thoughts on the functionality:

there's an icon to get into editing mode, so I think maybe there should be an icon to get out of editing mode, too.
it might be nice to automatically transition to editing mode if the user has focused the select element and then starts typing.
alternatively, this may be a case for an input-with-select type interaction, where the user can easily use either mode without having to switch explicitly
I know we discussed the challenges involved, but I think it would be nice to have an auto-complete list / suggestions in edit mode.

kenneth-marut-work · 2020-11-18T17:11:25Z

Thanks for taking a look @colin-grant-work. I agree with your requests on keeping the icon around in both input fields, and also adding automatic toggle to input mode when the user starts typing.
I think after some discussion, autocomplete functionality would be the preferable way to go since the current way this new feature works might not be too intuitive (file dialog automatically navigates as path is edited and resolved). I'll spend some exploring this idea.

kenneth-marut-work · 2020-12-07T19:32:07Z

Very nice work, and a vast improvement over the current dialog. Some thoughts on the functionality:

there's an icon to get into editing mode, so I think maybe there should be an icon to get out of editing mode, too.

it might be nice to automatically transition to editing mode if the user has focused the select element and then starts typing.

alternatively, this may be a case for an input-with-select type interaction, where the user can easily use either mode without having to switch explicitly

I know we discussed the challenges involved, but I think it would be nice to have an auto-complete list / suggestions in edit mode.

I've pushed a new revision that changes the input behavior to autosuggest the closest matched directory. Additionally I've addressed your comments above:

Added a icon to toggle back to 'select' mode
If dropdown is selected and user starts typing, the input field will transition to a the text input

@colin-grant-work would you mind taking another look when you have a chance?

colin-grant-work

Working very nicely. I have a few minor comments about the code, but I think the functionality is all in place.

packages/filesystem/src/browser/style/file-dialog.css

packages/filesystem/src/browser/file-dialog/file-dialog.ts

packages/filesystem/src/browser/location/location-renderer.tsx

kenneth-marut-work · 2021-01-04T17:46:28Z

@vince-fugnitto thanks for the review, I have addressed your comments as follows:

nit: it should be placed under a new ## v1.10.0 entry, but I can always update it when preparing the changelog for the upcoming release.

I have updated the changelog with the next release 👍

Ideally we should not hardcode values (such as height and width) but rather calculate them

I have added variable names for new rules that I have introduced (where acceptable) and also updated some existing rules that were relevant

I believe this should be added to the changelog as a breaking change since downstream clients who extend the file-dialog will be broken. It is also a breaking change due to: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#property-injection.

Thanks for pointing this out, I've made some slight adjustments to the file-dialog constructor and the location-renderer constructor to make the FileService a required constructor argument. I had originally made this optional (along with some conditional logic inside the location-renderer to avoid breaking changes (and slightly overcomplicate the logic), but I think it's best to simplify the logic and accept the breaking change. I've pushed this change and updated the changelog.

vince-fugnitto

The changes work very well, I tried multiple different use-cases and they all seem correctly handled. If possible I'd like someone else to review as well to see if I may have missed something during the review.

CHANGELOG.md

colin-grant-work · 2021-01-21T17:43:37Z

Pulled the code to have another look and found that I'm not getting any focus behavior as I tab through the buttons. There's a rule to that effect on master and in the new code:

.dialogContent .theia-NavigationPanel span:focus 
{
    outline: none;
    border: none;
    box-shadow: none;
}

But I'm not sure that's desirable? Perhaps this rule should only apply, like this one, when those buttons are disabled:

.dialogContent .theia-NavigationBack.theia-mod-disabled:focus,
.dialogContent .theia-NavigationForward.theia-mod-disabled:focus,
.dialogContent .theia-NavigationHome.theia-mod-disabled:focus,
.dialogContent .theia-NavigationUp.theia-mod-disabled:focus
{
    opacity: var(--theia-mod-disabled-opacity) !important;
}

Otherwise it looks and feels good. Fix the conflict in the CHANGELOG, and it should be good to go.

kenneth-marut-work · 2021-01-21T23:21:53Z

Pulled the code to have another look and found that I'm not getting any focus behavior as I tab through the buttons. There's a rule to that effect on master and in the new code:
.dialogContent .theia-NavigationPanel span:focus 
{
    outline: none;
    border: none;
    box-shadow: none;
}
But I'm not sure that's desirable? Perhaps this rule should only apply, like this one, when those buttons are disabled:
.dialogContent .theia-NavigationBack.theia-mod-disabled:focus,
.dialogContent .theia-NavigationForward.theia-mod-disabled:focus,
.dialogContent .theia-NavigationHome.theia-mod-disabled:focus,
.dialogContent .theia-NavigationUp.theia-mod-disabled:focus
{
    opacity: var(--theia-mod-disabled-opacity) !important;
}
Otherwise it looks and feels good. Fix the conflict in the CHANGELOG, and it should be good to go.

I've rebased and fixed the changelog, also made some modifications to the CSS to allow the toolbar elements to apply focus only when enabled. See below:

The focus is only applied when an enabled toolbar item is clicked or tabbed too. Note that the focus border stays visible until something else is focused (even on click). For accessibility reasons, this is nice, but it's not necessarily pretty.

@vince-fugnitto do you have any input on how to best handle focusing toolbar items in Theia? It seems that in some places focus is applied on click, and not in others. Also note that on master there is no focus behavior for the file dialog toolbar items.

vince-fugnitto · 2021-01-21T23:50:35Z

I've rebased and fixed the changelog, also made some modifications to the CSS to allow the toolbar elements to apply focus only when enabled. See below:
focusing

The focus is only applied when an enabled toolbar item is clicked or tabbed too. Note that the focus border stays visible until something else is focused (even on click). For accessibility reasons, this is nice, but it's not necessarily pretty.

@vince-fugnitto do you have any input on how to best handle focusing toolbar items in Theia? It seems that in some places focus is applied on click, and not in others. Also note that on master there is no focus behavior for the file dialog toolbar items.

@kenneth-marut-work @colin-grant-work I'm agree with you that the outline is not necessarily pretty but for accessibility when tabbing it is likely the best solution, I just wish the icons were all of equal size (square) similarly to vscode:

toggle.mov

As for onClick, I'm thinking we can do something similar to the toolbar items by providing visual feedback ('bounce') to end-users when a toolbar item is clicked (but not the outline), similarly to #6099. Let me know if this is an interesting proposal.

feedback.mov

kenneth-marut-work · 2021-01-22T17:28:18Z

@vince-fugnitto thanks for the suggestions, both are great ideas. I went ahead and pushed a change that includes:

'bounce' effect on clicking an active item
Evenly sized focus borders
Focus only on keyboard access (using the focus-visible rule)

@colin-grant-work and @vince-fugnitto let me know what you think. I know that there is still room to improve the file dialog, but I don't want to go too far beyond the scope of this MR 🙂

vince-fugnitto · 2021-01-22T17:54:11Z

@colin-grant-work and @vince-fugnitto let me know what you think. I know that there is still room to improve the file dialog, but I don't want to go too far beyond the scope of this MR

I think it looks much better, happy the suggestion was useful! 👍 I'll perform another round of review just to be sure that everything works as intended.

vince-fugnitto

The changes look good to me 👍

packages/filesystem/src/browser/location/location-renderer.tsx

colin-grant-work · 2021-02-10T15:49:56Z

packages/filesystem/src/browser/location/location-renderer.tsx

+                const latestInput = this.pendingResolvedDirectories.get(directoryToResolve)?.input || '';
+                const childDirectories = children.filter(child => child.isDirectory)
+                    .map(directory => `${directory.resource.path}/`)
+                    .sort();


Do they come out of (alphabetical) order to begin with? Usually fs.readDir returns things in alphabetical order, but perhaps resolve behaves differently.

You're right, I've removed .sort()

packages/filesystem/src/browser/location/location-renderer.tsx

kenneth-marut-work · 2021-02-10T20:31:01Z

Thanks for the comments @colin-grant-work. I've made some changes that address your issues. I've tested in both low and high latency

colin-grant-work · 2021-02-22T16:03:15Z

The new changes look good to me 👍

- Add text input to locationList Renderer - Add 'navigate upward' icon - Fix icon focus behavior when disabled Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com>

vince-fugnitto

I verified the changes and they work really well 👍

I found a future improvement which can be made for both macos and linux. The idea to support ~ as a replacement for the user's home when editing the path (which is something that is supported in the native dialog).

kenneth-marut-work · 2021-03-01T20:32:03Z

I verified the changes and they work really well 👍

I found a future improvement which can be made for both macos and linux. The idea to support ~ as a replacement for the user's home when editing the path (which is something that is supported in the native dialog).

Thanks for the review, I'm going ahead with the merge and I'll create an issue regarding the ~ suggestion

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch 3 times, most recently from cf9db8b to fbc77c3 Compare November 13, 2020 18:58

kenneth-marut-work changed the title ~~WIP: File dialog Enhancements~~ File dialog Enhancements Nov 13, 2020

kenneth-marut-work marked this pull request as ready for review November 13, 2020 21:24

kenneth-marut-work requested review from colin-grant-work and kittaakos November 13, 2020 21:24

colin-grant-work reviewed Nov 17, 2020

View reviewed changes

packages/filesystem/src/browser/file-dialog/file-dialog-model.ts Outdated Show resolved Hide resolved

packages/filesystem/src/browser/style/file-dialog.css Show resolved Hide resolved

packages/filesystem/src/browser/style/file-dialog.css Show resolved Hide resolved

colin-grant-work added enhancement issues that are enhancements to current functionality - nice to haves dialogs issues related to dialogs file dialog issues related to the file dialog ui/ux issues related to user interface / user experience labels Nov 17, 2020

kenneth-marut-work changed the title ~~File dialog Enhancements~~ WIP: File dialog Enhancements Dec 3, 2020

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch 7 times, most recently from 7fd234e to ca0b37b Compare December 7, 2020 16:55

kenneth-marut-work closed this Dec 7, 2020

kenneth-marut-work reopened this Dec 7, 2020

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch from ca0b37b to 6de2965 Compare December 7, 2020 17:43

kenneth-marut-work changed the title ~~WIP: File dialog Enhancements~~ File dialog Enhancements Dec 7, 2020

colin-grant-work reviewed Dec 10, 2020

View reviewed changes

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch 2 times, most recently from 7e99b8c to a194c8c Compare December 21, 2020 20:14

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch 3 times, most recently from b5c2cee to f707600 Compare January 4, 2021 17:28

vince-fugnitto reviewed Jan 7, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

vince-fugnitto requested a review from paul-marechal January 7, 2021 17:52

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch from f707600 to 03528ba Compare January 11, 2021 14:06

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch from 03528ba to 09aa93f Compare January 21, 2021 23:13

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch from 09aa93f to 3a81f56 Compare January 22, 2021 17:11

vince-fugnitto approved these changes Jan 25, 2021

View reviewed changes

paul-marechal reviewed Jan 25, 2021

View reviewed changes

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch from 3a81f56 to f3191b0 Compare January 26, 2021 22:41

colin-grant-work reviewed Feb 10, 2021

View reviewed changes

packages/filesystem/src/browser/location/location-renderer.tsx Outdated Show resolved Hide resolved

colin-grant-work reviewed Feb 10, 2021

View reviewed changes

packages/filesystem/src/browser/location/location-renderer.tsx Outdated Show resolved Hide resolved

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch from f3191b0 to d47c8ff Compare February 10, 2021 20:03

colin-grant-work approved these changes Feb 22, 2021

View reviewed changes

File dialog enhancements

4cd3cde

- Add text input to locationList Renderer - Add 'navigate upward' icon - Fix icon focus behavior when disabled Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com>

kenneth-marut-work force-pushed the feature/file-dialog-enhancements branch from d47c8ff to 4cd3cde Compare March 1, 2021 18:54

vince-fugnitto approved these changes Mar 1, 2021

View reviewed changes

kenneth-marut-work merged commit ab74a91 into eclipse-theia:master Mar 1, 2021

kenneth-marut-work mentioned this pull request Mar 1, 2021

Support ~ as replacement for HOME directory in file dialog text-input #9142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File dialog Enhancements #8748

File dialog Enhancements #8748

kenneth-marut-work commented Nov 12, 2020 •

edited by vince-fugnitto

Loading

colin-grant-work commented Nov 17, 2020 •

edited

Loading

kenneth-marut-work commented Nov 18, 2020

kenneth-marut-work commented Dec 7, 2020 •

edited

Loading

colin-grant-work left a comment

kenneth-marut-work commented Jan 4, 2021

vince-fugnitto left a comment

colin-grant-work commented Jan 21, 2021

kenneth-marut-work commented Jan 21, 2021

vince-fugnitto commented Jan 21, 2021

kenneth-marut-work commented Jan 22, 2021

vince-fugnitto commented Jan 22, 2021

vince-fugnitto left a comment

colin-grant-work Feb 10, 2021

kenneth-marut-work Feb 10, 2021

kenneth-marut-work commented Feb 10, 2021

colin-grant-work commented Feb 22, 2021

vince-fugnitto left a comment

kenneth-marut-work commented Mar 1, 2021

File dialog Enhancements #8748

File dialog Enhancements #8748

Conversation

kenneth-marut-work commented Nov 12, 2020 • edited by vince-fugnitto Loading

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work commented Nov 17, 2020 • edited Loading

kenneth-marut-work commented Nov 18, 2020

kenneth-marut-work commented Dec 7, 2020 • edited Loading

colin-grant-work left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented Jan 4, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

colin-grant-work commented Jan 21, 2021

kenneth-marut-work commented Jan 21, 2021

vince-fugnitto commented Jan 21, 2021

kenneth-marut-work commented Jan 22, 2021

vince-fugnitto commented Jan 22, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

colin-grant-work Feb 10, 2021

Choose a reason for hiding this comment

kenneth-marut-work Feb 10, 2021

Choose a reason for hiding this comment

kenneth-marut-work commented Feb 10, 2021

colin-grant-work commented Feb 22, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented Mar 1, 2021

kenneth-marut-work commented Nov 12, 2020 •

edited by vince-fugnitto

Loading

colin-grant-work commented Nov 17, 2020 •

edited

Loading

kenneth-marut-work commented Dec 7, 2020 •

edited

Loading