Aaron Leventhal Reporter
	Description • 20 years ago

When ALT text is missing on an image or image map area, and there is no title,
our current repair strategies are not very good.

For an image link or image map area, we can follow to the linked-to page and
grab the title.

	sairuh (rarely reading bugmail)
	Comment 1 • 20 years ago

aaron, is this problem only specific to Firefox (branch or trunk or both?), or
does it also occur in Seamonkey?

	Aaron Leventhal Reporter
	Comment 2 • 20 years ago

The repaired text will be available through the accessible name interfaces for
each accessibility API toolkit.

	Benjamin Smedberg
	Comment 3 • 20 years ago

When do you plan to retrieve the linked page title? Async network loading and
privacy issues both come into play here, as well as trying to convert an
arbitrary network load into usable title information.

	Hixie (not reading bugmail)
	Comment 4 • 20 years ago

Make sure to ONLY do this for images in *links*. The current algorithm was tuned
over a very long time to be the best we could come up with for non-link images.

	Aaron Leventhal Reporter
	Comment 5 • 20 years ago

(In reply to comment #4)
> Make sure to ONLY do this for images in *links*. The current algorithm was tuned
> over a very long time to be the best we could come up with for non-link images.

Which code are you talking about? The current impl we have for creating an
accessible name for missing ALT text lives in the accessibility module and just
uses the filename of the image. It's not finely tuned.

	Aaron Leventhal Reporter
	Comment 6 • 20 years ago

(In reply to comment #3)
> When do you plan to retrieve the linked page title? 
Several conditions must be present:
1. Accessibility API impl has been initialized by a screen reader or other
assistive technoloy.
2. Pref is turned on (turned off by default for now)
3. Image content with no ALT text, but links to another page, is initialized.

> Async network loading
Is this an issue?

> and privacy issues both come into play here
I'd like to understand this more, but this is still something that 1) will
really only come into play for users who need it, such as screen reader users
and 2) can be turned off. What's the privacy issue, anyway? Cookie related?

> as well as trying to convert an arbitrary network load into usable 
> title information.
It has a fallback repair method if the incoming data is not HTML. The fallback
repair method is similar to the old method of using the image filename, but also
uses the name of the document being pointed to. It tries to choose the better
name from those two.

	Aaron Leventhal Reporter
	Comment 8 • 20 years ago

Benjamin, I just realized you weren't CC'd when I added comment 6.

	Benjamin Smedberg
	Comment 9 • 20 years ago

> > Async network loading
> Is this an issue?

You aren't allowed to block the main thread to do network loading, so yes.

> and 2) can be turned off. What's the privacy issue, anyway? Cookie related?

You are going to need to validate the protocol; you must not try to pre-load
javascript: URIs, because when they are attached to links they frequently
perform one-time actions that should only occur when the user actually chooses
the link. In addition, you need to make sure you are security-checking using
checkLoadURI.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 20 years ago

So... that won't work at all for XHTML, huh?

	Aaron Leventhal Reporter
	Comment 11 • 20 years ago

Bz, I'll test/work on the XHTML part in a separate bug, if that's okay. XHTML is
important. But, I'm not actually sure that it won't work as it is.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 20 years ago

Do you really want to be extracting something from 404 responses?

Also, is there a reason the channels aren't being added to the page's loadgroup?
I'd think that leaving the page should cancel those channels, no?

	Aaron Leventhal Reporter
	Comment 14 • 20 years ago

