Simplify Dropbutton markup in line with our CSS standards

No empty <a>. Parts of the code has been for JS reasons, can't remember which ones yet but expect to run into troubles with just this markup.

The proposed markup seems to be a a11y regression.

Any reason why we can't use the span in the trigger - <span class="visually-hidden">List additional actions</span>

It's way cleaner code which is great. If it is a button, shouldn't it semantically be defined that way? https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Tec...

Updating issue summary to add wrapper div for proposed solution.

My plan of attack:

* Add a dropbutton.html.twig file and associated preprocess. If single then only output a button, if multiple then a dropbutton.
* Rework css/js to get back to current UI.
* Test. A lot.

Adding more notes from my initial research.

Views UI generates markup in ViewsEditForm.php in two spots (function getDisplayDetails, function renderDisplayTop) and that needs to get updated. Ideally those two functions will call preRenderDropbutton() in Dropbutton.php so that dropbuttons are always generated in the same way.

<div class="dropbutton dropbutton--modifier"> <!-- whole component is dropbutton. -->
  <a class="button dropbutton__action">Action One</a>
  <button type="button" class="button dropbutton__trigger">
    <span class="visually-hidden">List additional actions</span>
  </button>
  <ul class="dropbutton__menu" role="menu">
    <li><a class="dropbutton__action">Action Two</a></li>
    <li><a class="dropbutton__action">Action Three</a></li>
  </ul>
</div>

Code self explanation. Cheers.

@LewisNyman, what do you think of droplet's suggested markup?

I started the work of renaming the existing structure to a classname structure when during the banana clean up.
The real issue here is that dropbottom dont uses BEM & --modifiers, or prefixes the js required classes.
Getting those things fixed at first would be important win to get. We dont wanna end up in a situation with a component that will conflict with the css, as its the case now.

<div class="js-dropbutton-wrapper js-dropbutton-processed js-dropbutton-wrapper--multiple">
  <div class="js-dropbutton-widget">
  <ul class="js-dropbutton">
  <li class="publish js-dropbutton-action">
    <input type="submit" name="op" value="Save and keep published" class="button form-submit">
  </li>
  <li class="js-dropbutton__toggle">
    <button type="button">
      <span class="js-dropbutton__toggle__arrow">
        <span class="visually-hidden">List additional actions</span>
      </span>
    </button>
  </li>
  <li class="unpublish js-dropbutton-action js-dropbutton__secondary-action">
    <input type="submit" name="op" value="Save and unpublish" class="button form-submit">
  </li>
</ul>
</div>

what we should look into is separating the classe so its clear whats used for "theme" so we dont use the js- prefixed classes for that all over.

js-prefixed all elements. pretty bad idea.

@droplet about prefix how Else would You "protect" css classes that's only used for js?

we prefix classes that are needed for the js to work.
it's how it should be done according to our CSS docs in d8

TDLR : seperate the css & js first, then play around with class names later.

@lewis well then how do you then propose we seperate the js from the css without using .js- ;)
We gotta get the seperation done first, if thats not done the rest dont matter, then were back in the same mess as d6, d7
So instead of this dance around classnames how about we start with getting the seperation done first

Rewriting everything with new markup etc, is a fine goal - but at RC-1 all markup is locked down.

@lewis ok so we turn this issue into doing that or do we still wanna rewrite it all ?

I'm at Drupalcon LA and restarting work on this issue. Combining the original CSS classes from @lewis and the required "js-" classes from @morten leaves us with something like for an open dropbutton:

<div class="dropbutton dropbutton--modifier is-open"> <!-- whole component is dropbutton. -->
  <a class="button dropbutton__action">Action One</a>
  <button type="button" class="button dropbutton__trigger">
    <span class="visually-hidden">List additional actions</span>
  </button>
  <ul class="dropbutton__menu js-dropbutton__menu" role="menu">
    <li><a class="dropbutton__action">Action Two</a></li>
    <li><a class="dropbutton__action">Action Three</a></li>
  </ul>
</div>

My plan of attack:

• Make the appropriate markup changes in /core/misc/dropbutton/dropbutton.js and /core/modules/system/templates/dropbutton-wrapper.html.twig
• Rename dropbutton-wrapper.html.twig to dropbutton.html.twig
• Refactor CSS

This patch has all the new markup as well as JS and CSS changes.

Next steps:

• CSS still needs to be formatted according to the Drupal 8 standards.
• Test, test, test!!!

Rebased against 8.0.x and found a JS error. Also cleaned up the CSS according Drupal standards, but for Bartik only at this point. Before I address the CSS changes for Seven and Classy I want to run testbot to track down what tests will need updating (xpath tests against CSS classes and when the classes change the tests too need to change).

