Problem/Motivation

BAD

  • In Firefox and IE with enabled hardware acceleration (and supporting Subpixel), the input fields are rendered sightly taller. This causes part or all of the spinning/searching state of the throbber in autocomplete fields to be revealed - even while the autocomplete is in idle state (see screenshot above).
  • Furthermore, if the input font size is increased or if extra padding is added to inputs, styles are also broken as above shown - in all browsers.
  • In some browsers, like old Chrome (#1891764: Autocomplete throbber causes excessive CPU usage on some browsers) animated GIFs cause excessive CPU usage. Having the animated searching/spinning state and the idle state of the throbber in a single animated GIF file forces us to load it in all pages that have an autocomplete field. So the CPU issue occurs even when the autocomplete is idle. What's worse is that most of the time users have no clue why their browser hogs their system only on some sites (made with Drupal) and only in certain pages (that contain autocomplete fields).

Proposed resolution

  • Split throbber.gif into 2 files and hide the animated/spinning GIF file by default to resolve high CPU usage.
  • Remove the fixed value and let browsers vertical align the image correctly.

Remaining tasks

N/A

User interface changes

On buggy browsers, it shows the throbber correctly:

GOOD

API changes

N/A

Original report by @ogi

When Bartik is set for adding and editing content, throbber in textfield shows part of the rotating throbber. See the attached screenshot.

I fixed this by changing background-position in my Bartik subtheme by adding 4px. Additionally, I moved throbber slightly left. The second attachment shows the result.

/* Animated throbber */
html.js input.form-autocomplete {
  background-image: url(../../../../../misc/throbber.gif);
  background-position: 99% 6px; /* LTR */
  background-repeat: no-repeat;
}
html.js input.throbbing {
  background-position: 99% -14px; /* LTR */
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet’s picture

Version: 7.0 » 7.x-dev
Issue tags: +IE

tagging IE

ogi’s picture

It's not IE-only, I observe this behaviour in Firefox 4 Beta under Windows 7.

Unfortunately my changes are not enough. Attached is how Tags field is shown with my changes.

droplet’s picture

Issue tags: -IE

oh cool, forgot Im using IE9

I do some test and it seems only happen on browser with hardware acceleration enabled .

enabled hardware acceleration, inputs height will increase... FTW!! (related to font-size: 0.929em, Sub-pixel problem)

ogi’s picture

Yes, after setting gfx.direct2d.disabled to true throbber looks OK.

klonos’s picture

I see this too in latest dev from March 10. I only came across it in the 'Authored by' field in node create/edit. Any progress?

droplet’s picture

limited height or remove INPUTs padding is a way to solve this problem

droplet’s picture

Title: Throbber in textfield in Bartik theme is misaligned » Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx)
Component: Bartik theme » base system
Status: Active » Needs review
Issue tags: +gfx, +sub-pixel
FileSize
8.13 KB

I edited throbber.gif to have more space between hover & non-hover state. and adding JS to calc elements height and apply right background position.

It's potential risk to other themes as well.

Status: Needs review » Needs work

The last submitted patch, throbber.patch, failed testing.

klonos’s picture

@droplet: can you please attach the .gif as a file instead of a binary diff please.

droplet’s picture

FileSize
2.37 KB

@klonos,
sure

correct my comment on #7
JS is calc elements inner height (height + padding). because padding is one of the main reason cause this problem.

klonos’s picture

...thanx, this fixes things, but I don't actually know how it will do with themes other then Bartik.

droplet’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
marcoka’s picture

#10 would help fixing all issues regarding to the change of height of an auto complete input field. meaning you can alter the css and fix it. currently you have to replace the throbber.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
5.9 KB

Here's #10 rerolled to fix some whitespace and to apply against 8.x. Most of the changes appear to have already been committed.

webchick’s picture

Component: base system » javascript
Issue tags: +JavaScript

Oh, nice! This has been driving me nuts in the Spark/Edit queue.

I can't actually have the front-end chops to give this a final RTBC but very excited to see it being worked on! :D

Futzing with some metadata.

ry5n’s picture

So the problem here is the classic one where a sprite will have adjacent elements bleed into view if the size of the container exceeds the sprite spacing in the image. Although it looks like the proposed solution would work (extra space in the sprite), using JS to set the background position seems… like not the right tool for the job.

A better approach in my experience is to use dedicated markup to contain the icon, something like:

<!-- autocomplete-container would have 'position: relative' -->
<div class="autocomplete-container">
  <input class="text-input" type="text" /><span class="icon-throbber"></span>
</div>

.

The icon container would have 'width' and 'height' declared so the sprite spacing is guaranteed not to be an issue. It can also be positioned correctly with just CSS. This is another issue that could benefit from http://drupal.org/node/1849712.

droplet’s picture

Sigh! 2 years old patch, no one review it.

@ryn5,

I was patched for D7 ( no markup changes), :)

