Problem/Motivation

This could maybe be turned into a larger issue, but this is a start and I think at least this case can be handled with a workaround. Not really sure on the right component, that can change.

Let's just say you're using the Seven admin theme. The progress bar for batch API is rendered via Classy's progress-bar.html.twig which attaches the classy/progress library to style the progress bar appropriately.

However when you have a managed file field configured to use the progress bar instead of throbber, the markup for that progress bar comes from /core/misc/progress.js and doesn't include the classy/progress library.

Note that to test this currently you need the patch from #2587755: AJAX error when using progress bar on file field widget.

Steps to Reproduce

Setup:

  1. Apply the patch from #2587755: AJAX error when using progress bar on file field widget
  2. Increase upload_max_filesize and post_max_size in your PHP config to something big like 1G
  3. Install Pecl progress or APC and add apc.rfc1867 = 1 to your PHP config (status report will tell you whether you have it set up correctly or not)

Testing:

  1. Clean install of Drupal 8, standard profile
  2. Add a file field to either article or basic page content type
  3. On the manage form display of the content type: change the widget's progress indicator from throbber to bar with progress meter
  4. Make or acquire a big file, on OS X I used mkfile 900m big.txt - other options here
  5. Test uploading that large file on the node add form, it should hopefully take long enough so you can see the progress meter :)

Proposed resolution

Figure out a way to include the library as needed. One idea:

In a Classy preprocess function for the file-managed-file.html.twig, attach the library if the form widget is configured to use the progress bar rather than the throbber.

libraries-extend to the rescue!

Before:

After:

Remaining tasks

  • Patch
  • Tests
  • Review

User interface changes

The progress bar will look pretty when uploading files.

API changes

TBD, should be none.

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cottser created an issue. See original summary.

droplet’s picture

Wim Leers’s picture

Component: javascript » Classy theme
Status: Active » Needs review
FileSize
419 bytes

Figure out a way to include the library as needed.

Simpler: use libraries-extend. This is exactly what that was intended for.

Wim Leers’s picture

Related issue added for libraries-extend, CR at https://www.drupal.org/node/2497313.

star-szr’s picture

Issue summary: View changes
Status: Needs review » Needs work

@Wim Leers oh that would be great, big fan of libraries stuff as you may know :)

In manually testing it didn't seem to fix it though. Added steps to reproduce to the issue summary, I wish they were shorter but haven't found an easier way yet.

star-szr’s picture

Issue summary: View changes
Wim Leers’s picture

Status: Needs work » Needs review

I turned CSS aggregation off.

With patch, this is present, without it is not:

@import url("/core/themes/classy/css/components/progress.css?o22dwx");

Perhaps you forgot to clear caches?

star-szr’s picture

Hmm I did clear caches several times trying to get it to work, will re-check this morning though. Thanks!

star-szr’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
29.31 KB
31.45 KB

Well I was doing something wrong last night, not sure what. Adding before and after screenshots to the issue summary.

Since the fix is in Classy we probably can't/won't add test coverage for this. This seems a bit iffy for 8.0.x if someone is loading their own library to compensate for this in a Classy subtheme, so moving to 8.1.x.

swentel’s picture

Version: 8.1.x-dev » 8.0.x-dev

Hmmm the progress bar is broken on so many levels in 8.x at this point because of #2587755: AJAX error when using progress bar on file field widget and #2662932: Fix file upload progress bar, so I think this can go in 8.0.x, because well, nobody can even use it ..

Feel free to correct me of course :)

star-szr’s picture

Thanks @swentel, that's not a bad point but I'm also concerned that progress.js could be used for other things too so still seems risky to me: I don't want to assume the core file module is the only one using this. I'll leave the version as 8.0.x for now but I still think this should only be committed to 8.1.x.

Also at this time there isn't much of a gap in the calendar between 8.0.4 (or 8.0.5) and 8.1.0 anyway, so it won't be a long wait :)

swentel’s picture

Heads up, #2662932: Fix file upload progress bar is now working too - JS reviews needed there :)

davidhernandez’s picture

I think I agree with Cottser. It is essentially a bug fix, but this not adding an insignificant amount of CSS. Probably better to be safe and release it with 8.1.

  • catch committed 285b971 on 8.1.x
    Issue #2662778 by Wim Leers: Ajax progress is rendered via JavaScript...
catch’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Fixed

I don't have a strong opinion, but given that just committing to 8.1.x for now, we can cherry-pick back later if we need to.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.