File field style update

Design isn't finished yet.

Design is ready for implementation.

Design isn't ready for implementation imho.

Issues I've found:

1. File widget description is missing from the design.
2. The table (especially the table heading cells) used on the widget design does not follow the existing implementation. Do we really want that to be different?
3. No example found on how to display the Display boolean (the checkbox, see the attached files).
4. No example for the file's Description input (see the attached files).
5. I think that it would be nice to display a required mark in the Details title if the file field is mandatory (mainly because we want to wrap even the single file widgets into a collapsible Details).

Question:

1. It seems that the inner spacing of the Details was changed since it was committed. Do we want to change the existing implementation?
2. The collapsible element (that looks like a Details) does not follows the existing implementation. Do we really want this?

Added a screenshot about the file widgets displayed by Seven theme (with multiple states; but without errors).

I will start with this issue by setting up the needed markup changes for the single-field cases.

Just agreed on that we will handle the details wrapper of the single items in a follow-up (it seems to be a really complex task).

Adding the updated design fixing this. Please use Figma to check spacing other specs, but here's an quick screenshot:

POC for wrapping single file and image widgets into a details.

Assigning myself because I was (and am) working on this parallel with other issues.

This is my current progress.

There are some issues with the design's current spacing and @lauriii noticed them :).

Questions:

