1176081 - Consider not showing placeholders for images that are loading

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

If we paint the page while it is stil loading, and there are images that are not yet loaded, we show the "broken image" icon. I think it would probably look nicer to just not do that while the page is loading. If the image load fails, show it then.

Comment 1

[Timothy Nikkel (:tnikkel)]

I think you are talking about the loading image placeholder, it is distinct from the broken image placeholder.

I think the ideal behaviour for all images (regardless of page loading or not) would be to draw nothing or a light gray outline for some fixed time (say 500ms) and then if the image is still not available show the loading image placeholder.

Or we could redesign the loading image placeholder to more subtle all the time.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

I guess we sorta have a pref for this, browser.display.show_image_placeholders. But that looks like it might disable them entirely, which is not quite the same.

Comment 3

[Seth Fowler [:seth] [:s2h]]

I concur; I also really dislike the appearance of the current placeholders.

Maybe we should just show a shaded region where the image would go? Like a black rect the size of the image with 33% alpha or something?

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch

This adds a pref to control drawing the loading image placeholder (assuming the image placeholder pref is enabled) and disables that pref on android and b2g. If this works well we should do the same on desktop. We will still draw a border in nsImageFrame::DisplayAltFeedback, this should give the user an indication that an image will go there, without being too visually busy.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8624512 [details] [diff] [review]
patch

Review of attachment 8624512 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 6

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45367c7530a1

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add separate prefs for disabling image loading/error placeholders

There is a bunch of churn here that just allow us to not add the nsDisplayAltFeedback item if we don't need it

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8624512 [details] [diff] [review]
patch

Review of attachment 8624512 [details] [diff] [review]:
-----------------------------------------------------------------

Hah, I didn't even see that you already posted a patch. Looks fine to me :)

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think I want to kill the border now too. It's terrible. It looks like a MOTIF widget. We should just not show anything at all while an image is loading.

A good test site is: http://www.deelay.me/

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2 - disable border too, and flip the pref everywhere

I did more testing, and Chrome/Safari never draw a border, even if the image size is given.

This patch also removes the border based on snorps patch.

And I went ahead and flipped the pref to disable the loading icon/border everywhere.

Comment 11

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=572c0f973916

Comment 12

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8631476 [details] [diff] [review]
patch v2 - disable border too, and flip the pref everywhere

Review of attachment 8631476 [details] [diff] [review]:
-----------------------------------------------------------------

Love it.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acc0b3f3daba

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/acc0b3f3daba

Comment 15

[Ray Satiro]

Attachment: Screenshot of artifacts in image placeholder

Can someone reopen this, it's causing what looks to be artifacts. I've attached a screenshot. I'm using Nightly 20150811011940 ec77f5c19262 but I ran mozregression and bisected it to acc0b3f3daba. Also see a short video at https://vid.me/OkZ5 demonstrating the problem. The steps to reproduce are:


- Go to https://github.com/mozilla/mozregression

- Scroll down the page until the README is in view and you can see the following links:

- - Latest Version Downloads Python versions License

- Click on setup.py

- Click the back button on your mouse

- Note that the links are truncated and not rendering properly

- To coax the issue you may have to repeat clicking forward and then back up to several dozen times.


In this particular case I don't think the prior behavior is correct either though. You can see in prior builds, eg 20150710030206, instead of those artifacts there'd occasionally be missing image placeholders which never went away; which is weird because they're just links and if I'm going back a page those should already be rendered and never missing. Should I file that as a different bug?

Comment 16

[Timothy Nikkel (:tnikkel)]

Can you file a new bug for this please? Usually we handle regressions in new bugs (unless we back out the original patch, but this doesn't seem bad enough to back out for).

Comment 17

[Ray Satiro]

Ok. I filed a new report at https://bugzilla.mozilla.org/show_bug.cgi?id=1194928