Closed Bug 1775245 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: General, defect)

defect

Tracking

(thunderbird_esr102 fixed, thunderbird102 affected)

RESOLVED FIXED
104 Branch
Tracking Status
thunderbird_esr102 --- fixed
thunderbird102 --- affected

People

(Reporter: henry-x, Assigned: henry-x)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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

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

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 104 Branch

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.

Status: RESOLVED → REOPENED
Flags: needinfo?(henry)
Resolution: FIXED → ---

(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?

Flags: needinfo?(henry) → needinfo?(geoff)

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

Flags: needinfo?(geoff)

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

Keywords: leave-open
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/5f7e28323e23 follow-up - Remove some white-space causing a test to fail. rs=bustage-fix

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.

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

Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED

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)

Attachment #9282195 - Flags: approval-comm-beta?

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.

Attachment #9284730 - Flags: approval-comm-beta?

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.

Attachment #9282195 - Flags: approval-comm-beta? → approval-comm-beta-

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

[Triage Comment]

Attachment #9284730 - Flags: approval-comm-beta? → approval-comm-beta-

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)

Attachment #9282195 - Flags: approval-comm-esr102?

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.

Attachment #9284730 - Flags: approval-comm-esr102?

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

Flags: needinfo?(thee.chicago.wolf)
Flags: needinfo?(bugzilla2007)
Attachment #9284730 - Flags: approval-comm-esr102? → approval-comm-esr102+

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

Attachment #9282195 - Flags: approval-comm-esr102? → approval-comm-esr102+

Was there a reason I was needinfo'ed?

Flags: needinfo?(thee.chicago.wolf)

(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?)

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

Attachment

General

Created:
Updated:
Size: