Problem/Motivation

When both Settings Tray and Quick Edit module are enabled then you can edit certain inputs in QuickEdit.

Steps to reproduce:

  1. Enable both Settings Tray and Quick Modules
  2. Create an article
  3. View the article
  4. Enter Settings Tray "Edit" mode by clicking "Edit" in the toolbar
  5. Click the article body text to open QuickEdit toolbar.
  6. Click on author name to edit
  7. The "author" input textbox appear.
  8. Attempt to click in the text box (you can't!)

We need to fix the last step.

Proposed resolution

We need to fix the CSS pointer-events: none; that stops this.

Remaining tasks

Review patch

User interface changes

None, it will just work.

API changes

None

Data model changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow’s picture

Status: Active » Needs review
FileSize
1.34 KB

Ok the fix here is actually very simple. Just don't apply the pointer-events css to .quickedit-editing fields.

I am going go back on what I said in #2 and hope we could get this and then add specific tests in follow up after #2828528: Add Quick Edit Functional JS test coverage is fixed.

tedbow’s picture

tedbow’s picture

Issue summary: View changes
Issue tags: -JavaScript, -Needs tests +CSS
tedbow’s picture

Issue summary: View changes
starshaped’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
406.85 KB
377.54 KB

Tested the patch in #3 and I can confirm the fields can be edited now. Attached screenshots before and after the patch was applied to confirm this change works! Setting to RTBC.

xjm’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs usability review

Hmmm. I thought this was done deliberately to avoid confusing people with two different quick edit behaviors that had different interaction patterns, one of which would edit an individual page's content and the other of which edited the whole site. Can we check back and confirm that? Maybe a UX review would help confirm we do want to make this change.

I'm also not keen on postponing the test coverage. Tests are a core gate. So we can add the first ever test for Quick Edit here as needed. :)

I do think this change could be okay for backport during beta if we do confirm this is not by design. While there is a change to Stable, it's purely a bugfix, probably major, and unlikely to disrupt themes, and ST itself has not yet been stable before this release. After RC1 ships we might want to keep it 8.6.x only, though.

tedbow’s picture

I thought this was done deliberately to avoid confusing people with two different quick edit behaviors that had different interaction patterns, one of which would edit an individual page's content and the other of which edited the whole site.

No specifically set up the "Edit" mode for Settings Tray to work with the QuickEdit module.

In Edit mode when the user clicks anywhere on an entity that is QuickEdit enabled(from the quickedit module) the QuickEdit modules functionality is triggered and the off-canvas dialog is closed but Edit mode is not disabled.

This way in Edit mode Settings Tray and QuickEdit modules' functionality are both triggered in the same manner, by clicking on elements on the page, you don't have to use contextual links.
From settings_tray.es6.js

** Capture all clicks on the rendered quick edit item
$(quickEditItemSelector)
.not(contextualItemsSelector)
.on('click.settingstray', (e) => {
  /**
   * For all non-contextual links or the contextual QuickEdit link
   * close the off-canvas dialog.
   */
  if (!$(e.target).parent().hasClass('contextual') || $(e.target).parent().hasClass('quickedit')) {
    closeOffCanvas();
  }
  // Do not trigger if target is quick edit link to avoid loop.
  if ($(e.target).parent().hasClass('contextual') || $(e.target).parent().hasClass('quickedit')) {
    return;
  }
  *** Click the QuickEdit module's contextual link to trigger the modules functionality
  $(e.currentTarget).find('li.quickedit a').trigger('click');
});

This was done in #2782915: Standardize the behavior of links when Outside In editing mode is enabled you can see in the proposed solution there.

We also have a test for behavior \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testQuickEditLinks it just that we didn't actually test filling in the fields and submitting the form. Which we would need to do now.

tedbow’s picture

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.57 KB
2.23 KB

I don't think we can test this our current Javascript testing using Phantomjs.

Looking back at #2782915: Standardize the behavior of links when Outside In editing mode is enabled we had to the test module settings_tray_test_css because CSS pointer-events attribute is not handled correctly by PhantomJS.

The current bug is caused by the fact that when the QuickEdit inputs have pointer-events: none applied and this makes it so manually testing you can can't click on the input and have the input gain focus.

PhantomJS unfortunately does not behave the same way.

I am including 2 patches here.

One is test only patch that shows currently running the test with now fix the "author" field in quickedit gains focus when you click on it.

the other patch is the fix #3 and also the test.

Both will pass because PhantomJS doesn't have the same behaviour as browsers.

So basically even though we should still do #2944810: [PP-1] Add tests edit various fields with QuickEdit module when Settings tray is enabled. we still won't be able to test this particular bug.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

I'm also not keen on postponing the test coverage. Tests are a core gate. So we can add the first ever test for Quick Edit here as needed. :)

Setting back to RTBC because I think #11 points that we can't really test this. It is not a problem testing Quick Edit but with 'pointer-events' and phantomjs

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@tedbow I wonder if we move the test to webdriver if we can test this? To move a test to WebDriver you can add

  /**
   * {@inheritdoc}
   */
  protected $minkDefaultDriverClass = DrupalSelenium2Driver::class;

to the test class. For more instructions about how to test using webdriver see core/tests/README.md