GNOME Bugzilla – Bug 584540
Orca should not duplicate images and lines of text on facebook
Last modified: 2009-11-09 21:35:14 UTC
1. log into a facebook with several headings of status. I think it helps if your friends have pictures. 2. Arrow down through each persons status. Notice that each person has an image-link and a standard link. I'm noticing that for each person the image link is rendered twice by orca. I also notice that if the comment has several lines the last line in the comment is reproduced twice.
Also this happens with orca from trunk and firefox 3.1 beta 3.
Will and I did some bartering. I'm taking this one.
Created attachment 138259 [details] [review] revision 1 The problem is that Facebook has included multiple images which we are in turn presenting. That said, this removes the extraneous image. <cough>crappy markup</cough> :-) I suspect this patch is sound, but I have three hours worth of regression tests to run. Stay tuned for the review request. :-)
Passes the regression tests. Will please review. Thanks!
Something makes me nervous about this patch -- it seems like it might end up with false positives. However, I could be way wrong. Is it that this piece of code knows what's going to happen in speech_generator.py:_generateName and thus avoids that?
I'm not sure I follow you. This change is in isUselessObject (which is called by isLayoutOnly). Right now (i.e. without the patch) the behavior is this: If it is an image with no useful information associated with it (no alternative text, no title), isUselessObject returns True. (And thus the offending image is not included by getUtterancesFromContents - unless it is the only thing present in the contents). BUT isUselessObject does one last check to see if this otherwise useless item happens to have an ancestor of role link in which case it declares the image of use after all. That is the source of this bug. The premise in the patch is that if a completely useless image inside of a link has siblings, one of those siblings (be it another image, or text) hopefully has the meaning associated with the link, in which case the image which lacks meaning can be safely ignored. If it's the lone image inside of the link, the behavior is what we've been doing (i.e. declaring it of use after all). If the image had any alternative text or a title, we wouldn't ignore it because it never would have been declared potentially useless in the first place. I *believe* we want to catch this condition before we ever get to a generator (speech or braille). As I understand it, we don't want to present this thing (name, role, whathaveyou) at all. (Right?) All of this said, if you could elaborate more on your concerns, hopefully I will see where I may have gone wrong. Thanks!
OK - thanks for the more detailed explanation. I say commit. :-)
Heheh. Because of your comments, I just wrote a new regression tests with all sorts of garbage inside links (checked in) and ran it using my patch to be sure. It turns out there's a case I wasn't handling. So let me handle that and spin another patch. Sorry!!
Created attachment 138287 [details] [review] revision 2 Ok, this handles all sorts of cases -- including a new one I just thought of and added to the regression test. I'll re-run the regression tests now, but given that they take 3 hours.... Will, if you happen to be around, please review this version. If there's more cases/things I should be doing and/or doing differently, no point in letting the tests complete. Thanks!
All regression tests passed.
(In reply to comment #9) > Ok, this handles all sorts of cases -- including a new one I just thought of > and added to the regression test. I'll re-run the regression tests now, but > given that they take 3 hours.... Will, if you happen to be around, please > review this version. If there's more cases/things I should be doing and/or > doing differently, no point in letting the tests complete. Thanks! I think this is fine, though it does seem to be getting kind of elaborate to handle, ahem, "creative" markup. However, we gotta do what we gotta do.
Thanks Will. Patch committed to master. Mike, I am not seeing the duplicate lines of comment text you are describing. However, the other day I checked in a patch which solved a bug we had missing text on a page. As a side effect, one of the regression tests where we were duplicating text stopped duplicating it. So I'm hoping that that fix is responsible for solving your duplicate comment text problem. As such I'm going to close this bug as FIXED. If you are still seeing duplicate comment text, please open a new bug just for that issue and include a debug.out. Thanks!