salesforce / lwc

āš”ļø LWC - A Blazing Fast, Enterprise-Grade Web Components Foundation
https://lwc.dev
Other
1.62k stars 394 forks source link

Using lightning/datatableKeyboardMixins on platform with custom data types errors on load #1962

Open tsalb opened 4 years ago

tsalb commented 4 years ago

Description

Extending lightning-datatable with custom data types also extending baseNavigation like the below gives an aura error before lightning-datatable fully initializes.

I've used the datatableDeleteRowBtn example straight from the lightning-datatable documentation.

FYI, this issue seems it cannot be repro-ed in a playground, so I've created an sfdx project repo to go along with this.

I am not sure this issue is supposed to be logged against the lwc framework repo since the error does come up as an aura error. That being said, I am guessing that this issue has something to do with how lwc and aura interact on platform, so I thought it to be useful to log it here.

Steps to Reproduce

Does not work in a v48 and v49 scratch org: https://github.com/tsalb/lightning-datatable-accessibility-issue

Works on playground: https://developer.salesforce.com/docs/component-library/tools/playground/I93mYl8MU/19/edit

The following does not work on platform:

// datatableDeleteRowBtn.js not working
import { LightningElement, api } from 'lwc';
import { baseNavigation } from 'lightning/datatableKeyboardMixins';
import template from './datatableDeleteRowBtn.html';

export default class DatatableDeleteRowBtn extends baseNavigation(LightningElement) {
    ...
    // Required for mixins
    render() {
        return template;
    }
    ...
}

