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

fix(ui5-dialog): fix text selection on chrome #3532

Merged
merged 5 commits into from
Aug 3, 2021

Conversation

georgimkv
Copy link
Contributor

@georgimkv georgimkv commented Jul 23, 2021

On Chrome, clicking on slotted elements (light DOM) in the
dialog (shadow DOM) causes the focus to travel up the light DOM tree to
reach the closes element that is focusable.
Which ends to be the body element as document.activeElement.

This is not the case for FF & Safari, where the focus travels up the
slot, reaching the dialog root which is actually the closest focusable element.

This change introduces a workaround to align the focus behaviour in Chrome
with the other browsers.

Related issue: WICG/webcomponents#773
Fixes #3466

Copy link
Contributor

@dimovpetar dimovpetar left a comment

Choose a reason for hiding this comment

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

Text selection is still not possible in the following samples:
"resizable dialog"
"draggable & resizable dialog"
"empty dialog (no focusable element)"

@ilhan007 ilhan007 closed this Jul 24, 2021
@ilhan007 ilhan007 reopened this Jul 24, 2021
Copy link
Contributor

@fifoosid fifoosid left a comment

Choose a reason for hiding this comment

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

The build for this PR is fixed, but there is a failing test for the Popover, which is caused by this change

WICG/webcomponents#773

On Chrome, clicking on slotted elements (light DOM) in the
dialog (shadow DOM) causes the focus to travel up the light DOM tree to
reach the closes element that is focusable.
Which ends to be the body element as document.activeElement.

This is not the case for FF & Safari, where the focus travels up the
slot, reaching the dialog root which is actually the closest focusable element.

This change introduces a workaround to align the focus behaviour in Chrome
with the other browsers.

Fixes #3466
@georgimkv georgimkv force-pushed the dialog_text_selection_fix branch from d7e4327 to b68bfdf Compare July 26, 2021 11:31
@georgimkv georgimkv requested a review from dimovpetar July 26, 2021 11:32
Copy link
Contributor

@alexandar-mitsev alexandar-mitsev left a comment

Choose a reason for hiding this comment

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

  • add a unit test
  • fix is not working for sample "Open empty dialog (no focusable element)" on Dialog.html test page

@georgimkv
Copy link
Contributor Author

  • add a unit test
  • fix is not working for sample "Open empty dialog (no focusable element)" on Dialog.html test page

I couldn't find a way to select text with wdio to test this.

Fixed the case for the sample "Open empty dialog (no focusable element)"

@georgimkv georgimkv dismissed stale reviews from alexandar-mitsev and dimovpetar August 2, 2021 10:18

adressed comments

@ilhan007
Copy link
Member

ilhan007 commented Aug 2, 2021

Is the following expected?
In Chrome the selection of the text remains, when you release the mouse, while in FF and Safari - the selection is lost.

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Let's wait for the last comment to be resolved

@georgimkv georgimkv dismissed ilhan007’s stale review August 2, 2021 17:15

addressed comments

Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Lint error

15:1 error @ui5/webcomponents-base/dist/Device.js import should occur before import of ./generated/templates/PopupTemplate.lit.js import/order

@ilhan007 ilhan007 dismissed their stale review August 3, 2021 06:49

outdated

@ilhan007 ilhan007 merged commit 1abd40d into master Aug 3, 2021
@ilhan007 ilhan007 deleted the dialog_text_selection_fix branch August 3, 2021 07:09
ilhan007 pushed a commit that referenced this pull request Aug 3, 2021
On Chrome, clicking on slotted elements (light DOM) in the
dialog (shadow DOM) causes the focus to travel up the light DOM tree to
reach the closes element that is focusable.
Which ends to be the body element as document.activeElement.

This is not the case for FF & Safari, where the focus travels up the
slot, reaching the dialog root which is actually the closest focusable element.

This change introduces a workaround to align the focus behaviour in Chrome
with the other browsers.

Related issue: WICG/webcomponents#773
Fixes #3466
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialog: Not able to select text inside it
6 participants