Problem/Motivation

Original statement:

Need to build out the spinning circle throbber for use in autocomplete and ajax loading events based on the Proposal: A Style Guide for Seven.

Motivation is discussed in more detail in the style guide.

Proposed resolution

(To confirm: does this issue cover approving the new spinner into core? Or is this issue just about coming to some sort of agreement regarding the style guide element[s]?)

Replace the existing spinner with a preferred SVG version, falling back to a GIF for older browsers.

Testing instructions

The spinner is used on taxonomy fields (entity reference, taxonomy term), so from a clean install you can just add an Article node and check the tags field – see #9 for instructions on what CSS to change to keep it running continuously, and how to check the fallback animated GIF.

The spinner is also used is Place Blocks. From a clean Drupal install, enable the Place Blocks experimental core module. This will add a Place Block option (with a lego brick item) to the right of the toolbar. Click this and you'll get a screen where every region has a plus sign. The spinner is displayed when the plus icon is clicked.

Remaining tasks

1. Write/propose patch. (done: #9)
2. Trial patch. (done: #14)
3. Identify where the spinner is not being applied. (#17)
4. Re-roll/add to patch, in order to cover all instances where spinner is required.
5. RTBC patch.

User interface changes

Superficial, in line with the style guide.

API changes

n/a although patch as stands includes the Modernizr library in its assets.

Data model changes

None.

Comments

AaronChristian created an issue. See original summary.

AaronChristian’s picture

FileSize
506 bytes

Attached are the throbber elements to be used. SVG for modern browsers and a GIF for fallback support (i will upload later). I can assist with writing some HTML5 animations if we want to go that route as well. Fallback support can be determined on body classes if SVG's are supported. Both should meet accessibility standards as a benchmark.

AaronChristian’s picture

Status: Active » Needs review
AaronChristian’s picture

FileSize
170.89 KB

Attached is the transparent gif animation.

Throbber

AaronChristian’s picture

Gábor Hojtsy’s picture

This looks amazing to me personally :)

AaronChristian’s picture

Thanks @Gabor, I'll cleanup the fallback GIF as I originally started with the SVG (and converted to gif from there). As Angie and Andrew noted it looks like the tail drags a bit just due to some extra frames that are in the animated GIF.

AaronChristian’s picture

FileSize
87.56 KB

AaronChristian’s picture

Adding patch for replacement of autocomplete throbber & ajax throbber (small & fullscreen).

Notes:

It can be hard to get that throbber going if you have no content/tags on your site, since this throbber "grows" and has more of an overlay effect rather than a hard replacement of the light grey to blue spinning clock you can fake it by adding the class "autocomplete-loading" to the "autocomplete-enabled" div.

<div class="autocomplete-enabled autocomplete-loading">

Also if you want to see what the fallback looks like just replace the .spinner div with this;

<div class="spinner">
  <div class="fallback">&nbsp;</div>
</div>
  • The throbbers are large so they can scale cleanly.
  • I updated modernizer to include the SVG library.
  • This uses pure CSS3 animations with plain SVG's (ie. doesn't use SVG SMIL animations anymore) with fallback GIF support for older browsers.
  • I've only added the SVG's to the autocomplete widget, however I had to fix up the normal ajax throbbers as well as they all relied on the same images.

Future Plans:

  • Replace all AJAX progress throbbers with the same SVG/GIF treatment.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Version: 8.3.x-dev » 8.2.x-dev

Tried this on a quick test site and it works fabulous on the tags autocomplete widget. Look like this needs to be applied to AJAX too and then get some more topic-expert reviewers :)

Gábor Hojtsy’s picture

Version: 8.2.x-dev » 8.3.x-dev

Version move was unintentional.

yoroy’s picture

Compare with #1974928: Update throbber icon and decide on which design and which implementation to move forward with.

Manjit.Singh’s picture

FileSize
1.27 MB

It looks cool !! @AaronChristian
But In some of cases i have observe that it spin so quick that we can't figure out, Is it loading or not.

AaronChristian’s picture

Great, thanks @Manjit.Singh, yes i have also observed this, I could add a delay on the fade out, or make a full rotation before stopping. This is noted in an earlier comment because of the fact that the spinner "grows" and the lack of terms within a base drupal install.