But this works fine (as expected, since it doesn't extend baseNavigation)

// datatableDeleteRowBtn.js working, import itself is not the issue
import { LightningElement, api } from 'lwc';
import { baseNavigation } from 'lightning/datatableKeyboardMixins'; // not used
import template from './datatableDeleteRowBtn.html'; // not used

export default class DatatableDeleteRowBtn extends LightningElement {
    ...
    // Required for mixins
    // render() {
    //     return template;
    // }
    ...
}

Expected Results

lightning-datatable with a custom data type extending baseNavigation initializes after first paint.

OR

lightning-datatable specification needs to be updated with more accurate information on how to leverage the baseNavigation mixin.

Actual Results

The following aura error shows up before lightning-datatable is fully initialized on platform.

afterRender threw an error in 'c:datatableExample' [Access denied: {"from":{"namespace":"system"},"to":{"namespace":"c"}}]

Browsers Affected

Chrome Latest: Version 83.0.4103.116 (Official Build) (64-bit)

Additional Context

I hit this issue attempting to extend my own custom data type here, before debugging down to this issue to ensure that the error was not from that code stack.

pmdartus commented 4 years ago

/cc @garazi.

manuel-jasso commented 4 years ago

Hi @tsalb I am from Locker team, investigating this issue. There is a bit of history I need to dig to see if extends baseNavigation(LightningElement) is supported by Locker, this may take some time, but I wanted to ask you: do you absolutely need to do that or can you as a workaround use extends LightningElement?

(Internally tracking with W-7851976)

manuel-jasso commented 4 years ago

Hi @tsalb I found today that we have another similar case internally and using extends baseNavigation(LightningElement) should work fine and is necessary for accessibility.

tsalb commented 4 years ago

Hey @manuel-jasso, thanks for checking in.

My understanding is that extends LightningElement doesn't allow for keyboard navigation (accessibility) with custom datatable data types. To your point, I am currently using this as the workaround.

Your second question is the root of this bug - the documentation (and your internal findings) says it should work fine but in reality it doesn't. You can push the linked SFDX project to a fresh summer-20 scratch org and check for yourself. I have just tried it today and I see that the error still persists.

I hope that clears it up!

manuel-jasso commented 4 years ago

Thanks @tsalb, we will continue troubleshooting this issue

AceNinjaForce commented 4 years ago

Greetings @manuel-jasso this issue is adversely impacting our team in a similar manner. The inability to keyboard navigate the datatable is impactful in two ways:

A. The overall user experience is quite lacking; considering the user has to move between the mouse and keyboard depending on which column they're navigating from/to.

B. I believe this behavior results in section 508 non-compliance (which should raise the level of priority). The two applicable sections and Salesforce's stated compliance are as follows:

From the Salesforce Accessibility Conformance Report...

As a final note, I would like to point out the known issue W-7851976 opened for this case is slightly misleading in the suggestion for the workaround.

"Do not use baseNavigation from lightning/datatableKeyboardMixins module so that lightning-datatable works fine without accessibility and keyboard navigation support."

We do not feel "works fine without accessibility and keyboard navigation" is actually a workaround when users must have access to navigate via keyboard.

Thanks for your attention and the great work you are doing for the developer community.

manuel-jasso commented 4 years ago

Hi @AceNinjaForce, you are right, the "workaround" is not an acceptable solution. We've had a few fires lately, but we will get back with some answer for you and @tsalb in the next few days.

Thank you!

Cc @animmakayala @seksenov @salesforce/ui-security

AceNinjaForce commented 3 years ago

Greetings @manuel-jasso, any updates on this issue by chance? Can you possibly elaborate on the priority of this issue?

Thanks!

manuel-jasso commented 3 years ago

Hi @AceNinjaForce, sorry about not posting any updates here for a while, we've had a few deep dives here and we've found the issue is actually a lot trickier than we initially thought, this pattern (class DatatableDeleteRowBtn extends baseNavigation(LightningElement) is supposed to work but the way your code is structured doesn't match any of our test cases, and this is not a trivial thing, we need to keep digging to find the nuances that make it different to create a test case that would fail the same way in order to find the root cause. But as I'm sure you know, we have a few other deliverables and priorities, and we need to balance what we do, so far we recognize this is a problem but it it is sadly a lower priority than other commitments we have, we are planning to fix this not during our current release cycle (Spring 21) but during the next one (Summer 21).

Cc @animmakayala @seksenov @salesforce/ui-security

tsalb commented 3 years ago

Thanks for the update @manuel-jasso! What about my example? In my example, I literally copy pasted from the lightning-datatable documentation and I don't think I made any mistakes.

If the docs themselves don't work, then what should?

manuel-jasso commented 3 years ago

Hi @tsalb, your code is fine and the docs are fine, we have a mismatch with our testing code inside Locker, and the error is on our side, this is part of why this will be a little trickier to solve than expected.

manuel-jasso commented 3 years ago

Hi @tsalb @AceNinjaForce sorry for the delayed update šŸ™šŸ»ā€ā™‚ļø , we did a detailed analysis and we found that Locker has a very limited support for extending from mixins, it only works if the mixin doesn't do any kind of DOM manipulation or Event handling, which unfortunately is what baseNavigation does, so it is not supported.

The only workaround we could suggest is to refactor your code to use composition rather than inheritance, I know this may not be trivial, but this is the only suggestion we have at the moment.

I also wanted to mention that we're actively working on "Locker vNext", a re-architecture of Locker, and this scenario will be supported.

tsalb commented 3 years ago

@manuel-jasso thanks for the update, was just on github and got the notification icon. Fortuitous timing!

I am not sure what the next steps are as a customer who is simply reading the docs and following the example code - and yet - it doesn't work on platform. Based on the internal framework for lightning-datatable we are not able to selectively choose composition/inheritance for custom data types (in the datatable cell).

Perhaps there is a miscommunication? My root problem is:

  1. I am following the lightning-datatable docs on the how-to for Accessibility with Custom Data Types
  2. That is not working, hence this issue I raised.

I quote, the exact docs here:

Accessibility for Custom Data Types To support accessibility and keyboard navigation on your custom data types, import baseNavigation from the lightning/datatableKeyboardMixins module. baseNavigation queries all focusable elements, such as buttons, anchors, and input fields. Use data-navigation="enable" in your custom component as a marker for focusable elements.

lightning/datatableKeyboardMixins has the keyboard-mode attribute, which determines if the user is in navigation mode or action mode. Valid options are NAVIGATION and ACTION. The default is NAVIGATION. Action mode triggers the actionable element in that cell, such as an edit icon. This example shows how you can apply the action mode to your custom data type.

Shouldn't this section be omitted from the lightning-datatable docs then? What am I missing here?

manuel-jasso commented 3 years ago

@tsalb I think we have a documentation issue, I believe we should update the docs to clarify that the "Accessibility for Custom Data Types" is not working under current Locker, but I need to discuss internally to figure out what is the right thing to do. Thanks for your patience and your detailed feedback!

AceNinjaForce commented 3 years ago

Greetings @manuel-jasso is Summer 21 still expected to resolve this issue.

manuel-jasso commented 3 years ago

Hi @AceNinjaForce, we had several cycles of internal reviews and we have a new internal ticket W-8793287 to change the components documentation, @dhruvkediaSFDC is your contact for this. In regards to the code itself and as mentioned before, this scenario will be supported in "Locker vNext", but we don't have a GA date at the moment.

scottmcook commented 3 years ago

Hello @manuel-jasso it's been awhile since a status update was posted for this issue, is extends baseNavigation(LightningElement) supported as of Summer 21' or is the suggestion still to use the aforementioned workaround?

manuel-jasso commented 2 years ago

Hello @SMCRunner88, you have to use the workaround until "Locker vNext" is GA, @seksenov is our PM if you want to know about timelines.

petergascoyne commented 1 year ago

Hi @manuel-jasso, @seksenov,

Any update custom 'datatable : custom data types' supporting keyboard navigation?

Will 'Locker vNext' still resolve this? What status/timelines?

Is there a workaround and do you have details for this?

Thanks,

manuel-jasso commented 1 year ago

Hi @petergascoyne, yes "Locker vNext" solves this issue, and there is no workaround for "Legacy Locker" šŸ™.

In terms of timelines, that's a question for @seksenov, @garychangsf, @jdalton

petergascoyne commented 1 year ago

Hi @seksenov, @garychangsf, @jdalton,

Any loose timeline on this?

Thanks

nolanlawson commented 1 year ago

@petergascoyne According to the comment above, Locker vNext (aka Lightning Web Security) solves this issue. Can you enable Lightning Web Security?

petergascoyne commented 1 year ago

Hi @nolanlawson,

Enabling LWS would be on our Package subscriber side, so not guaranteed, but can try to encourage.

Sounds like we could explorer having two custom data type components per custom type based on LWS disabled/enabled:

  1. e.g. deleteRowButton - LWS Disabled component without 'extends baseNavigation' keyboard navigation support
  2. e.g. deleteRowButtonLWS - LWS Enabled 'extends baseNavigation' keyboard navigation support

Any comments on above approach?

With following added to deleteRowButtonLWS, should we need to add anything else for the accessibility and keyboard navigation to work for custom data type? (cc: @tsalb)

Is there any documentation on this? I did try try searching with no luck.

Any loose timeline when LWS will fully replace locker and enforced?

Thanks,

nolanlawson commented 1 year ago

This ^ would be a question for @seksenov, @garychangsf, @jdalton and crew; I don't know much about Locker vs LWS.