This patch contains the following changes/additions from the previous one in 30.

• There were some mistakes in my Jquery so I've fixed those.
• Fixed whitespace errors in CSS.
• Small CSS changes for dropbutton--single.
• Rewrote xpath conditions for a few tests to accommodate the new markup.

This patch contains:

• Found some jquery implementation errors on my part.
• Fixed whitespace errors in CSS.
• Updated tests to have correct current dropbutton selectors.

Had trouble creating the interdiff this time. Sorry that it's missing.

Rerolled patch, and got the interdiff too!

This patch contains:

• Fixed more whitespace errors in CSS.
• Updated the remainder of tests to have correct current dropbutton selectors.
• Replaced the selectors in Seven with the new ones but there are issues with look/feel in Seven and Classy. I'd like to get the maintainers of those themes to weigh in on how much CSS should remain in dropbutton.css and how much should be theme specific.

Please note that even if this patch passes all tests it is NOT ready for review until the issue I raised in the 3rd bullet point is resolved.

Only change is two spaces on line 160 of RowRenderCacheTest.php without which the assertEqual on line 165 fails.

Well, it seems this still needs some work as you can see in the screenshots:

DROPDOWN BUTTON BEFORE

DROPDOWN BUTTON AFTER

Just fixed a couple of nits. I wonder what is the standard when using [dir="rtl"], in some places there is a space after and in other places no.

Which means, if we need a class for js functionality, we should use a new class that is only used for functionality

Is it possible to use data attributes for this? We really do want to get away from the js- prefixes. We're just using it as a last result of sorts to split the existing ones.

I assume that it should not apply anything on Stark

Yes. Stark should remain as close to browser defaults as possible.

For Classy, I think that it should basically look as the Seven design but without colors

Yes. I didn't pour over the CSS, but we are ok to have the structural stuff in Classy, but not colors and other styling. And so long as this is an agreed upon standard for how we want buttons or whatever to look for Drupal and the style guide. Not just the default place to dump stuff that Seven and Bartik use.

Working on a reroll for this, will also review Lewis' separation of styles between core and Classy.

Rerolled patch.

Lewis had changed the .dropbutton class to .js-dropbutton so I've fixed the tests to accommodate for that.

Missed two! Should be all set now...

Forgot to attach patch and interdiff for #59.

Just a quick review:

1. +++ b/core/misc/dropbutton/dropbutton.css
   @@ -1,164 +1,118 @@
   + * with a primary action. Secondary actions are hidden in a dropdown menu.
   + *
     */
   

   Don't need this extra line at the end.

2. +++ b/core/misc/dropbutton/dropbutton.css
   @@ -1,164 +1,118 @@
   - * When a dropbutton has only one option, it is simply a button.
   + * The button class is added when there is only one action. This should be moved
   + * to button.css
   

   This looks like an @todo?

Does all of this need to be done here or can some of it be broken off to keep the patch easier to review?

Interdiff doesn't work straight away so not uploading one.

Rmoved css file from core dropbutton library.
Removed leftover </li> from js theme function.
Simplified the JS, I don't like unwrap. It's more predictable now.
Added .dropbutton to all dropbuttons (not just the ones with a menu). This fixes problems with views ui.
Added the previously used .dropbutton-multiple on dropbuttons with menus (This class only adds the padding required).

might as well go with a data attribute right now. rolling patch and fixing tests.

got rid of js-dropbutton and used data-drupal-dropbutton as it should.

Things are still working on the frontend, let's see how much I broke tests.

Not sure this can be committed now. Markup and CSS is frozen after RC.

Local tests take too long, sorry for the noise while I fix remaining test failures.

Did some profiling, the new code is marginally faster.

As we are now Post Drupal 8 release, can the future steps for this issue be laid out please?
ie. Looking at the code in the latest patch - what can still be used, what needs to go and where do we move things to?

We have bugs being raised based on contrib modules having dropbuttons with single values having no styles. This issue will solve that with the solo value = button markup.
#2700521: Dropbutton with a single option misses its rounded corners

I found this after @lauriii pointed me at the parent issue, #1899236: Add new Splitbutton render element to eventually replace Dropbutton.

Over in #2893663: Dropbutton should report open/closed state to assistive technology I suggest we follow Menu Button pattern from the WAI-ARIA Authoring Practices.

I like the proposed markup here, it's much closer to the specimen code for the ARIA menu button pattern. Notably, the more-actions button is no longer nested inside the list which it controls, which was a bit weird currently.