I can make this addition but would love to find out if this is the throbber we plan on pursuing or not.

AaronChristian’s picture

Gábor Hojtsy’s picture

So #9 says "Replace all AJAX progress throbbers with the same SVG/GIF treatment." that to me means that work is not yet done. Also it sounds like having two throbbers in core would be bad, so I think that would be a requirement to fix in this issue?

jp.stacey’s picture

Issue summary: View changes
Issue tags: +Dublin2016

I think it would be good to get clarity on a couple of points:

1. the ultimate goal of this issue i.e. what "done" looks like
2. what further work is required to bring it to that state

This sounds like a lot but I think really it's just process! But with this in mind I've reworded the issue description using the standard template: please feel free to edit if you don't think it matches the situation.

@GáborHojtsy could you also clarify (approximately?) what extra work you think needs doing? Is it that there are spinners that are not yet fixed by this patch? Can you suggest where?

jp.stacey’s picture

Issue summary: View changes
vulcanr’s picture

Just tested this patch, and with a simple test creating a new Article page, trying to change the author, the throbber/loader doesn't appear.

I agree with jp.stacey. I think we are missing the point here, how it should look like?

lauriii’s picture

Component: ajax system » Seven theme
Status: Needs review » Needs work

On the UI perspective things look good. I tested multiple different use cases including Node edit form, Field UI and Block UI. Good work on that!

However, I'm afraid that we need to limit all the changes to Seven theme. That's why I'm moving this to the Seven issue queue.

I'm not sure if we can modify the throbber at all in Stable theme because it might affect themes people have built. Anyway, we don't have to deal with that here.

I'm also quite confident that we can drop the fallback gif throbber since svg images seem to be supported pretty well: http://caniuse.com/#feat=svg

#13: Seven styleguide includes design for throbbers which I believe we should here.

Gábor Hojtsy’s picture

@jp.stacey, @vulcanr: it could very well be that your setup being local was/is too fast to respond and therefore no throbber seen, see @lauriii and I seen it work :)

vulcanr’s picture

#22 +1

lauriii’s picture

Title: Build Spinner (ajax, replaces “throbber”) » Update throbber icon in Seven theme
juhog’s picture

Assigned: AaronChristian » juhog
Bojhan’s picture

Issue tags: -Needs usability review
tkoleary’s picture

Issue tags: -spinning circle, -Dublin2016
Manjit.Singh’s picture

Assigned: juhog » Unassigned
tkoleary’s picture

+++ b/core/misc/autocomplete.js
@@ -213,8 +226,28 @@
+            '<svg viewBox="0 0 100 100">' +
+            '<circle stroke="#d9d9d9" stroke-width="4" fill="none" stroke-linecap="round" stroke-dashoffset="0" cx="50" cy="50" r="20" />' +
+            '</svg>' +
+            '<svg class="rotater" viewBox="0 0 100 100">' +
+            '<circle stroke="#009eff" stroke-width="4" fill="none" stroke-linecap="round" stroke-dasharray="89, 200" stroke-dashoffset="0" cx="50" cy="50" r="20" />' +
+            '</svg>'

Shouldn't we reference this svg file rather than inlining it? Then it will be more easily used elsewhere if needed.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
111.47 KB

Rerolled since the patch did not apply anymore to autocomplete.js. Hope this reroll is good (vs. binaries, etc).

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2017
tikaszvince’s picture

@tkoleary We cannot apply CSS formatting on SVG image included as an image (with img tag) or as background image. With CSS we can control the size, color, etc. of SVG image parts if its placed as inline element.

John Cook’s picture

Status: Needs review » Needs work
FileSize
26.41 KB

Tested Gábor's re-roll against 8.4.x.

The throbber when testing in the tags auto complete widget, the throbber is displayed at full size and not resized to fit the widget.

AaronChristian’s picture

Hey guys just checking back in on this after being away on holidays for a few weeks. Does anyone need me to pick this back up? Sounds like we're getting close, would love to get this wrapped up and moved into core soon!

Gábor Hojtsy’s picture

@AaronChristian: yes please, I can help push this forward as much as possible but not do the actual making it work :)

xmacinfo’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
28.91 KB

Marking RTBC! Awesome implementation!

