Steps to reproduce: 1. Launch Thunderbird and Accerciser 2. In Thunderbird, select a mail folder with unread messages whose unread count is displayed in the folder lists, e.g. "my folder (3)" 3. In Accerciser's list of objects (left-hand pane), locate the folder chosen in step 2. And note its accessible name. ("my folder (3)") 4. In Thunderbird, mark one of the unread messages read. This should cause Thunderbird to display an updated count ("my folder (2)"). 5. Examine the accessible name of the item chosen in step 3. Its name will also be automatically updated ("my folder (2)"). 6. Repeat steps 4 and 5 until there are no more read messages in the chosen folder. Expected results: When the unread count has reached 0, the accessible name of the object would match the what is displayed by Thunderbird, i.e. both would be "my folder". Actual results: The accessible name is updated correctly *until* the last unread message is marked as read. At that point, the item displayed by Thunderbird lacks a count ("my folder"); but the accessible name continues to include a count of 1 ("my folder (1)"). Impact: Orca presents the "(1)" that is no longer displayed by Thunderbird. Note: If this were an AT-SPI2 caching issue, either of the following should cause the accessible name to be updated: a. Clearing the cache of the accessible (acc.clearCache() in the iPython console), OR b. Quitting and restarting Accerciser, and relocating the accessible object corresponding with the mail folder. However, neither of those solutions works. As a result, Orca cannot do anything to address this on its end. The only thing that causes the problem to go away is quitting and restarting Thunderbird.

This issue is caused by the fix for Bug 1203861, which leads to not updating the accessible name if it has the same prefix as the current one. Fix that by comparing the whole names as C strings, rather than only the prefix. This maintains the fix for Bug 1203861, and fixes this one.

Comment on attachment 8831199 [details] [diff] [review] Compare the whole accessible name when checking equality > MaybeFireNameChange(AtkObject* aAtkObj, const nsString& aNewName) > { > NS_ConvertUTF16toUTF8 newNameUTF8(aNewName); >- if (aAtkObj->name && >- !strncmp(aAtkObj->name, newNameUTF8.get(), newNameUTF8.Length())) >+ if (aAtkObj->name && !strcmp(aAtkObj->name, newNameUTF8.get())) I guess this works, but I'm not sure get() is required to return a pointer to a null terminated string. So I'd prefer you used strlen() on atkObj->name as the length. That is I'm not sure using strdup instead of strndup is actually safe, but that's sort of a different issue.

also sorry about the lag here.

(In reply to Trevor Saunders (:tbsaunde) from comment #2) > > MaybeFireNameChange(AtkObject* aAtkObj, const nsString& aNewName) > > { > > NS_ConvertUTF16toUTF8 newNameUTF8(aNewName); > >- if (aAtkObj->name && > >- !strncmp(aAtkObj->name, newNameUTF8.get(), newNameUTF8.Length())) > >+ if (aAtkObj->name && !strcmp(aAtkObj->name, newNameUTF8.get())) > > I guess this works, but I'm not sure get() is required to return a pointer > to a null terminated string. As mentioned in the header, this is at least not buggier than the code below it, which requires a nul-terminated string from get(). > So I'd prefer you used strlen() on atkObj->name as the length. That wouldn't work, it would have the same problem, but when the name grows -- and wouldn't prevent access past the end of the get() result if strlen(aAtkObj->name) > newNameUTF8.Length(). To properly use the length, if I'm not mistaken we'd have to do something like that: if (aAtkObj->name && strlen(aAtkObj->name) <= newNameUTF8.Length() && !strncmp(aAtkObj->name, newNameUTF8.get(), newNameUTF8.Length()) so the strncmp() check never catches an obviously shorter string. No tested, though.

(In reply to Colomban Wendling from comment #4) > (In reply to Trevor Saunders (:tbsaunde) from comment #2) > > > MaybeFireNameChange(AtkObject* aAtkObj, const nsString& aNewName) > > > { > > > NS_ConvertUTF16toUTF8 newNameUTF8(aNewName); > > >- if (aAtkObj->name && > > >- !strncmp(aAtkObj->name, newNameUTF8.get(), newNameUTF8.Length())) > > >+ if (aAtkObj->name && !strcmp(aAtkObj->name, newNameUTF8.get())) > > > > I guess this works, but I'm not sure get() is required to return a pointer > > to a null terminated string. > > As mentioned in the header, this is at least not buggier than the code below > it, which requires a nul-terminated string from get(). That was what was implied in what I said, and I gave you an r+ so you are free to commit it, or set checkin-needed as a keyword. > > So I'd prefer you used strlen() on atkObj->name as the length. > > That wouldn't work, it would have the same problem, but when the name grows > -- and wouldn't prevent access past the end of the get() result if > strlen(aAtkObj->name) > newNameUTF8.Length(). To properly use the length, if > I'm not mistaken we'd have to do something like that: erg, yeah. > if (aAtkObj->name && > strlen(aAtkObj->name) <= newNameUTF8.Length() && > !strncmp(aAtkObj->name, newNameUTF8.get(), newNameUTF8.Length()) > > so the strncmp() check never catches an obviously shorter string. No > tested, though. I think that's correct, but don't particularly trust myself here.

(In reply to Trevor Saunders (:tbsaunde) from comment #5) > That was what was implied in what I said, and I gave you an r+ so you are > free to commit it, or set checkin-needed as a keyword. Okay, thanks :) > I think that's correct, but don't particularly trust myself here. Me neither at 100%, I'd have to wrap my head around that and make some tests to be sure. But I'll stick to the current patch for now; if it ever gets required to support unterminated strings something like the above could still be used after making sure it's actually correct.

Hi, I'm the Thunderbird sheriff and I can't check this in since the patch applies to M-C. Please move this to the appropriate module in Core. Clearing checkin-needed for now.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13d5c32e4a61 Compare the whole accessible name when checking equality. r=tbsaunde

Hello all, Could it be possible to include this patch on Thunderbird 52 branch? It is a really bad behavior for a blind user not to know if it has again unread messages or not. Best regards.

Comment on attachment 8831199 [details] [diff] [review] Compare the whole accessible name when checking equality Any chance to get this uplifted to mozilla-esr52, it fixes accessibility issues in Thunderbird. It doesn't look like a risky change and hasn't caused regressions so far. If not, I can include it in our Thunderbird release branch. [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: See above. User impact if declined: Accessibility issues in Thunderbird Fix Landed on Version: mozilla54 Risk to taking this patch (and alternatives if risky): Not risky. String or UUID changes made by this patch: None.

Comment on attachment 8831199 [details] [diff] [review] Compare the whole accessible name when checking equality Looks like a low risk change and fix an accessibility issue. Let's uplift it to ESR52.3.

Thanks!