Failure to update accessible name when last unread mail is marked as read

RESOLVED FIXED in Firefox -esr52

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jdiggs, Assigned: cwendling)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
All
Linux
Points:
---

Firefox Tracking Flags

(firefox-esr52 fixed, firefox54 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8831199 [details] [diff] [review]
Compare the whole accessible name when checking equality

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.
(Assignee)

Updated

2 years ago
Attachment #8831199 - Flags: review?(tbsaunde+mozbugs)
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.
Attachment #8831199 - Flags: review?(tbsaunde+mozbugs) → review+
also sorry about the lag here.
(Assignee)

Comment 4

2 years ago
(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.
Assignee: nobody → cwendling
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 6

2 years ago
(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.

Comment 7

2 years ago
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.
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(cwendling)
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Component: Disability Access → Disability Access APIs
Flags: needinfo?(cwendling)
Keywords: checkin-needed
Product: Thunderbird → Core
Version: unspecified → Trunk

Updated

2 years ago
Flags: needinfo?(tbsaunde+mozbugs)

Comment 8

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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/13d5c32e4a61
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 10

a year ago
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.
Flags: needinfo?(jorgk)
Duplicate of this bug: 1332877

Comment 12

a year ago
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.
Attachment #8831199 - Flags: approval-mozilla-esr52?
status-firefox-esr52: --- → affected
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.
Attachment #8831199 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 14

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/48a89721d076
status-firefox-esr52: affected → fixed

Comment 15

a year ago
Thanks!

Updated

a year ago
Flags: needinfo?(jorgk)
You need to log in before you can comment on or make changes to this bug.