ry5n’s picture

@droplet I don’t understand #17. Can you elaborate?

relaxpor’s picture

override the css file with these value

For an idea, in the corolla subtheme.
This is an example to display no throbber when no load
and display a mythrobber.gif when there is load

html.js input.form-autocomplete {
background-image: none;
}

html.js input.throbbing {
background-image: url(../../../../misc/mythrobber.gif);
background-position: 98% 10px;
}

quicksketch’s picture

A better approach in my experience is to use dedicated markup to contain the icon, something like:

This is also what we're doing in system.base.css for the AJAX throbber CSS. +1 to r5yn's approach.

Uccio’s picture

I can confirm that #14 javascript patch fix the problem on D7 (Bartik 7.22 and Garland 7.22).
My working solution uses the image contained in #10 because I have met some difficulties to patch the image file.

almc’s picture

I've got a message for the patch #14: '... cannot be applied in the selected context', tried it with Drupal 7.23. The issue is showing up as a partially visible continuously spinning throbber, even when the autocomplete field is empty. This happens for D7.23 Bartik theme, in IE and Firefox particularly, wile looking good in Opera.

almc’s picture

Status: Needs review » Needs work

The last submitted patch, 14: throbber-alignment-1069152-14.patch, failed testing.

almc’s picture

The patch #10 (.gif replacement) fixes the issue (no continuous spinning), however:

  • it eliminates any spinning of throbber (e.g. when autocomplete is looking for matching)
  • the throbber is not aligned to the center of the field vertically
alexandrezia’s picture

Issue summary: View changes
FileSize
5.86 KB

This is the same patch from comment #14
Just fixing file paths so the patch can be applied by drush make

alexandrezia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 26: throbber-alignment-1069152-14-2.patch, failed testing.

alexandrezia’s picture

Version: 8.x-dev » 7.x-dev
alexandrezia’s picture

Status: Needs work » Needs review
alexandrezia’s picture

Status: Needs review » Needs work

The last submitted patch, 26: throbber-alignment-1069152-14-2.patch, failed testing.

alexandrezia’s picture

Setting to 7.x branch

alexandrezia’s picture

Status: Needs work » Needs review
gmclelland’s picture

I think we would be better off splitting the gif into two separate files and using just CSS to position the throbbers.

The reason I say two files is because it is an animated sprite that is always animating even if you can't visible see it.

To see the problem, turn on painting using the Google Chrome Development Tools or Firefox's Development Tools and you will see the throbber is constantly being repainted.

To eliminate the extra http request maybe we can inline the two small files in the css? See http://css-tricks.com/data-uris/

What does everyone think?

gmclelland’s picture

gmclelland’s picture

droplet’s picture

Version: 7.x-dev » 8.x-dev
Component: javascript » markup
Issue tags: -JavaScript
FileSize
3.6 KB

- Two separate image files
- Fixed GFX issue (padding issue)
- Fixed CPU issue

Status: Needs review » Needs work

The last submitted patch, 38: 1069152-throbber.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review

38: 1069152-throbber.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 38: 1069152-throbber.patch, failed testing.

droplet’s picture

Testbots error ?

droplet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 42: 1069152-throbber-for-testbots-only.patch, failed testing.

The last submitted patch, 42: 1069152-throbber-for-testbots-only.patch, failed testing.

gmclelland’s picture

@droplet - how about using a base64 encoded file in the css? That would eliminate the extra http requests for each throbber. Normally I use http://compass-style.org to do this.

Here is an example of what I'm talking about http://keegan.st/2012/09/14/easy-image-data-uris-with-compass/

droplet’s picture

Personally, I don't think we have to include 500 bytes (or around 3kb of 2 images) extra data to every site. And there's not used for any modules in D8 CORE. I will keep as it for now. :)

It seems like the testbot has problems at the moment, I will retest the patch soon.

droplet’s picture

38: 1069152-throbber.patch queued for re-testing.

The last submitted patch, 38: 1069152-throbber.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
5.79 KB

Reroll

droplet’s picture

almc’s picture

50: 1069152-throbber-50.patch queued for re-testing.

