The loading spinner graphic that appears when loading the overlay content is often not visible because is is displayed outside the viewport. Currently, it is displayed at a position of 50% 50% (right in the middle of the current overlay). If the current overlay is particularity long, the spinner is often displayed out of sight. I find that this looks odd and inconsistent as sometimes I see a spinner and sometimes I don't. I should either see it every time or not at all.

Two solutions I can think of:

1) Display it a set number of pixels from the top of the overlay (eg. 100px) instead of centring it. This would ensure that it is visible all the time.

2) Calculate the size of the viewport and centre the spinner within that. This is probably overkill though.

Attached patch does option 1 and IMO is an improvement as it ensures consistency when clicking around on the top level tabs (click People, then Structure, then Content etc) It is not perfect, as for example, if you scroll down to the bottom of an overlay and click a link, the spinner is now displayed at the top of the overlay - out of sight. However, this is the same situation as without the patch (visibility depends on where you are scrolled to in the overlay). A JavaScript solution more like what I describe in option 2 would be needed to to address that.

CommentFileSizeAuthor
#25 jquery142overlay.patch1.05 KBcasey
#14 overlayloading.patch3.56 KBcasey
#12 overlayloading.patch3.56 KBcasey
#3 overlayloading.patch677 bytescasey
#2 overlayloading.patch652 bytescasey
overlayloadinggraphic.patch859 bytesmrfelton
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrfelton’s picture

mrfelton CreditAttribution: mrfelton commented
Title: Loading 'spinner' often not visible because it is displayed outside the viewport. » Overlay's Loading 'spinner' graphic often not visible because it is displayed outside the viewport
casey’s picture

casey CreditAttribution: casey commented
FileSize
overlayloading.patch652 bytes

Something like option 2 is pretty simple. A it looks nicer I think.

casey’s picture

casey CreditAttribution: casey commented
FileSize
overlayloading.patch677 bytes

Oops

mrfelton’s picture

mrfelton CreditAttribution: mrfelton commented

Yes, agreed, that is even better.

Status: Needs review » Needs work

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

Status: Needs work » Needs review

Re-test of overlayloading.patch from comment #3 was requested by casey.

Kiphaas7’s picture

Kiphaas7 CreditAttribution: Kiphaas7 commented

Patch in #3 fixes this issue for me. mrfelton, did you also test it or do you just agree? RTBC then, although it probably needs a re-roll.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Fixed

Thanks, this looks good to me! Still applies cleanly. Committed to HEAD.

Status: Fixed » Closed (fixed)

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

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Status: Closed (fixed) » Active

Casey notified me that currently there is kind of a basket open/closing effect between each overlay. For example :

1. Structure admin page
2. I click on Content
3. Loading icon
4. The white area quickly collapses and opens to the appropriate size
5. Content page

So my main problem is with 4, it shouldn't collapse it should stay the same size, get bigger or get smaller - it shouldn't flickr to 10% and then go to 65%.

Kiphaas7’s picture

Kiphaas7 CreditAttribution: Kiphaas7 commented

New idea: rollback #3 to prevent the jumping from #10, and fix the position of the spinner with inline styles, set with javascript?

casey’s picture

casey CreditAttribution: casey commented
Status: Active » Needs review
FileSize
overlayloading.patch3.56 KB

Or just keep overlay content visible while loading... just like other browsers do.

Maybe this needs a spinner next to the "Loading..." title.

Well give this a try... overlay documents will be visible quicker.

casey’s picture

casey CreditAttribution: casey commented

Result is even better combined with patch in #674852: Remove shortcut-add-remove-link and tabs in Drupal.overlay.load instead of Drupal.overlay.bindChild.

casey’s picture

casey CreditAttribution: casey commented
FileSize
overlayloading.patch3.56 KB

Reroll

casey’s picture

casey CreditAttribution: casey commented

#686926: Keep last page until next page has loaded in overlay instead of spinner

casey’s picture

casey CreditAttribution: casey commented

#674852: Remove shortcut-add-remove-link and tabs in Drupal.overlay.load instead of Drupal.overlay.bindChild is committed.

This patch also fixes issues like #690276: 'Weight' column is not displayed and shifts other columns to left

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Tested it, and i'm putting this to RTBC.
This looks way better. The only thing we can discuss is when the tabs should dissapear.
Like they do now or do they have to wait untill the page is loaded.
But this sounds more like a follow up patch.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community
aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Reviewed & tested by the community » Needs review

Putting it back, I'm running some tests with other overlay patches

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

This can go in now after some extra testing!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

My experience with this latest patch (FF 3.6, Mac) is that I never see the spinner at all. The only subtle indication that anything happened when I clicked something is the title changes to "Loading..."

I agree it seems to be loading pages faster, which is definitely a good thing, but I think we need that eye-catching animation somewhere (Maybe next to the word "Loading..." ?) so that people don't continue clicking thinking that a particular link didn't do anything.

Make sense, or am I on crack?

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

I don't know....

I haven't got a good view on it, cause I have full speed internet, running drupal on localhost and got a fast computer so loading takes less then 2 second....
Do you think that users will have the time to click away before the page is loaded.

casey’s picture

casey CreditAttribution: casey commented

The patch is committed by accident? Anyway I think it's an improvement.

About the spinner; do we need it? Browsers already show a spinner/loadingbar when loading something. I think that's enough visual indication.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

I think extra spinners will make things ugly...

casey’s picture

casey CreditAttribution: casey commented
Status: Needs work » Needs review
FileSize
jquery142overlay.patch1.05 KB

If we keep this patch we need this followup patch to fix an issue since jquery 1.4 is committed.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

Great and fast

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Btw webchick you already commited patch #14

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

Oops. :P I didn't mean to do that. :P

Well, anyway, committed the fix in #25.

If the consensus here is to get rid of the spinner, then we should remove that graphic and references to it in the CSS. I still feel like this might need some discussion with the UX team, though. If not, then feel free to move back to fixed.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Last call for UX people :)???

Can someone make a patch adressing comment of webchick (#28)

casey’s picture

casey CreditAttribution: casey commented

Apparently there were people that requested this: #686926: Keep last page until next page has loaded in overlay instead of spinner.

If we keep it this way, we can remove the spinner image.

eigentor’s picture

eigentor CreditAttribution: eigentor commented

Yippee, +1 to get rid of the spinner rather today than tomorrow :)
It may be a relic from the times when the overlay was loading real slow.

casey’s picture

casey CreditAttribution: casey commented
Status: Needs work » Closed (fixed)

#668640: Overlay shouldn't be based on jQuery UI Dialog landed. Let's close this issue.

There is however no loading animation when opening the overlay, but we should discuss this in another issue.