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

[GH-#10878] Fix bug of Getting Start Widget Accept Focus #11807

Conversation

WhiteHsu
Copy link
Contributor

@WhiteHsu WhiteHsu commented Oct 27, 2022

What it does

Closes #10878.
The fix for the bug #10878 to make the Getting Start Widget could be focused.

How to test

  1. Open the application with no workspace.
  2. Try to focus the Getting Started Widget.
  3. For now the widget will be marked active (brighter text) .

Review checklist

Reminder for reviewers

However, the console will still show the warning message:

root WARN Widget was activated, but did not accept focus after 2000ms: getting.started.widget

I've tried many different ways to fire the focus, but no way can resolve this warning message. May I know whether we must resolve the warning, or it should be ok because the Getting Start Widget can be focused properly for now?
image

Please kindly suggest and feedback. Thank you very much.

@WhiteHsu

This comment was marked as resolved.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 👍

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

@WhiteHsu

This comment was marked as resolved.

@vince-fugnitto

This comment was marked as resolved.

@WhiteHsu WhiteHsu force-pushed the bug-10878_Getting_Start_Widget_Not_Accept_Focus branch from 9380f7f to 9b187a3 Compare October 27, 2022 16:39
@WhiteHsu WhiteHsu force-pushed the bug-10878_Getting_Start_Widget_Not_Accept_Focus branch from 9b187a3 to 83fa879 Compare October 27, 2022 16:42
@WhiteHsu

This comment was marked as resolved.

@vince-fugnitto

This comment was marked as resolved.

@WhiteHsu
Copy link
Contributor Author

@vince-fugnitto got it and thanks!

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@WhiteHsu for reference theia-blueprint (downstream product) made a change to fix the issue where they could successfully accept the focus:

https://github.com/eclipse-theia/theia-blueprint/pull/201/files

@WhiteHsu
Copy link
Contributor Author

hi @vince-fugnitto Yes I've learned from that, then the Getting Started Widget could be focused now.

@vince-fugnitto
Copy link
Member

@WhiteHsu I had referenced the theia-blueprint commit since your changes don't actually seem to work as the widget still warns about focus not being accepted:

logger-protocol.ts:113 2022-10-28T12:47:45.505Z root WARN Widget was activated, but did not accept focus after 2000ms: getting.started.widget

You'll need to modify getting-started-contribution.ts to pass activate to the openView methods:

async onStart(app: FrontendApplication): Promise<void> {
if (!this.workspaceService.opened) {
this.stateService.reachedState('ready').then(
() => this.openView({ reveal: true })
);
}
}
override registerCommands(registry: CommandRegistry): void {
registry.registerCommand(GettingStartedCommand, {
execute: () => this.openView({ reveal: true }),
});
}

And ensure that onActivateRequest for the widget itself can properly accept focus (ex: focus on the first available link)

@vince-fugnitto vince-fugnitto added the getting-started issues related to the getting-started extension label Oct 28, 2022
@WhiteHsu
Copy link
Contributor Author

@vince-fugnitto Ah i see! Sorry for my miss! Thanks a lot for your kind feedback and I'll revise it accordingly!

Signed-off-by: White Hsu <[email protected]>

==
To modify getting-started-contribution.ts to pass activate to the openView methods.
To focus on the first available link in getting-started-widget.tsx.
[Question] But the warning below still exists. May I ask your kind suggestion? Thank you very much.
--
WARN Widget was activated, but did not accept focus after 2000ms: getting.started.widget
--
@WhiteHsu WhiteHsu force-pushed the bug-10878_Getting_Start_Widget_Not_Accept_Focus branch 2 times, most recently from ad084fc to ca0f332 Compare November 17, 2022 10:00
@WhiteHsu
Copy link
Contributor Author

WhiteHsu commented Nov 17, 2022

hi @vince-fugnitto

Many thanks for your kind feedback. I've revised further but the warning still exist. What I've updated are:

  1. To modify getting-started-contribution.ts to pass activate to the openView methods.
  2. To focus on the first available link in getting-started-widget.tsx.

However the warning still there:

"WARN Widget was activated, but did not accept focus after 2000ms: getting.started.widget"

I've tried to accept focus on many different elements include:

  • gs-container
  • The rendered Header
  • The rendered class gs-hr
  • The rendered class flex-grid
  • The rendered class col
  • The rendered links with class as gs-action-container

But nether can resolve the warning. May I ask your kind suggestion? Please kindly review my code changes here. Thank you very much.

@vince-fugnitto
Copy link
Member

@WhiteHsu I believe that only certain elements can actually accept focus based on the following documention:

The HTMLElement.focus() method sets focus on the specified element, if it can be focused.

I believe that in theia-blueprint they correctly focus on the input field (which can accept focus), but in our case we might not have an element which can accept the focus. I would need to investigate more.

@WhiteHsu
Copy link
Contributor Author

WhiteHsu commented Nov 23, 2022

@vince-fugnitto Thanks for your kind feedback!

Because this bug was mentioning the dysfunctional to highlight the widget after focusing it as the description below, this bug has already been resolved as the functional perspective. Therefore should we close this ticket first, and I can open another ticket to mention the "warning", then it'll be clearer for project management? :-)

Observe that the tab is never marked active (brighter text)

Cc @JonasHelming

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Nov 23, 2022

but in our case we might not have an element which can accept the focus. I would need to investigate more.

There are quite a few elements in the Getting Started widget that can accept focus. For example, this might work:

this.node.getElementsByTagName('a')[0]?.focus();

That would put focus on the first 'command link' in the widget, which in the browser is Open.

@WhiteHsu
Copy link
Contributor Author

ah i see @colin-grant-work . thanks for suggestion!

I'll try it again on the weekend!

@WhiteHsu WhiteHsu force-pushed the bug-10878_Getting_Start_Widget_Not_Accept_Focus branch from ca0f332 to de59f4c Compare January 3, 2023 03:55
@WhiteHsu
Copy link
Contributor Author

WhiteHsu commented Jan 3, 2023

Thanks for the suggestion @colin-grant-work @vince-fugnitto , and it seems workable with getElementsByTagName('a') to accept focus!

Please kindly help review and feedback. Thank you very much!

image

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

I think this code could be subject to a bug that can be prevented fairly easily.

@WhiteHsu WhiteHsu force-pushed the bug-10878_Getting_Start_Widget_Not_Accept_Focus branch from de59f4c to e59e7e4 Compare January 4, 2023 02:04
…hub.com:WhiteHsu/theia into bug-10878_Getting_Start_Widget_Not_Accept_Focus

==
To modify getting-started-contribution.ts to pass activate to the openView methods.
To focus on the first available link in getting-started-widget.tsx.
==
@WhiteHsu WhiteHsu force-pushed the bug-10878_Getting_Start_Widget_Not_Accept_Focus branch from e59e7e4 to 0b49087 Compare January 4, 2023 02:12
@WhiteHsu
Copy link
Contributor Author

WhiteHsu commented Jan 4, 2023

Hi @colin-grant-work
Many thanks for your kind feedback, which is very helpful!

I've updated codes accordingly also merge the imports to follow Lints. Please kindly help review again. :-)

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

The changes look good to me, and the widget now accepts focus. Once CI is green, I'll merge.

@colin-grant-work colin-grant-work merged commit 066b541 into eclipse-theia:master Jan 4, 2023
@colin-grant-work colin-grant-work added this to the 1.34.0 milestone Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
getting-started issues related to the getting-started extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting Started Widget doesn't accept focus
3 participants