almc’s picture

Triggered re-testing, the patch couldn't be opened in an editor.

Status: Needs review » Needs work

The last submitted patch, 50: 1069152-throbber-50.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
5.97 KB

Same as Patch #55 and updated to jQuery UI coding.

- Fixed new jQuery UI Autocomplete alignment problem

almc’s picture

55: 1069152-throbber-55.patch queued for re-testing.

almc’s picture

Triggered re-testing, I applied the patch to the folder /misc, however haven't noticed a difference in throbber appearance in autocomplete field.

droplet’s picture

in D8, you have to clear caches after any styles changes.

almc’s picture

I thought the patch is for D7 as D8 is not ready for production use yet, and the problem does exist for D7.

droplet’s picture

This is Patch for D8 :)

I will add D7 Patch soon.

Please review it :)

droplet’s picture

55: 1069152-throbber-55.patch queued for re-testing.

droplet’s picture

55: 1069152-throbber-55.patch queued for re-testing.

droplet’s picture

55: 1069152-throbber-55.patch queued for re-testing.

eigentor’s picture

FileSize
3.33 KB

Tested the patch with latest dev and works.
Maybe you should write a reasoning what you did and why with your patch.

As far as I can see, it divides the throbber into two images instead of one, so the image is always perfectly placed, even if the input field gets big:

Autocomplete Throbber

klonos’s picture

This looks great! Can't wait to see make its way back to D7.

almc’s picture

agree, given that D7 version is the production platform of choice now ...

droplet’s picture

Issue summary: View changes
klonos’s picture

Issue summary: View changes
eigentor’s picture

Status: Needs review » Reviewed & tested by the community

To RTBC this i guess it needs a code review. It being mostly in system.module.css and not in sevens css files seems right to me, but I don't know if there are any special policies as to which css files to use.'

The css itself cannot be much wrong: it is set to right and vertically centered. Not much more in here. So I'll be bold and set it RTBC, maybe this will trigger someone else to put it under scrutiny.

sun’s picture

This is a great clean-up and we should move forward here.

That said, I see two issues that could be addressed in follow-ups:

  1. The throbber looks a bit squeezed to the right border.

    → Instead of 100%, we could consider to use 98% (or a fancy CSS trick, if there is any).

  2. I wonder why this is an animated GIF in the first place?

    → If we'd consider to put the throbber next to the input element (instead of inside), then CSS3 is simple as this:

    .throbber {
      animation: throbber-rotation 1s infinite linear;
    }
    
    @keyframes throbber-rotation {
      from { transform: rotate(0deg); }
      to   { transform: rotate(359deg); }
    }
    

    cf. http://jsfiddle.net/uv8a5/

    See also:

    http://37signals.com/svn/posts/2577-loading-spinner-animation-using-css-...
    http://aino.github.io/throbber.js/
    http://onwebdev.blogspot.com/2011/01/css3-background-rotate-property.html
    http://blog.freeside.co/post/42357804660/css-background-transformations

klonos’s picture

@sun: Great idea about using CSS animation instead! Filed #2181399: Consider moving the animated .GIF throbber outside the autocomplete fields and using CSS animation instead. for your suggestion (and gave credits to you for it).

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Nice work! Committed 8910339 and pushed to 8.x. Thanks!

alexpott’s picture

Version: 8.x-dev » 7.x-dev
cmonnow’s picture

Interestingly, I only noticed on my PC a few weeks ago that drupal.org's animated throbber was peeking through at the bottom of inputs in all of my browsers (in Ubuntu 12.04), which I never noticed previously. I wasn't sure if it was a site change or some visual configuration was changed on my setup.

klonos’s picture

Yep, still an issue and now that d.o runs on D7 it hits more users:

droplet’s picture

Component: markup » CSS
Status: Patch (to be ported) » Needs review
FileSize
5.78 KB
nitishchopra’s picture

FileSize
6.6 KB

Thanks for that patch. This is how the throbber field looks like on firefox 28 on ubantu 12.04.The issue seems to be solved.

tstoeckler’s picture

Issue tags: +affects drupal.org

Per #75

Manjit.Singh’s picture

76: 1069152-throbber.patch queued for re-testing.

mgifford’s picture

#77 seemed like a good review, even though it was on April Fools.

@nitishchopra any reason you didn't mark it RTBC? Also, just to confirm, you've got hardware acceleration on and see the bug without the patch?

droplet’s picture

Notes to reviewers,

