1775245 - tree-listbox should have ARIA role of "tree" for addressbook and account manager

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Henry Wilkes (they/them) [:henry-x]]

The addressbook tree and account manager trees should have role="tree".

Rather than wait for Bug 1752532, we can modify the existing tree-listbox class to use the correct roles for 102

Comment 1

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1775245 - Use ARIA role of "tree" for the addressbook and account manager trees. r=aleca,darktrojan

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2b88e08e67f3
Use ARIA role of "tree" for the addressbook and account manager trees. r=aleca,darktrojan

Comment 3

[Geoff Lankow (:darktrojan)]

Somehow you've broken browser_calendarList.js. Failure log.

From a quick glance, it seems like the selectedIndex is 0 when it should be 1 after this delete and the wrong calendar gets deleted.

Comment 4

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Geoff Lankow (:darktrojan) from comment #3)

Somehow you've broken browser_calendarList.js. Failure log.

I tracked it down to the added whitespace here https://hg.mozilla.org/comm-central/rev/2b88e08e67f3#l1.14

This extra whitespace was acting as a MutationRecord.previousSibling here https://searchfox.org/comm-central/rev/232c80b88b0dcd6c874ff0d754215df78e4f020e/mail/base/content/widgets/tree-listbox.js#292

That code is too fragile. I'll try and strengthen it rather than just remove the whitespace.

I also noticed that the current implementation tries to select the previous index https://searchfox.org/comm-central/rev/232c80b88b0dcd6c874ff0d754215df78e4f020e/mail/base/content/widgets/tree-listbox.js#275 but the convention is to select the same index. I can fix that here as well.

Also, looking through the code, this line is setting the selectedIndex when a calendar is about to be removed https://searchfox.org/comm-central/rev/232c80b88b0dcd6c874ff0d754215df78e4f020e/calendar/base/content/calendar-management.js#345 rather than just letting the widget handle this. @darktrojan Do you know why it is changing the selection?

Comment 5

[Geoff Lankow (:darktrojan)]

Probably because that code is much older than the widget it now controls. If it's getting in the way, change it.

Comment 6

[Geoff Lankow (:darktrojan)]

I'm going to remove the offending white space to clear the failing test.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5f7e28323e23
follow-up - Remove some white-space causing a test to fail. rs=bustage-fix

Comment 8

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1775245 - Refactor tree-listbox selection code when selected item is removed. r=darktrojan

The convention from xul:tree and xul:listbox is to select the next item that was not deleted. We also simplify the logic by using selectedRow (an Element) rather than selectedIndex.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/73636ef4138a
Refactor tree-listbox selection code when selected item is removed. r=darktrojan

Comment 10

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9282195 [details]
Bug 1775245 - Use ARIA role of "tree" for the addressbook and account manager trees. r=aleca,darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Wrong role announced by screen reader, and confusing presence of listboxes within listboxes.
Testing completed (on c-c, etc.): Tested on Daily with Orca
Risk to taking this patch (and alternatives if risky): Medium. This does some refactoring.

NOTE: Also want revision 5f7e28323e23 (see comment 6)

Comment 11

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9284730 [details]
Bug 1775245 - Refactor tree-listbox selection code when selected item is removed. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Previous code was fragile and would break under certain conditions (e.g. whitespace, or removing two items). Plus, the behaviour when deleting an item is not consistent with what is planned in bug 1752532
Testing completed (on c-c, etc.): Passed try-comm-central tests.
Risk to taking this patch (and alternatives if risky): Medium. Required refactoring.

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9282195 [details]
Bug 1775245 - Use ARIA role of "tree" for the addressbook and account manager trees. r=aleca,darktrojan

[Triage Comment]
Next week is the merge, making beta 104. So an uplift to beta won't be happening.

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9284730 [details]
Bug 1775245 - Refactor tree-listbox selection code when selected item is removed. r=darktrojan

[Triage Comment]

Comment 14

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9282195 [details]
Bug 1775245 - Use ARIA role of "tree" for the addressbook and account manager trees. r=aleca,darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Wrong role announced by screen reader, and confusing presence of listboxes within listboxes.
Testing completed (on c-c, etc.): Tested with Orca
Risk to taking this patch (and alternatives if risky): Medium. This does some refactoring.

NOTE: Also want revision 5f7e28323e23 (see comment 6)

Comment 15

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9284730 [details]
Bug 1775245 - Refactor tree-listbox selection code when selected item is removed. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Previous code was fragile and would break under certain conditions (e.g. whitespace, or removing two items). Plus, the behaviour when deleting an item is not consistent with what is planned in bug 1752532
Testing completed (on c-c, etc.): Passed try-comm-central tests.
Risk to taking this patch (and alternatives if risky): Medium. Required refactoring.

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9284730 [details]
Bug 1775245 - Refactor tree-listbox selection code when selected item is removed. r=darktrojan

[Triage Comment]
Approved for esr102

extra smoke testing of 102.1.1 would be good for address book

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9282195 [details]
Bug 1775245 - Use ARIA role of "tree" for the addressbook and account manager trees. r=aleca,darktrojan

[Triage Comment]
Approved for esr102

Comment 18

[Arthur K. (he/him)]

Was there a reason I was needinfo'ed?

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 102.1.1:
https://hg.mozilla.org/releases/comm-esr102/rev/5c73f9843d96
https://hg.mozilla.org/releases/comm-esr102/rev/591151807915
https://hg.mozilla.org/releases/comm-esr102/rev/690bd5e094e5

Comment 20

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #16)

extra smoke testing of 102.1.1 would be good for address book

I actually did smoketesting of 102.1.1 two days after this needinfo request, tested AB, and saw a bug, see my email message to beta testers dated 2022-08-05, 21:26 CEST:

SUCCESS

• menus
• setup gmx-imap, connect Carddav-AB, caldav-cal
• setup gmail-imap, get messages,
• read and reply/reply-all,
• open attachments (incl. PDF), save attachments, drag attachments out to OS folder,
• compose with attachments, drag attachments into composition, including inline image
• Calendar: CALDAV event creation and edit,
• Address Book: CardDAV card creation and edit

After adding a birthday 29-Feb without year to a CardDAV card, it disappeared from card display and on next edit, not found.

This works for me now on 102.1.2 (64-bit), Win10 - we've fixed birthdays without year in Bug 1779100 - Re-allow entering a birthday or special date without year (avoid ux-interruption, also for migrated contacts w/o year).

Today I've tested the behaviour of deleting contacts on 102.1.2 (64-bit), Win10 and as a result, filed
Bug 1785314 - After deleting a contact, first contact in list gets selected and search box is focused
(although maybe this bug was only affecting deleting address books?)