1. Are we sure that this is fine? (on the screenshot: file widget summary is focused, and there isn't any space between the focus box and the first element in the content).

   

2. I guess we should think over the spacing and the placement of the dragged message.

   

#16.1 Good catch @lauriii & @huzooka! I just updated a part of the specs to follow the default fieldset styles. I'll update the styles later on.

#16.2 I would actually expect this result because the focus will sometimes appear surrounding an element instead of inside of it, and makes sense that it ends up over over something else. I'm more concerned with the right alignment/padding leaving a white space, but I think this is not the place to solve it. I'd say let's discuss it on another issue so we can unblock this one. :)

Attached a new patch that follows the old design. Hopefully, to address the new one from #17 we only have to remove some code :).

Tabledrag-related enhancements were added to our follow-up #3083051-2: Refactor tabledrag when core issues are resolved.

#20 contains every needed improvements for the image widgets. I removed them at @lauriii's request.

What happened?

1. Assuming that we need to keep target sizes everywhere and not only for buttons, Claro now globally depends on core/modernizr. I've refactored every small and extrasmall component variant and we use them only if the device is not touch-capable (this is exactly the same that we did for buttons).
2. I've improved out IE11/Edge support and fixed the size of the file upload input for them (it was a bit bigger because of the browse button
3. Renamed our existing 'small' form-element variant to extrasmall (it was a naming convention error)
4. Added extrasmall variant for Action link components.
5. Added two icon variant for the Action link components. These are used for the Show/Hide row weights toggle of draggable tables.
6. The summary of required file widget details gets a required mark.

Differences from the design:

1. Below 37.5em browser viewport with, the table of the multiple file widget reaches the border of their wrapper details. I think this is really useful for lower screen sizes.
2. The table head cells are 36px tall ((0.5rem vertical padding * 2) + 1.25rem line-height), and not 34px.
3. We missed the design for singe file widget with a value, please check whether my implementation matches the conception or not.

+++ b/claro.theme
@@ -262,6 +251,99 @@ function claro_element_info_alter(&$type) {
+    // We want to display the description of single-cardinality widgets similar
+    // to the multiple value widget, so render the description from the field
+    // config in the details, separated from the other descriptions (that are
+    // computed from the file and image upload validators and from the
+    // cardinality).
+    // Actually, the description of the managed file widgets is rendered, and
+    // all of these descriptions are merged, and the field-config description is
+    // replaced with the rendered description.
+    // Right now, the only way to get back the potential field config
+    // description is to render the file_upload_help with an empty
+    // '#description' key, and compare its string value of the original
+    // description.
+    // @todo refactor when https://www.drupal.org/node/3084899 is fixed.
+    // @see https://git.drupalcode.org/project/drupal/blob/ade7b950a1/core/modules/file/file.field.inc#L53
+    // @see https://git.drupalcode.org/project/drupal/blob/ade7b950a1/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php#L264
+    if (!empty($element['#description']) && !empty($element['#upload_validators'])) {
+      // For removing potential HTML comments.
+      $html_comment_pattern = '/<!--(.|\s)*?-->/';
+      $whole_description_string = (string) $element['#description'];
+      $upload_help_description_renderable = [
+        '#theme' => 'file_upload_help',
+        '#description' => '',
+        '#upload_validators' => $element['#upload_validators'],
+        '#cardinality' => $element['#cardinality'],
+      ];
+
+      $upload_help_description = trim(
+        preg_replace(
+          $html_comment_pattern,
+          '',
+          \Drupal::service('renderer')->renderPlain($upload_help_description_renderable)
+        )
+      );
+      $whole_description = trim(preg_replace($html_comment_pattern, '', $whole_description_string));
+
+      if (($start = strpos($whole_description, $upload_help_description)) !== FALSE) {
+        $element['#description'] = $upload_help_description;
+        $element['#theme_wrappers']['details']['#description'] = [
+          '#markup' => substr($whole_description, 0, $start),
+        ];
+      }
+    }

@lauriii and I agreed on that we will remove this ugly workaround and accept that empty single cardinality file widgets will have the merged description, and that it will be displayed on the bottom of the upload field.

We need follow-up for creating designs for #24 and we will try to update this based on that.

I haven't reviewed the code yet (will work on that next), but in the meanwhile I noticed that the ajax throbber doesn't look great:

Do we have support for the progress bar as well? Would someone be able to post screenshots on how that looks?

Really good work! I noticed, that the icon is not well aligned on RTL.

Addressing #29.

Found some minor RTL-related bugs and fixed them.

1. +++ b/claro.libraries.yml
   @@ -49,6 +51,10 @@ global-styling:
   +      # Since we have small and extra small form elements, action links, buttons
   +      # and we want to keep our touch target sizes big enought, we need
   +      # Modernizr.
   

   This is missing some crucial parts to be able to understand why we actually depend on modernizr here. We should mention that we adjust some small controls to be bigger if the user is using a touch device and that we are using modernizr for recognizing that.

2. +++ b/claro.libraries.yml
   @@ -49,6 +51,10 @@ global-styling:
   +    - core/modernizr
   

   Why are we loading this always? Couldn't we load it only when one of these elements are present on the page?

3. +++ b/claro.theme
   @@ -262,6 +251,56 @@ function claro_element_info_alter(&$type) {
   +      // Change widget title.
   +      // @see https://git.drupalcode.org/project/drupal/blob/ade7b950a1/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php#L192
   +      $element['#title'] = t('Add a new file');
   

   Why do we have to duplicate the value from there?

4. +++ b/claro.theme
   @@ -262,6 +251,56 @@ function claro_element_info_alter(&$type) {
   +      $element['#title_display'] = 'invisible';
   +      $element['#description'] = NULL;
   

   Let's explain here why we have to do this.

5. +++ b/claro.theme
   @@ -1126,3 +1191,82 @@ function claro_preprocess_links__action_links(&$variables) {
   +              if (
   +                isset($last_cell_item['#attributes']['class']) &&
   +                is_array($last_cell_item['#attributes']['class']) &&
   +                in_array('remove-button', $last_cell_item['#attributes']['class'])
   +              ) {
   

   Nit: According to Drupal coding standards, control structure conditions shouldn't be wrapped to multiple lines.

6. +++ b/claro.theme
   @@ -1126,3 +1191,82 @@ function claro_preprocess_links__action_links(&$variables) {
   +                in_array('remove-button', $last_cell_item['#attributes']['class'])
   

   In the future, maybe we could use this: #2511548: Add a "context" array variable to all theme hooks and "#context" array property to all elements to provide optional contextual data here 🤔

7. +++ b/css/src/base/variables.css
   @@ -92,7 +92,15 @@
   +  --input-border-size: 1px; /* (1/16)em ~ 1px */
   

   👍

8. +++ b/css/src/components/details.css
   @@ -71,7 +71,6 @@
   -  text-transform: capitalize;
   

   What is this change needed for?

9. +++ b/css/src/components/form--managed-file.css
   @@ -0,0 +1,79 @@
   +.form-managed-file .file {
   

   Should we add a new class form-managed-file__file?

10. +++ b/css/src/components/form--managed-file.css
    @@ -0,0 +1,79 @@
    +.table-file-multiple-widget .form-item::before {
    

    Can we document here why we can't add the clearfix class to these elements?

11. +++ b/templates/details.html.twig
    @@ -58,6 +59,9 @@
    +        <span class="required-mark"></span>
    

    How is this going to be read by screenreaders?

12. It doesn't seem that the design works well in all scenarios on mobile:
    

    Maybe this is something that will be solved by #3068696: Tables overflow on mobile.

13. +++ b/claro.theme
    @@ -262,6 +251,56 @@ function claro_element_info_alter(&$type) {
    +  // We need to display even single-cardinality widgets wrapped in a details.
    

    We could change this to just Wrap single-cardinality widgets with a details element.

14. +++ b/claro.theme
    @@ -403,6 +442,27 @@ function claro_preprocess_details(&$variables) {
    +    // Seven and even Stark do have this issue.
    

    🤯

15. +++ b/claro.theme
    @@ -1126,3 +1191,82 @@ function claro_preprocess_links__action_links(&$variables) {
    +    $variables['table']['#attributes']['class'][] = 'table-file-multiple-widget';
    
    +++ b/css/src/components/table--file-multiple-widget.css
    @@ -0,0 +1,107 @@
    +.table-file-multiple-widget td {
    

    How about draggable-table--file-multiple-widget?

16. +++ b/css/src/components/table--file-multiple-widget.css
    @@ -0,0 +1,107 @@
    +  white-space: nowrap; /* Don't let ajax-progress be broken into a new line. */
    

    👍

Re #35:

1. Well, not just for some. We do that for all of them. Comment rephrased.

2. I think I cannot guarantee full support. In theory, this would be possible (with a significant refactor of Claro's libraries + claro.theme), but for example I cannot do this for every link, since LinkGenerator only calls the link alter hooks that are implemented by modules. I can only add it for those elements that can be preprocessed or pre-parsed by a theme.

3. This is not a duplicate. This copies the behavior of the multiple file widget for the single cardinality widget. Comment added.

4. Done.

5. Fixed.

8. I just noticed this, and I'm sure we don't need this. We neither need this to be re-set for simpletest summaries. Nor re-setting it for Claro's Details. I bet that the design never defined that the details summary has to be capitalized.

9. 9. No, I don't think so. File is a standalone component (both in terms of CSS and the template system).

10. We are able to add that, but it won't help. This is a ::before pseudo, and not an ::after!

11. This will be read by screen readers the same way as the required mark of a form-item label or a fieldset label: won't be read at all, because there isn't anything to read. This is only a visual help.

12. This can be worse: make row weights be shown!

13. Done.

14. What? :)

15. Well, nothing guarantees that this will be a draggable table. Let's keep this.

16. :)

The danger variant of the new action link icons is broken.

Fixed #40 and besides that, I applied the optimized SVGs on action links.

This behavior on the single file upload is still a bit strange. Before uploading a file, the description is shown below the upload field:

However, after uploading a file, it's above the uploaded file:

Re #43:
Roger! You don't like that. But what to do then? Should we put back #24? I also hate that, but that is the only way to make this consistent.

Please, define your acceptance criteria and I'll ship the solution that matches them.

From slack pm with @lauriii:

lauriii 3:01 PM
I think we should place it under the uploaded image and the additional fields associated to the file

so it’s the same as Seven

huzooka 3:04 PM
Do you want me to do that?

lauriii 3:04 PM
that would be great

Re-rolled #41.

Let's see whether this applies cleanly or not.

This patch addresses #45.

#3085878: Test file widget designs with progress bar as a follow-up.

Is it possible that the patch doesn't apply anymore?

Here's a reroll of #50.

Thanks for the work here! I compared this with the designs and there are still a few things to do:

- Multiple field:
1. The remove button should be top aligned.
2. There should be top&bottom padding to the name.
3. The button should use the smaller size: XS.

- Single field:
4. The remove button should be right aligned.
5. There should be less space between the description field and the file name.

Ah, I forgot the screenshots:

Thanks for the work! I've found a few small things:
1. The left and right space should be the same when there's only 1 file.
2. The space between the icon and the file name should be bigger.
3. The file icon should be vertically aligned with the name.

Really good work @huzooka :) I recognized a few things:

1. To me, this exact issue described in the SO question happens to me with the upload input.
   Basically the horizontal alignment of the initial text and the input itself is off.
   There is also an answer provided in the question.
   https://stackoverflow.com/questions/27616054/file-upload-styling-in-chrome

   Firefox (correct)
   

   Chrome (wrong)
   

2. The message renders under "Show row weight" - this isn't that bad but if it could render above the table and the button "Show row weight" it would be nicer imho.

   Now
   

   Proposed solution
   

There was a small error in the rebased patch of #54.

Needs reroll because #3023310: Vertical Tabs style update just landed.

Rebase of #63.

This patch addresses #58.2, #58.3 and #59.

Re #58.1:
That's not a margin. I applied a max-width on the managed file widget. The motivation behind this decision is that I don't want to let the remove button be rendered too far from the other file-related UI elements.

Re. #58.1 I'm not convinced with this solution, but I see the problem you're pointing to. It'll need a design solution, sol let's not block this issue with this. :)

I've found just a very small thing: when the file name is really long it jumps to another line and it should really remain on the same:

Re #68:
Sadly, we cannot do this when we're in the same table-cell where the drag-handle is.

We should be able to do it as discussed on Slack, but it can be a follow-up. So marking this as RTBC.

Follow-up filled: #3086255: Align long file names for File field

@huzooka Can we generate screenshots for this, please? That would be really helpful to review this in detail!

My Safari freezes right at the beginning of the Nightwatch test script.
I'll upgrade my system (hoping that it will help).

Screenshots attached (except Safari on OS X).

I tested MacOS Safari manually. I also made some minor coding standard improvements, as well as improved wording on some documentation. Interdiff attached.

Thank you everyone! Pushed and committed!