XUL Alerts: Hide the image box if the alert image fails to load

NEW
Unassigned

Status

()

defect
4 years ago
2 years ago

People

(Reporter: lina, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

See the attached screenshot. If an image fails to load (for instance, because the request returns a 404), we should treat it as if the alert doesn't have one. We currently show large blank space instead.

Only XUL alerts are affected by this; OS X and libnotify use `imgLoader` directly.
Duplicate of this bug: 1337494
I took a shot at a fix, but it's slightly tricky because we adjust the height of the alert based on whether it has an image, and we also use that to decide where to position subsequent alerts. Does that mean we'd need to reposition all alerts below it, so that we don't end up with a gap? Alternatively, should we wait until the image loads before showing the alert at all?
(In reply to Kit Cambridge [:kitcambridge] from comment #2)
> I took a shot at a fix, but it's slightly tricky because we adjust the
> height of the alert based on whether it has an image, and we also use that
> to decide where to position subsequent alerts. Does that mean we'd need to
> reposition all alerts below it, so that we don't end up with a gap?
> Alternatively, should we wait until the image loads before showing the alert
> at all?

IMO, you solve the layout issue by having a placeholder icon that gets replaced when the icon is loaded. This also means you don't need separate layout modes for text and icon layouts or need to worry about layout reflow if something goes wrong with an icon. That'd also ensure that notifications have a uniform appearance when icon and text notifications are mixed together.

But barring that, IMO, it'd be best to assume an icon will load and display notifications immediately since they might be time-sensitive. (Though it'd seem to be better if there was a way for an author to decide whether notification presentation or immediacy of display take precedence.) If the icon fails to load, snap to text-only layout and use animation to slide the notification downward until it's adjacent (with the relevant margin) to the notification below it.
(In reply to Kit Cambridge [:kitcambridge] from comment #2)
> I took a shot at a fix, but it's slightly tricky because we adjust the
> height of the alert based on whether it has an image, and we also use that
> to decide where to position subsequent alerts. Does that mean we'd need to
> reposition all alerts below it, so that we don't end up with a gap?
> Alternatively, should we wait until the image loads before showing the alert
> at all?

If we were to wait, how long before we time out? This delay could be really bad on some connections, and make the timeliness of a notification less useful. On the other hand, we could assume that it doesn't have an image and then resize once the image loads, though that may cause the notification to visibly jump around which would be annoying as well (and likely more common). I think the safest route to go would be to use a fall-back image if one is specified. If the requested image has an error, then we can set the image attribute to the fall-back image. This will keep the height of the pop-up unchanged and side-steps the problem about re-positioning other alerts.
Comment on attachment 8834741 [details]
Bug 1238240 - Hide the image box for XUL alert images that fail to load.

https://reviewboard.mozilla.org/r/110584/#review112592

This patch is fine, but after more thought I think a fall-back image would be better. We should be able to use chrome://browser/skin/notification-icons.svg#desktop-notification as the fallback image.

::: toolkit/components/alerts/resources/content/alert.js:145
(Diff revision 1)
> +        alertImage.addEventListener("load", onImageEvent);
> +        alertImage.addEventListener("error", onImageEvent);

normally I would say to use {once: true} with the call to addEventListener, but since your event listener is set up to remove both event listeners once one is called this may not be feasible.

Note that there is an eslint rule that might flag your code for using {once: true} with "load".
Attachment #8834741 - Flags: review?(jaws) → review-
Thanks, Jared and Patrick, that makes sense. I won't have cycles to work on this again until later in the week. Please feel free to steal the bug from me if you'd like to fix it in the meantime. If not, I'll push an updated patch with the fallback image later.
It’s not clear what color the SVG icon should be. The default color is black, which is kind of harsh.
This image was apparently created from a decomposed image created in bug 137123 which is why the URL differs from that in comment #6.
Err: I meant bug 1371230.
You need to log in before you can comment on or make changes to this bug.