I see there's a role="menu" on the list. That's part of the ARIA Menu Button pattern, but adding that role alone won't suffice. There's also a need to manage roles on the descendent LI and A elements, as well as some aria-* properties on the button. Is this issue just about establishing the markup? The ARIA pattern calls for managing aria-expanded, which is a dynamic property.

minor title tweak for clarity.

This should really be consolidated with the linked issue!

Here's my PoC-proposal for splitbuttons.

Instead of rewriting the existing Dropbutton, this patch provides a new element: Splitbutton. Now it allowed to contain "submit", "button" and "link" type children. It also tries to converts the dropbutton-legacy '#links' children (so if you want to test this, you only have to modify the existing dropbutton type from ['#type' => 'dropbutton'] to ['#type' => 'splitbutton']).

I also attached a patch for Paragraphs Classic widget that operates with the new splitbutton element.

The splitbutton.js tries to be as accessible as possible (see #3067697-24: Dropbutton breaks when text is split to multiple lines).

I haven't done any styling for Seven, Bartik or Claro (but I had to fix a nit for Claro's multiple value form).

@huzooka @laurii is new styling required here or just simplified markup without breaking the existing styling?

Interesting to see some movement here.

1. #92: If this is a new splitbutton element, what if anything, will happen to the existing dropbutton? I had considered closing #2893663: Dropbutton should report open/closed state to assistive technology in favour of this one, but it sounds like splitbutton is a major change in direction. The issue summary and title need updating.

   If this issue is leaving dropbutton alone, then I'll leave #2893663: Dropbutton should report open/closed state to assistive technology open. It's still a bug which ought to be addressed in dropbutton.

2. In #86 here, I proposed following the ARIA menu button pattern. Now I'm not so sure. I've read a bunch of articles about ARIA menu and/or menubutton pattern, which cause me to doubt whether it is appropriate in the situations where we currently use dropbutton. The issue is that when you have the menu role, the only permitted descendants are "menuitem". In other words, there is no way to distinguish a link from a button. Some discussion in the a11y community is tending towards just a simple disclosure button to reveal a list of links, especially if it's for navigation. So if the distinction between link/button matters, or if the use case is navigation, then we shouldn't use the menu and menuitem roles, or aria-haspopup.

   I'll dig out bookmarks, and read about this all again. Meanwhile, some of my thoughts were already noted in comments #10-13 of [#3892663-10].

   If we remove the menu and menuitem roles, I don't think it will mean a big rewrite of the patch here, because those are just static properties. It could also mean we simplify the keydown handler (though I really like the look of that part of the patch).

   Another possibility is to include an option, called use_aria_menu_role or something. If true, we layer more roles on. If false, it's just a list of links.

   The use of aria-controls and aria-expanded is the same either way.

3. Different topic: is the HTML markup (ignoring ARIA roles and properties) still as shown in the Proposed Resolution? A standalone link ("action one"), followed by a button, which reveals a list of links.

   I had the idea that the standalone "action one" could be optional. If absent, there would still be a button which reveals a list of links. Currently layout builder depends on contextual links module, but there was some concern about whether that was the best choice, and if layout builder might use a different button for it's menus. This button might provide a basis for that.

Re #100:

Since we cannot change dropbutton for BC reasons (imho), the way to move on is:

1. Create a new element with the required markup + functionality + (accessibility) features.
2. Replace core's dropbutton and operations usage with the new splitbutton element.
3. Deprecate dropbutton.

So:
#100.1: Please, don't close #2893663: Dropbutton should report open/closed state to assistive technology.
#100.2: I'd continue with the new option use_aria_menu_role. I'd set it to true by default, but if there is any button (or [input="submit"]) inside the dropdown, (or it's set to false), we don't add the roles.
#100.3: Yes, yes, I want to do that. My plan was that I'd make SplitButton able to display more than one 'primary' item (that are displayed next to each one, outside of the dropdown). But if splitbutton would be able to have no primary items, it would be able to replace the contextual menu as well (where I mean the collapse/expand button and the contextual links dropdown).

Let's have tons of failed test!

Only minimal CSS styles, no text rewrites, but the provided PoC addresses #100.2 and #100.3

I was able to replace the fake Views UI dropbutton mentioned in https://www.drupal.org/project/drupal/issues/1899236#summary-proposed-re... #1 with a Splitbutton.

#100.3 can be checked on Views UI's view edit form:

Some progress...

Umami Splitbutton styles still have to be checked.

Duplicate of #1899236: Add new Splitbutton render element to eventually replace Dropbutton