(In reply to comment #13)
> Do you really want to be extracting something from 404 responses?
Definitely not. How do I detect them?

> 
> Also, is there a reason the channels aren't being added to the page's loadgroup?
> I'd think that leaving the page should cancel those channels, no?
Leaving the page (closing it) will cancel my channels indirectly, because the
image accessible will be destroyed. Cancelling the load won't yet though. Sounds
like using the loadgroup is good. What other things does the load group
association do besides sharing load cancels?

	Hixie (not reading bugmail)
	Comment 15 • 20 years ago

> Which code are you talking about? The current impl we have for creating an
> accessible name for missing ALT text lives in the accessibility module and just
> uses the filename of the image. It's not finely tuned.

My bad, I thought you meant the text that we made up for missing alt attributes.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 20 years ago

(In reply to comment #14)
> > Do you really want to be extracting something from 404 responses?
> Definitely not. How do I detect them?

http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/public/nsIHttpChannel.idl#179

> What other things does the load group association do besides sharing load
> cancels?

Allows the progress on the loads to be tracked by the progress listeners
attached to the page, and allows the loads to block the onload event (unless the
LOAD_BACKGROUND flag is used).

	Aaron Leventhal Reporter
	Comment 17 • 20 years ago

(In reply to comment #15)

> My bad, I thought you meant the text that we made up for missing alt attributes.

No, the bug title is misleading. Fixing.

	Aaron Leventhal Reporter
	Comment 18 • 20 years ago

The one thing I really don't like is the strange ways I have to kill the
parser+request combo. It's like trying to stop a freight train with an emegency
break. Stuff you don't want still happens afterward.

  mParser->CancelParsingEvents();
  mParser->Terminate();
  request->Cancel(NS_ERROR_HTMLPARSER_STOPPARSING);
  mChannel = nsnull;
  mParser = nsnull;

That's still not good enough. Sometimes my OnStartRequest() still gets called
after that. To avoid a crash I have to keep an "have we died yet" boolean member
variable around, so my OnStartRequest doesn't try to use any dea pointers.

	Aaron Leventhal Reporter
	Comment 19 • 20 years ago

(In reply to comment #18)
> That's still not good enough. Sometimes my OnStartRequest() still gets called
> after that. To avoid a crash I have to keep an "have we died yet" boolean member
> variable around, so my OnStartRequest doesn't try to use any dea pointers.

In fact, in some cases the parser itself keeps going, calling things like AddLeaf().

How can I stop the parser and network request at the same time?

	Aaron Leventhal Reporter
	Comment 20 • 20 years ago

Still to do:
// 1. Fix http://my.netscape.com/index2.psp//
//    With NodeInserted handling its ovewriting its own progress.
//    Can't load 2nd page.
// 2. netscape.com still crashing sometimes
//    0[3d30f0]: ###!!! ASSERTION: This cache entry shouldn't exist already:
'!oldAccessNode', file c:/moz/mozilla/accessible/src/base/nsAccessNode.cpp,
line 451
// 3. AccessibleObjectFromPoint broken
// 4. File separate bug to make it work with XHTML
// 5. File separate bug on Terminate() not working correctly

	Aaron Leventhal Reporter
	Comment 22 • 20 years ago

This should give people some insight into the repairs that we'll be able to do
with this fix.

Note, some of these images already had ALT text, but I'm logging the repairs
that would have happened. When the log says * Link repair by title, it means
that the fallback text is not necessary, but is calculated anyway to help
refine the heuristic.

	Aaron Leventhal Reporter
	Comment 24 • 20 years ago

Last remaining problem is to figure out why cancelling a load screws up future
loads.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 20 years ago

Note that nsIChannel inherits from nsIRequest, so QIing mChannel to nsIRequest
is not needed....

	Aaron Leventhal Reporter
	Comment 26 • 20 years ago

Boris, would you be willing to look at this since you have been a little bit
already?

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 20 years ago

Er... I can try, but a real review (instead of just looking at a random spot and
commenting if I see something) will take some time... :(

	Aaron Leventhal Reporter
	Comment 28 • 20 years ago

(In reply to comment #27)
> Er... I can try, but a real review (instead of just looking at a random spot and
> commenting if I see something) will take some time... :(

I understand. When do you think you'll have time for it? The title grabbing code
will be off by default at first.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 20 years ago

I'll try to look at it this coming weekend...

	Louie Zhao
	Comment 31 • 20 years ago

Before going through the patch, I have one question for this solution: If a page
contains many linkable images which have no ALT text, mozilla will try to load
all these linked pages (am I right?). How will this impact the performance ?

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 20 years ago

Comment on attachment 171640 [details] [diff] [review]
Contains some fixes suggested by pkw, including better i18n for "linksto" string composition

>Index: accessible/src/base/nsAccessible.cpp

>+  if (inputContent || objectContent) {

This branch is wrong for <object>.  <object> doesn't use <alt>, and if an
<object> cannot be rendered (with the exception of sized image objects in
quirks mode) it will show its children as  the alternate content.  So for
<object> the right check is to check whether it has children.

Also, why are we treating <img> and <input type="image"> differently?

>Index: accessible/src/base/nsBaseWidgetAccessible.cpp
>+NS_IMETHODIMP nsBaseImageAccessible::Init()

Is this expecting to be instantiated for only certain types of nodes?  If so,
please put preconditions here asserting that.  I can't tell whether this code
is right as things stand.

>+  // Split the new URI into a left and right part
>+  // (assume we're parsing it out right

Please, please do not do this.	There are nsIURL methods that will get you what
you want here, no?  Don't do your own URI parsing.

>+void nsBaseImageAccessible::GetLinkToText(nsAString& aResult,

I didn't really chek the logic here very carefully...

>+nsAString& nsBaseImageAccessible::CoreOfString(nsString& aText)
>+  while (iterEnd != iterStart) {
>+    if (*iterEnd == '/') {

So if your string ended with a char that was not '/', why is it ok to to
*iterEnd here?

>+      vanillaEnd.advance(-NS_STATIC_CAST(PRInt32, vanillaPageName.Length()));

What if this pushes vanillaEnd to before iterStart?

>+already_AddRefed<nsIURI> nsBaseImageAccessible::GetURIFromOwnerDoc()

>+  webNav->GetCurrentURI(&baseURI);

So are we getting a base URI or a current URI?

If you want a base URI for an image or link or something, you should be getting
it off the nsIContent or nsIDOM3Node for that element.	Using the webnavigation
URI there is wrong.

>+nsBaseImageAccessible::NewTextEquivFromURI(nsAString& aLinkURI,
>+  if (!securityManager ||
>+      securityManager->CheckLoadURI(baseURI, uri, kCheckFlags) == NS_ERROR_DOM_BAD_URI) {
>+    return nsnull;
>+  }

Please treat all error returns from CheckLoadURI as equally fatal.

Should you be doing an nsIContentPolicy check here?

>Index: accessible/src/base/nsBaseWidgetAccessible.h

>+  static nsTextEquivFromURI* NewTextEquivFromURI(nsAString& 
aLinkURI,

This needs to document that it allocates the object with new (and that it may
return null if the allocation fails).

>Index: accessible/src/base/nsTextEquivFromURI.cpp
>+    // XXX Is there a way to wait on setting up the parser until after
>+    //     we know the content type?

Yes, you could implement nsIStreamListener yourself, and set up the parser in
OnStartRequest, then just proxy over the notifications.

>+    mParser->SetContentSink(mSink);

You're passing in an XPCOM object with a refcount of 0?  Don't do that, please.

>+    mParser->Parse(aURI, mSink, PR_FALSE, nsnull, eDTDMode_quirks);

This is wrong.	It'll get the wrong text out of some documents...  Why,
exactly, is this being done?

>+    mChannel->AsyncOpen(streamListener, nsnull);

You want to set the priority before this call, no?

>+void nsTextEquivFromURI::Finish(PRBool aSuccess)
>+  mSink->Kill();
>+  mChannel = nsnull;
>+  mParser = nsnull;

What about nulling out mSink?

>+  if (!gParserService) {
>+    nsCOMPtr<nsIParserService> parserService =
>+      do_GetService("@mozilla.org/parser/parser-service;1");
>+    gParserService = parserService;

This is wrong.	gParserService needs to hold a ref to the service, and you need
to release it at shutdown.  As written, this code will crash if someone
restarts XPCOM, eg.

>+    NS_ASSERTION(gParserService, "Could not retrieve parser service");  

That can happen (either because there is no more memory, or for other reasons).
 It needs to be handled, not just asserted on.

>+ nsTextEquivContentSink::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)

>+    httpChannel->GetRequestSucceeded(&requestSucceeded);
>+    if (!requestSucceeded) {
>+      return NS_BINDING_ABORTED;
>+    }

No need to call Finish()?

>+ !contentType.Equals("application/x-unknown-content-type")) { // Often HTML

This should NOT be happening, for HTTP requests.  If it is, please point me to
a URL triggering this.	I don't know whether you want to parse random files
that we couldn't figure out the type for as HTML.  That sounds like a good way
to confuse the parser (consider image files).

>+NS_IMETHODIMP nsTextEquivContentSink::OpenContainer(const nsIParserNode& aNode)

>+    if (aTag >= eHTMLTag_h1 && aTag <= eHTMLTag_h6) {

I'd enumerate explicitly instead of assuming things about eHTMLTag ordering.

>+NS_IMETHODIMP nsTextEquivContentSink::CloseContainer(const nsHTMLTag aTag)
>+  if (aTag >= eHTMLTag_h1 && aTag <= eHTMLTag_h6 && 

Same.

>+PRBool nsTextEquivContentSink::MatchesXHTMLTag(const PRUnichar *aExpatName, 

Er... how would this be called, exactly?  You explicitly prevent expat ever
being invoked...

>+  const char* xhtmlNameSpace = "application/xml+xhtml";
>+  const PRUint32 xhtmlNameSpaceLen = 28;

NS_ARRAY_LENGTH, please?  That would mean making xhtmlNameSpace a char[], which
is fine.  Any  reason these are not static?

>Index: accessible/src/base/nsTextEquivFromURI.h
>+  nsTextEquivContentSink *mSink; // Weak ref, avoid circular references

What's the cycle you're avoiding, exactly?  I don't see one here...

>Index: accessible/src/html/nsHTMLImageAccessible.cpp
>+NS_IMETHODIMP nsHTMLImageAccessible::Init()

Again, what nodes can this be used for?  It should assert accordingly.

>+void nsHTMLImageAccessible::GetFallbackName(nsAString &aText)

>+  nsCOMPtr<nsIURI> baseURI = GetURIFromOwnerDoc();

Again, this gives the wrong base URI.

>Index: layout/base/nsIPresShell.h

>+class nsIAccessibilityService;

No ifdef needed?

>Index: layout/base/nsPresShell.cpp
>@@ -5857,10 +5880,19 @@ PresShell::HandleEventInternal(nsEvent* 
>+      static PRBool accessPrefsInited;

Why are we initing these in HandleEventInternal, exactly?  Why are we not
observing pref changes?  This is wrong for profile switches, for example.

>Index: layout/generic/nsImageFrame.cpp

>+#ifdef ACCESSIBILITY
>+  if (aPresContext->PresShell()->GetRepairingTextEquivalents() &&
>+      (!mContent->HasAttr(kNameSpaceID_None, nsHTMLAtoms::alt) ||
>+      mContent->HasAttr(kNameSpaceID_None, nsHTMLAtoms::usemap)) ||
>+      (aPresContext->PresShell()->GetRetrievingLongDescriptions() &&
>+      mContent->HasAttr(kNameSpaceID_None, nsHTMLAtoms::longdesc))) {

This is absolutely wrong for an nsImageFrame that's a frame for an <object>.

Perhaps some code in nsCSSFrameConstructor::CantRenderReplacedElement should be
factored out into this code?

Also, does the accessible hold a pointer to the frame or the content?  What
does it do if the frame is destroyed?

I'll wait for an r= before doing the next round of review....

	Aaron Leventhal Reporter
	Comment 33 • 20 years ago

Thanks for all the comments. Making a lot of changes based on them. Some questions:

>Index: accessible/src/base/nsAccessible.cpp

> Also, why are we treating <img> and <input type="image"> differently?
We don't want to try and follow a link around an <input> for alt text repair.
We only do that when there is an img inside of a link.

>>+  // Split the new URI into a left and right part
>>+  // (assume we're parsing it out right
> Please, please do not do this.  There are nsIURL methods that will get you what
> you want here, no?  Don't do your own URI parsing.
I agree there should be an nsIURI method for this, but there isn't.
I grabbed my parsing code from nsDocShell.cpp, which also needs this.

>>+nsAString& nsBaseImageAccessible::CoreOfString(nsString& aText)
>>+  while (iterEnd != iterStart) {
>>+    if (*iterEnd == '/') {
> So if your string ended with a char that was not '/', why is it ok to to
>*iterEnd here?
I don't understnad that question. What's wrong with *iterEnd?

More replies later. 

	Aaron Leventhal Reporter
	Comment 34 • 20 years ago

>>+ !contentType.Equals("application/x-unknown-content-type")) { // Often HTML

> This should NOT be happening, for HTTP requests.  If it is, please point me to
> a URL triggering this.  I don't know whether you want to parse random files
> that we couldn't figure out the type for as HTML.  That sounds like a good way
> to confuse the parser (consider image files).

We never parse the wrong file becuase I hvae a second check in WillBuildModel()
where I look at dtdType, which is always correct at that point. However, in
OnStartRequest() the content type  is still sometimes unknown.

I'll post a URI where it happening later.

> Er... how would this be called, exactly?  You explicitly prevent expat ever
> being invoked...
Where do I do that? Where I ask for eDTDMode_quirks? That's not preventing expat
 from being invoked. I've tested and used my nsIExpatSink impl already. I'd
actually prefer if I didn't need an nsIExpatSink impl and it would always use 
the html content sink for XHTML. That would make the code simpler.

> Also, does the accessible hold a pointer to the frame or the content?  What
> does it do if the frame is destroyed?
All accessibles hold onto nsCOMPtr<nsIDOMNode> mDOMNode
We listen for mutation events and pay attention to what documents are destroyed.
A Shutdown() method for an accessible any time it's node is being destroyed, so
that references like mDOMNode can be destroyed without destroying the entire
accessible. It needs to remain in memory in case the out-of-process client still
calls methods on it. However, it will return failures codes for methods called
on it at that point.

The only kind of accessible that holds onto a frame is an nsHTMLTextAccessible,
created for text nodes
nsHTMLTextAccessible.h:  nsIFrame *mFrame; // Only valid if node is not shut
down (mWeakShell != null)
Could that frame get destroyed without a mutation event occuring or the document
being destroyed?  That would be a problem at this point.

	Aaron Leventhal Reporter
	Comment 35 • 20 years ago

  >+ mParser->Parse(aURI, mSink, PR_FALSE, nsnull, eDTDMode_quirks);
  > This is wrong. It'll get the wrong text out of some documents... Why,
exactly, is this being done?

Which part is wrong, the eDTDMode_quirks or the Parse() call itself?

I don't know exactly what calling sequence I should be using to start things. Do
I not need this if I follow your advice about "you could implement
nsIStreamListener yourself, and set up the parser in OnStartRequest, then just
proxy over the notifications."

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 20 years ago

> I agree there should be an nsIURI method for this, but there isn't.

My comment said nsIURL, not nsIURI, no?  The docshell code is rather broken; see
the bug I have to remove it.

> I don't understnad that question. What's wrong with *iterEnd?

If iterEnd points past the end of the string (which it does in the case I
mentioned), it's something that's not in your string.

> where I look at dtdType, which is always correct at that point.

That depends on the parser getting the dtdType from the data, which may look
nothing like HTML...

> and it would always use the html content sink for XHTML.

The HTML sink can't parse XHTML correctly; it'd give bogus data back. My
question was based on your comment, which claimed that you didn't have to
implement nsIExpatSink, since you were passing a quirks mode.

> Could that frame get destroyed without a mutation event occuring or the
> document being destroyed? 

Absolutely.  Style changes could do it, for example, and not all of those are
accompanied by mutation events (preference changes come to mind, as does
enabling/disabling of stylesheets by the document).  I'm assuming you watch for
mutation events on attributes; if you don't then inline style changes will also
kill off frames without you noticing.

> Which part is wrong, the eDTDMode_quirks or the Parse() call itself?

The former.  That will parse documents wrong and give you bogus data in some cases.

> I don't know exactly what calling sequence I should be using to start things.

In general, this is fine.  You should be letting the parser autodetect the
parsemode, though.

	Aaron Leventhal Reporter
	Comment 38 • 20 years ago

Boris, holding onto the text frames indeed sees like a problem.
Is there any listener I can attach to listen for frames that are destroyed?

It's just an optimization for text frames that I have, because
GetPrimaryFrameFor() does a lot more calculation for text frames than what I do.
Doing GetPFF on text frames would increase the size of the primary frame map
since those are normally not stored there. I already havee the text frames as I
iterate through, and cahce those on the accessibles. I'd like to keep the
optimzation if possible.

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 20 years ago

You could set the NS_FRAME_EXTERNAL_REFERENCE bit on the framestate and add a
hook to PresShell::ClearFrameRefs that's #ifdef ACCESSIBILITY.

	Aaron Leventhal Reporter
	Comment 40 • 20 years ago

> Should you be doing an nsIContentPolicy check here?

Do I need to provide my own implementation of nsIContentPolicy to prevent it
from loading anything other than HTML? Or should the mime type check be enough?

	Aaron Leventhal Reporter
	Comment 41 • 20 years ago

(In reply to comment #39)
> You could set the NS_FRAME_EXTERNAL_REFERENCE bit on the framestate and add a
> hook to PresShell::ClearFrameRefs that's #ifdef ACCESSIBILITY.

Filed bug 280498 on that issue. It's been wrong for a while, but is really
separate from this bug since that problem is not an image accessible problem.

	Aaron Leventhal Reporter
	Comment 42 • 20 years ago

(In reply to comment #32)
> Why are we initing these in HandleEventInternal, exactly?  Why are we not
> observing pref changes?  This is wrong for profile switches, for example.

Eventually the pref will go away and we will always use this code.
This is just some quick code so that testers can turn it on while it's still
experimental.

What gets shut down when the profile is switched? There is no XPCOM or sevices
shutdown or anything right?

	Boris Zbarsky [:bzbarsky]
	Comment 43 • 20 years ago

> Do I need to provide my own implementation of nsIContentPolicy

Probably not, but I was wondering whether this code should load things that are
blocked by whatever content policy modules are loaded...  I'm not sure, hence
the question.

> What gets shut down when the profile is switched?

I'm not completely sure, actually.  Certainly preferences get reset....  I don't
think anything is really shut down, though.

> There is no XPCOM or sevices shutdown or anything right?

Right.  That can happen for other reasons, though.

	timeless
	Comment 44 • 20 years ago

iirc networking is supposed to go away temporarily :)

	Aaron Leventhal Reporter
	Comment 45 • 18 years ago

It's going to be hard to merge this patch to trunk, but if get the chance to, have text equiv repair would be great. I suggest we expose an object attribute called "name-guess".

	Aaron Leventhal Reporter
	Comment 46 • 17 years ago

The name-guess attribute would just be "true" if the accessible name was from the repair operation.

	Aaron Leventhal Reporter
	Comment 47 • 17 years ago

When a PNG image is used we should look for the accessible name as a title inside the file:
See http://www.vias.org/pngguide/chapter11_04.html
In this case it's not a guess -- the author of the .png put the text directly in the image. I don't know how common that is.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 48 • 9 years ago

Last activity was 2009-08-22. For practical reasons I'm closing as WONTFIX.