Tested Gábor patch #31 on Drupal 8.4.0-dev (latest pull). See screen capture attached.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Thanks everyone for your work on this.

No new patch was added between #34 and #37, so how did #34 get fixed?

Setting back to NR to confirm what the difference is. We can't commit this if it looks like #34 for some users, unfortunately. :)

Also, be sure to clear your browser caches. It might help to document the exact steps you use from e.g. "Install the Standard profile" to the screenshot to help figure out why we are getting different results.

xmacinfo’s picture

When I tested the patch #37 I used a fresh 8.4.x-dev cloned from git.

I tested only on a single browser. Maybe we need to have this patch confirmed in another browser, in addition to Firefox.

wturrell’s picture

Best thing is if we knew which browser + OS John Cook was using in #34, so we could double-check.

I've successfully tested the SVG and the fallback GIF (by manually editing classes as per instructions in #9), against the latest commit to 8.4.x, on:

- Opera 43.0.2442.806
- Firefox 51.0.1
- Chrome 56.0.2924.87
- Chrome Canary 58.0.3010.0
- Safari 10.0.3 (11602.4.8.0.1)

all of those Mac OS X El Capitan 10.11.6 desktop non-retina.

Looks very nice. Can someone check it's OK on some mobile devices?

xmacinfo’s picture

@wrturrel Based on your tests, you should put this issue back to RTBC. :-)

By the way, I tested this on Retina Mac Firefox 52.0b5.

vulcanr’s picture

Just tested this aswell and working fine. Retina Mac Firefox 51.0.1

John Cook’s picture

Status: Needs review » Reviewed & tested by the community

I've just re-tested Gábor's patch and it's working now.

Please disregard comment #34, I'm putting it down to "Tester Error".

Also, because of the extensive testing since then, I'm marking as RTBC.

Sorry everyone - I'm an idiot. :)

John Cook’s picture

P.S. Here it is working.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

We need to make sure that the new throbber is compatible with what's been done on #2793207: Fix off-set and goofy white jagged background of AJAX throbber when clicking Place Block - can some post a screenshot of the new throbber on the blue bartik header?

serg2’s picture

FileSize
58.44 KB

8.4.x on simply test me with place block enabled. With and without this patch:

Place blocks test

alexpott’s picture

Status: Needs review » Needs work

@serg2 thanks! So the screenshots prove this needs work.

serg2’s picture

Sorry for the lack of a proper patch:

core/modules/block_place/css/block-place.css

 .block-place-region .ajax-progress-throbber .throbber {
   display: block;
   padding: 0;
   height: 26px;
   width: 26px;
-  background-position: center 6px;
+ background-position: center center;
   background-color: #fff;
   border: 1px solid #ddd;
   border-radius: 100px;
   box-sizing: border-box;
xmacinfo’s picture

@serg2 Can you reroll the patch? :)

serg2’s picture

That should be it (but first core patch so please check!)

serg2’s picture

Status: Needs work » Needs review
FileSize
44.08 KB

A screenshot in chrome for #50 patch. Marking as Needs Review.

Screenshots

wturrell’s picture

Is there a way to stop it immediately disappearing, for testing purposes? (Seems to look ok.)

John Cook’s picture

@wrurrell, have a look at comment #9. It suggests setting "autocomplete-loading" as a class on the container element.

tkoleary’s picture