You won't have to enabled / disabled hardware acceleration on your browsers. You can apply Padding to input box. It's same results. Some tests already preformed on D8 patch. Generally said, I think it only needed code reviews.

Thanks.

Steven Jones’s picture

Before inactive:

Before throbbing:

After inactive:

After throbbing:

Patch looks okay technically, but I think this constitutes and kind of an API change doesn't it?
In the patch Bartik and Seven need to be adjusted to make the new throbber work, and any themes that assume the structure and do similar things, will need to be changed too? This could cause a lot of annoyance for themes. Although I admit that when I've done this in themes, I've basically removed all of the basic throbber stuff, and replaced it with my own.
This should be called out in the release notes at the very least.

I'm happy to mark to this as RTBC in the near future if someone is happy that the 'API change' is okay.

droplet’s picture

In the patch Bartik and Seven need to be adjusted to make the new throbber work, and any themes that assume the structure and do similar things, will need to be changed too?

the code on themes just overrided the CORE base styles. On the patch above, we removing the excessive code from Bartik & Seven only. If you have similar code on your theme. It remains SAME results. (That means, no fixes to your themes)

I won't consider it's an API change, at least it's not a edge cases.

Steven Jones’s picture

Status: Needs review » Reviewed & tested by the community

RTBC then, because of #82 and #83

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 1069152-throbber.patch, failed testing.

droplet’s picture

76: 1069152-throbber.patch queued for re-testing.

mgifford’s picture

76: 1069152-throbber.patch queued for re-testing.

mgifford’s picture

Status: Needs work » Reviewed & tested by the community

Sorry, @droplet already did that. It passes so back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 1069152-throbber.patch, failed testing.

mgifford queued 76: 1069152-throbber.patch for re-testing.

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community

Testbot failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 1069152-throbber.patch, failed testing.

dcam queued 76: 1069152-throbber.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 1069152-throbber.patch, failed testing.

droplet queued 76: 1069152-throbber.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 1069152-throbber.patch, failed testing.

dcam queued 76: 1069152-throbber.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 1069152-throbber.patch, failed testing.

droplet’s picture

Status: Needs work » Reviewed & tested by the community

droplet queued 76: 1069152-throbber.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 1069152-throbber.patch, failed testing.

mgifford queued 76: 1069152-throbber.patch for re-testing.

mgifford’s picture

Status: Needs work » Reviewed & tested by the community

bad bot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 1069152-throbber.patch, failed testing.

Mixologic’s picture

FileSize
5.78 KB

Reuploading a patch with non-duplicate file names.

Mixologic’s picture

Status: Needs work » Needs review
jthorson’s picture

Status: Needs review » Reviewed & tested by the community

Retest passed.

David_Rothstein’s picture

Title: Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx) » (Followup for Bartik RTL styling) Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx)
Status: Reviewed & tested by the community » Needs work
Issue tags: +7.33 release notes

Committed to 7.x - thanks!

I deliberately did not delete misc/throbber.gif during the commit though (we should leave that in place since other CSS out there certainly might still be referencing it).

Also:

--- a/themes/bartik/css/style.css
+++ b/themes/bartik/css/style.css
@@ -1326,14 +1326,6 @@ input.form-button-disabled:active,
   color: #717171;
 }
 
-/* Animated throbber */
-html.js input.form-autocomplete {
-  background-position: 100% 4px; /* LTR */
-}
-html.js input.throbbing {
-  background-position: 100% -16px; /* LTR */
-}
-

I'm pretty sure we need a small followup to fix this in Bartik for RTL languages also?

  • David_Rothstein committed f7cf579 on 7.x
    Issue #1069152 by droplet, alexandrezia, Mixologic, jhedstrom | ogi:...
droplet’s picture

Title: (Followup for Bartik RTL styling) Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx) » Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx)
Status: Needs work » Fixed

No. RTL is already done on base CSS.

David_Rothstein’s picture

Title: Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx) » (Followup for Bartik RTL styling) Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx)
Status: Fixed » Needs review
FileSize
483 bytes
8.88 KB
8.87 KB

Yes, but Bartik overrides that.

See the attached patch and screenshots. It's not a huge difference, but before the patch it still looks a bit misaligned to me, and after it looks fixed.

Before:

After:

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Good catch thanks @David_Rothstein, this is consistent with the rest of the patch.

David_Rothstein’s picture

Title: (Followup for Bartik RTL styling) Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx) » Throbber in textfield is misaligned when browser hardware acceleration enabled (gfx)
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

Status: Fixed » Closed (fixed)

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