'Set as Wallpaper' should wait till image is fully loaded before setting as wallpaper

RESOLVED FIXED in mozilla1.1alpha


UI Design
16 years ago
9 years ago


(Reporter: Chris Nebergall (cneberg@gmail.com), Assigned: Christopher Aillon (sabbatical, not receiving bugmail))


Windows XP

Firefox Tracking Flags

(Not tracked)




(1 attachment, 4 obsolete attachments)



16 years ago
This may require a slow connection.  I'm connected at 33.6

Steps to reproduce
1.  Go to listed URL
2.  As the image is loading right click and select 'set as wallpaper'
3.  Partially loaded image is set as wallpaper

expected results:  Image will not be set as wallpaper till its finished loading
or 'Set as wallpaper' will be greyed out until the image is fully loaded.

I realize this problem is some what trivial, but considering that this feature
is for new users, there is no reason to make the process more complicated than
it has to be.  Also, this probably be a fairly common problem because on a slow
connection most people will decide if they like an image before its 100% loaded.

Comment 1

16 years ago
Seeing this on 2002030803, Win 98. Confirming.
Ever confirmed: true
Created attachment 80146 [details] [diff] [review]
Patch v1.0

This just disables the item for <img> images while we're still loading them. 
Image maps and <object>s will not be fixed by this, but it's a start... The
real issue is that we have no real way (AFAIK) from the DOM to tell that
something has loaded other than to set up a listener for "onload".  A
.hasLoaded property or something which would apply to more than just IMG and be
true only if the element has loaded successfully, would be nice...  but in the
meantime I guess we can stick to img.complete.	This is a good workaround
though for like 95% (or more?) of images on the web.
Created attachment 80155 [details] [diff] [review]
Patch v1.1

(!A || (A && B)) == (!A || B)
Attachment #80146 - Attachment is obsolete: true

Comment 4

16 years ago
Created attachment 80250 [details] [diff] [review]
My take on the patch

I've moved the completion test for this patch.
Comment on attachment 80250 [details] [diff] [review]
My take on the patch

Yes but in the meantime you've broken the context menu so that it never appears
for anything except an <img>.
Attachment #80250 - Flags: needs-work+
Assignee: blaker → caillon
Comment on attachment 80155 [details] [diff] [review]
Patch v1.1

change |if (isWin)| to |if (isWin && this.onImage)| as discussed on IRC and
Attachment #80155 - Flags: review+

Comment 8

16 years ago
Created attachment 80284 [details] [diff] [review]
Address caillon's concerns
Attachment #80250 - Attachment is obsolete: true
Created attachment 81297 [details] [diff] [review]
Updated against tip

The other patch doesn't apply anymore.	Merged to tip.
Attachment #80155 - Attachment is obsolete: true
Attachment #80284 - Attachment is obsolete: true
Comment on attachment 81297 [details] [diff] [review]
Updated against tip

Still has r=biesi
Attachment #81297 - Flags: review+

Comment 11

16 years ago
Comment on attachment 81297 [details] [diff] [review]
Updated against tip

Attachment #81297 - Flags: superreview+

Comment 12

16 years ago
Instead of
  navigator.appVersion.indexOf("Windows") != -1
can you say
  navigator.platform == "Win32"
becuase won't all Windows platforms say Win32? Or is this too trivial to change?
Checked in on the trunk.  Going to see if this can make the branch.

Pete: navigator.appVersion was already there.  I just moved the code around a
bit.  We use both references throughout our code base, so you might consider
filing a bug calling for consistency.
Priority: -- → P3
Target Milestone: --- → mozilla1.0
Drivers isn't interested in taking this for 1.0.  Resolving fixed.
Last Resolved: 16 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.0 → mozilla1.1alpha
Product: Core → Mozilla Application Suite


9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.