serg2: I played around with this a bit and here are some thoughts:

  1. +++ b/core/assets/vendor/modernizr/modernizr.min.js
    @@ -1,3 +1,3 @@
    \ No newline at end of file
    
    +++ b/core/misc/autocomplete.js
    @@ -218,8 +231,28 @@
    +            '<circle stroke="#d9d9d9" stroke-width="4" fill="none" stroke-linecap="round" stroke-dashoffset="0" cx="50" cy="50" r="20" />' +
    

    The stroke width is too narrow here for instances where the input is smaller like the autocomplete. I'd push it up to at least 10.

  2. +++ b/core/misc/autocomplete.js
    @@ -218,8 +231,28 @@
    +            '<circle stroke="#009eff" stroke-width="4" fill="none" stroke-linecap="round" stroke-dasharray="89, 200" stroke-dashoffset="0" cx="50" cy="50" r="20" />' +
    

    Same applies here.

  3. +++ b/core/themes/stable/css/system/components/ajax-progress.module.css
    @@ -5,15 +5,16 @@
    +  background: url(../../../images/core/throbber-active.gif) transparent center center no-repeat;
    

    If I understand correctly Moernizr gives us inline SVG, but for the fallback we can still have an SVG referenced as a file, which covers all browsers and retina displays.

  4. +++ b/core/themes/stable/css/system/components/ajax-progress.module.css
    @@ -28,22 +29,22 @@ tr .ajax-progress-throbber .throbber {
       z-index: 1000;
    

    Whoa, 1000? That's going to put it above everything else that has a z-index. Is that what we want?

  5. +++ b/core/themes/stable/css/system/components/ajax-progress.module.css
    @@ -28,22 +29,22 @@ tr .ajax-progress-throbber .throbber {
    +  background: transparent url(../../../images/core/throbber-active.gif) center center no-repeat;
    

    Same comment as above.

  6. +++ b/core/themes/stable/css/system/components/autocomplete-loading.module.css
    @@ -5,18 +5,193 @@
    +  animation-duration: 1.8s;
    +  -webkit-animation-duration: 1.8s;
    +  -moz-animation-duration: 1.8s;
    

    This went by so fast I didnt even see it.

  7. +++ b/core/themes/stable/css/system/components/autocomplete-loading.module.css
    @@ -5,18 +5,193 @@
    +  animation: dash 1.7s ease-in-out infinite 0s, spinner-color 6s ease-in-out infinite -0.75s;
    +  -webkit-animation: dash 1.7s ease-in-out infinite 0s, spinner-color 6s ease-in-out infinite -0.75s;
    +  -moz-animation: dash 1.7s ease-in-out infinite 0s, spinner-color 6s ease-in-out infinite -0.75s;
    

    I changed this to just ease-out in the browser and it worked better. The slow expansion of the dash means it's gone before it has a chance to appear.

serg2’s picture

re #54 I have made the changes suggested for points 1,2,4,6,7.

1. Done
2. Done
3. Use SVG file rather than GIF. Not done yet
4. Doing a quick scan of different z-indexes on the same pages the throbber is present I found values of 500-1250 so left it as 1000.
5. Use SVG file rather than GIF. Not done yet
6. Increase from 1.8s to 2.6s. The dash animation may need to be changed to fit in with this. Will visually test against this patch
7. Done

This will need visual browser testing & screenshots again.

tkoleary’s picture

@Serg

Looks much better. Visually tested in Chrome/Mac/Sierra.

frederickjh’s picture

I did a screen capture at 15 fps then dumped the frames into a animated gif. This is a bit slower than the original. I figure there should be ~67ms between frames. This gif is with 100ms between frames.

serg2’s picture

FileSize
40.62 KB

@frederickjh: is that with the patch applied?

It should appear as:

Patch 55 gif

@tkoleary: I am not sure about the increase in line thickness made in #55. This moves us too far away from the style guide. The patch was RTBC back at #43 and my intention was just to re-roll after #2793207: Fix off-set and goofy white jagged background of AJAX throbber when clicking Place Block had been committed.

I will upload a new patch just with the essential changes and some screenshots which can hopefully then be committed, with other changes as follow ups.

frederickjh’s picture

@serg2 Yes that is with 2775725-55.patch applied on 8.4.x. It did however complain about whitespace.

$ curl https://www.drupal.org/files/issues/2775725-55.patch | git apply -v 
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  109k  100  109k    0     0   350k      0 --:--:-- --:--:-- --:--:--  440k
<stdin>:415: trailing whitespace.
2�����ެ�����B�D�ls�:&���:�,?-�����X}5	Zw-B�^s-6�8�@v�k��U��v�m�=7�q����v/�s�X�}B�
                                                                                   {L��A����>�ލN���R�sݕ�M3�$0.��L�Vx�à�my˯F��Ȝgn6ۧw�xΥ�%�P#>0�~<;�=�����>��x�nu�D�/����k�0��J/��9k?=��[_,��z��p����4��ضN쟊j�����@���i����A��>�堀��h.!H�*�w���������
<stdin>:612: trailing whitespace.
w�w?����7@9�wy�-@`�z�ܠ��
                            ؠ
<stdin>:711: trailing whitespace.
�+���ѹu'
        E�/�䐔-�0�U�BēP
<stdin>:848: trailing whitespace.
��ǔ�3�RV��v;������&e�Q^��P`��>)��Hw@�<�v���-���+!���cI��?�
                                                                 "��Kɧ���1��u0����,L��?���SI6Z�#@�� ܡ�ZC)L���A	
