Closed Bug 897491 Opened 8 years ago Closed 7 years ago

alt attribute inserted dynamically is not used by broken images

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: zbraniecki, Assigned: mjh563)

Details

(Whiteboard: [mentor=bz][lang=c++])

Attachments

(1 file, 1 obsolete file)

4.10 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Gecko does not use alt attribute inserted from running code in place of a broken image.

Example:

<html>
  <head>
    <script type="text/javascript">
window.addEventListener('load', function() {
  document.getElementById('img1').setAttribute('alt', 'Use me');
});
    </script>
  </head>
  <body>
    <img id="img1" src="./non-existing.jpg" />
  </body>
</html>

Current behavior: img doesn't use the alt even after it has been set

Expected behavior: as soon as alt attribute is set, img box should use it

Example consequence is that client-side L10n libraries (Firefox OS l10n, L20n etc.) cannot set localized alternative text on nodes.
Hmm.  So this happens because -moz-alt-content uses the value of the alt attr _if_ it's present, but if it's absent just doesn't generate an attribute content kid at all.

I believe this is the behavior the spec requires.  

We should probably implement a GetAttributeChangeHint here that does a reframe if the <img> has a frame and that frame is not an image frame.
Whiteboard: [mentor=bz][lang=c++]
(In reply to Boris Zbarsky (:bz) from comment #1)
> Hmm.  So this happens because -moz-alt-content uses the value of the alt
> attr _if_ it's present, but if it's absent just doesn't generate an
> attribute content kid at all.
> 
> I believe this is the behavior the spec requires.  

Do you have any pointer in spec that defines that? I was looking in HTML5.1 spec but it doesn't seem to cover this scenario at all.
It looks as though a reframe is needed when both of these are true:

1. no valid image is displayed, and
2. the alt attribute is either added or removed

I'm not sure how to find out if HTMLImageElement is displaying a valid image though (as opposed to a broken image placeholder). This seems to work, but is there a better way?

bool isValidImage = false;
if (mCurrentRequest) {
  uint32_t status;
  mCurrentRequest->GetImageStatus(&status);
  isValidImage = (status & imgIRequest::STATUS_ERROR) == 0;
}
Attached patch patch (obsolete) — Splinter Review
bz, I've requested feedback rather than review because I'm not sure if this patch is the right way to fix the bug or not. What do you think?

In bug 408782, a similar bug with image inputs was fixed in a simpler way. Would it be better to do it like that instead?

And I should include some tests as well, right?
Assignee: nobody → mjh563
Attachment #785757 - Flags: feedback?(bzbarsky)
Comment on attachment 785757 [details] [diff] [review]
patch

> Do you have any pointer in spec that defines that?

HTML5.  See the parts about what an image represents and the parts in the "rendering" section that depend on that.

For checking for a valid image, just check that GetPrimaryFrame() and that do_QueryFrame to nsImageFrame fails?

Or you could unconditionally reframe on add/remove of @alt, I guess.  Should be pretty rare anyway.
Attachment #785757 - Flags: feedback?(bzbarsky) → feedback+
> For checking for a valid image, just check that GetPrimaryFrame() and that
> do_QueryFrame to nsImageFrame fails?

I don't think that will work, because (in quirks mode, at least) a broken image placeholder is displayed when there's no valid image and no alt text. The check would give the wrong result in that case.

But I decided to skip all that anyway, and just do this:

> Or you could unconditionally reframe on add/remove of @alt, I guess.  Should
> be pretty rare anyway.
Attached patch patch v2Splinter Review
This patch includes tests for adding and removing the alt attribute in both quirks and standards mode.

I confirmed that the tests pass with the code change applied and fail with the code change not applied.
Attachment #785757 - Attachment is obsolete: true
Attachment #791147 - Flags: review?(bzbarsky)
Comment on attachment 791147 [details] [diff] [review]
patch v2

r=me.  Thank you for fixing this!
Attachment #791147 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7752f8198098
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.