<stdin>:883: trailing whitespace.
z�ޝ���˻�����ӈ��|���C E��
Checking patch core/assets/vendor/modernizr/modernizr.min.js...
Checking patch core/misc/autocomplete.js...
Checking patch core/modules/block_place/css/block-place.css...
Checking patch core/themes/seven/css/components/form.css...
Checking patch core/themes/stable/css/system/components/ajax-progress.module.css...
Checking patch core/themes/stable/css/system/components/autocomplete-loading.module.css...
Checking patch core/themes/stable/images/core/throbber-active.gif...
Checking patch core/themes/stable/images/core/throbber-inactive.png...
Applied patch core/assets/vendor/modernizr/modernizr.min.js cleanly.
Applied patch core/misc/autocomplete.js cleanly.
Applied patch core/modules/block_place/css/block-place.css cleanly.
Applied patch core/themes/seven/css/components/form.css cleanly.
Applied patch core/themes/stable/css/system/components/ajax-progress.module.css cleanly.
Applied patch core/themes/stable/css/system/components/autocomplete-loading.module.css cleanly.
Applied patch core/themes/stable/images/core/throbber-active.gif cleanly.
Applied patch core/themes/stable/images/core/throbber-inactive.png cleanly.
warning: squelched 2 whitespace errors
warning: 7 lines add whitespace errors.

But, I am not sure that should change anything.

frederickjh’s picture

Regarding the thickness, It could be that I had the screen zoomed in when I did the screen capture. I am on Ubuntu 14.04 with high dpi screen. There are some issues there so I many times have to zoom in so things are the normal size. The zoom goes in steps and so it may not be the exact same size as it would be on other screens.

serg2’s picture

Here is a minimal re-roll from #31. The only additional changes are the animation adjustments suggested by tkoleary in #54.

Browser testing and screenshots to follow

serg2’s picture

FileSize
36.75 KB

I tested in multiple browsers and it works fine, as before.

Here is a screenshot from Chrome:

Chrome

@frederickjh I am sure that must have been a browser cache issue. Would you mind clearing caches etc and then checking?

AaronChristian’s picture

Yup small warning on the training spaces but this has come together quite nicely. Good work everyone.

Are we still needing this one converted into an SVG?

frederickjh’s picture

@serg2 I would try clearing the caches but I have already removed that development environment. Sorry it did not come out correctly. I thought I had run drush cr before recording the screencast.

ressa’s picture

FileSize
2.38 KB

@frederickjh: You can test Drupal 8.4.x together with the patch at simplytest.me:
https://simplytest.me/project/drupal/8.4.x?patch[]=https://www.drupal.or...
To make the throbber run continuously, use the "autocomplete-loading"-tip in #9.

The patch seems to work fine (Ubuntu 16.04 64-bit, Firefox 51.0.1):
Autocomplete loading

xmacinfo’s picture

Status: Needs review » Reviewed & tested by the community

@ressa: you can mark RTBC. Thanks for the tip with Simplytest.me.

I used Simplytest.me and tested the following browsers successfully.

  • Firefox 52.0b7 macOS
  • Safari 10.0.3 macOS
  • Chrome 58.0.3013.3 dev macOS

I like the stroke width and the speed and animation.

I suggest to commit this and later on deal with the fallback icon.

wturrell’s picture

Issue summary: View changes

Added basic testing instructions to Issue Summary (for taxonomy terms and inline block-placement).
Still unsure if there's a way to continuously activate the animation for the latter (I used classes in #9 when doing previous tests.)

jwilson3’s picture

+++ b/core/misc/autocomplete.js
@@ -218,8 +231,28 @@
+      if(Modernizr.svg) {
+        spinner.append(
+            '<svg viewBox="0 0 100 100">' +
+            '<circle stroke="#d9d9d9" stroke-width="4" fill="none" stroke-linecap="round" stroke-dashoffset="0" cx="50" cy="50" r="20" />' +
+            '</svg>' +
+            '<svg class="rotater" viewBox="0 0 100 100">' +
+            '<circle stroke="#009eff" stroke-width="4" fill="none" stroke-linecap="round" stroke-dasharray="89, 200" stroke-dashoffset="0" cx="50" cy="50" r="20" />' +
+            '</svg>'
+        );
+      } else {
+        spinner.append('<div class="fallback">&nbsp;</div>');
+      }

I don't want to stop progress on this vast improvement, but an inline SVG buried inside javascript with no template seems really hard for a front-end dev to override. Possibly even more difficult than what we currently have. Is there a javascript variable or template system we could leverage here to facilitate an override? Is there an argument for not using an external SVG file added added as a background image via css?

tkoleary’s picture

@jwilson3

It's true that we could use pure css like: http://codepen.io/filipekiss/pen/yJxFo

But i'm not convinced that this complex css is any better as a developer experience than the relatively simple variables that the svg provides.

heddn’s picture

re #69: +1 to your suggestion. We had to replace the circle icon with a goofy running dog on a client site. It meant we had to implement our own ajax callback and the work to swap out the simple icon was pretty extensive, for what seemed like a fairly simple theme thing.

wturrell’s picture

Status: Reviewed & tested by the community » Needs review

We need to get someone to check this in Windows browsers before we RTBC, imho. Note you only need to do IE11 / Edge, as when 8.3.0 comes out the current plan is to formally drop support for IE9 and 10.

xmacinfo’s picture

@jwilson3 see comments in #29 and #33 as to why inline SVG was choosen.

I have Windows with IE9 and IE10, but if we drop support with 8.3.0, I will not test on Windows as I do not have Windows 10.

ressa’s picture

FileSize
8.92 KB

I just tried it on Browserstack and it looks fine, but probably best to also have a genuine Windows machine test it:
os=Windows, os_version=8.1, browser=IE, browser_version=11.0
Autocomplete Windows IE11

tkoleary’s picture

@xmacinfo Yes that's true but we were talking about referencing the svg rather than putting it in the code there. We have not as yet looked at a pure css (no image file) solution like the example in the link in #70.

Presumably the advantage there is both simpler dev experience and no need for fallback or polyfill for other browsers like IE.

xmacinfo’s picture

@tkoleary Agreed. CSS would be great. But if we want to implement the throbber in CSS we may have to change this issue to “Needs work”. We would also need to know more precisely which browsers support that type of CSS animated icons.

vulcanr’s picture

I agree with @tkoleary at #70 & #75, I don't think that pure CSS will give us a better experience, and it's a lot of code to build a simple animation. I would rather go as well with a SVG.

AaronChristian’s picture

It would be great to roll this in as the MVP version, and if someone would like to enhance the experience create another issue. I think this is a big step in the right direction, but it has seemed to have stalled due to quite a bit of discussion on best practices (but no real consensus).

The solution that is implemented is very close to the spinning throbber that you see when you update google chrome. It's using inline SVG's with some animations.

What are everyone elses thoughts?

Chrome

xmacinfo’s picture

I would also go for inline SVG like developed here (current patch).

CSS icon is not a a good option.

All other icons in Drupal 8 are now using SVG. We should stay inline with the other icons and use, accordingly, SVG for the throbber.

AaronChristian’s picture

Are we good to set this to RTBC for the time being?

jwilson3’s picture

IMHO, no, this is not RTBC. My point about it being too difficult to override an inline SVG buried inside autocomplete.js was echoed by @heddn and has not been addressed. We had a JS template system in Drupal 7; I don't know if this has gone away in d8, but if it has not, this is a prime candidate to use it. Even if there is no proper "template system" per-se we should at the very least expose the SVG in a variable that can be overridden easily by a themer without temptation (for newbies) to hack or replace and override the entire autocomplete.js file.

Additionally, abstracting the inline svg into a separate template opens it up for easily droping it into other usages as well.

Edit:

One more point. The current patch modifies a core js file, which has nothing to do specifically with the Seven theme because it will affect all themes, so 1) the scope of the original issue has changed and the issue summary and title needs updating and 2) to drive home the importance of my previous point above, with the current patch as is, there wouldn't actually be a way to cleanly "override" the throbber for any single core theme, without introducing duplicate code.