1976137 - Clean up caret rect code

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[James Teh [:Jamie]]

In bug 1966812, GetCaretRect is being unified into HyperTextAccessibleBase. That means we can clean up a bunch of things:

1. On Windows, there are different versions of AccessibleWrap::UpdateSystemCaretFor for local and remote. These can be unified into a single function which uses HyperTextAccessibleBase::GetCaretRect. It might even make sense to just move this into Platform.cpp.
2. PlatformFocusEvent and PlatformCaretEvent currently take a caret rect. For LocalAccessible, this is empty on all platforms. We should just get rid of this argument and have platforms call GetCaretRect if they need it.
3. DocAccessibleChild::GetCaretRectFor should take a LocalAccessible, rather than an id. Calling ID() is a lot nicer than reinterpret_cast. This will require an equivalent change to SendFocusEvent and SendCaretMoveEvent.

Comment 1

[James Teh [:Jamie]]

If bug 1966812 doesn't do this, we should also probably make GetCaretRect return `std::pair<mozilla::LayoutDeviceIntRect, nsIWidget*> instead of taking nsIWidget as an out parameter.

Comment 2

[James Teh [:Jamie]]

Attachment: Bug 1976137 part 1: Unify Windows AccessibleWrap::UpdateSystemCaretFor.

Previously, we had separate versions for LocalAccessible and RemoteAccessible.
Now that GetCaretRect is unified (bug 1966812), we can unify these into a single function.

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 1976137 part 2: Move UpdateSystemCaretFor to Platform.cpp.

This is only in AccessibleWrap for vestigial reasons.
These days, it has nothing to do with AccessibleWrap at all.
This just moves the code with no changes.

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1976137 part 3: Remove aCaretRect argument from PlatformCaretMoveEvent and PlatformFocusEvent.

This argument was provided inconsistently.
it wasn't even used on Windows for LocalAccessible because we need the nsIWidget there.
Now that we have unified GetCaretRect, it isn't necessary, so just get rid of it.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1976137 part 4: Remove DocAccessibleChild overrides of SendCaretMoveEvent and SendFocusEvent.

Instead of overriding these with intermediate functions that get and pass the caret rect, just get and pass it directly.
This avoids indirection and thus makes the code easier to follow and reason about.
I think this indirection was originally added because the need for the caret rect was previously Windows specific, but it isn't any longer.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/05970d567b06
https://hg.mozilla.org/integration/autoland/rev/23c510f72594
part 1: Unify Windows AccessibleWrap::UpdateSystemCaretFor. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/21f1cfda8680
https://hg.mozilla.org/integration/autoland/rev/0810e9c1e06a
part 2: Move UpdateSystemCaretFor to Platform.cpp. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/b46aa6464fce
https://hg.mozilla.org/integration/autoland/rev/e4f2e7c8959d
part 3: Remove aCaretRect argument from PlatformCaretMoveEvent and PlatformFocusEvent. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/102a35ed28b8
https://hg.mozilla.org/integration/autoland/rev/66b4c64987b9
part 4: Remove DocAccessibleChild overrides of SendCaretMoveEvent and SendFocusEvent. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/a2bbd0d67239
https://hg.mozilla.org/integration/autoland/rev/b41ceaa0a5c9
apply code formatting via Lando

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ee3a4d446486
https://hg.mozilla.org/integration/autoland/rev/8b0d004a4d3f
Revert "Bug 1976137: apply code formatting via Lando" for causing build bustages @ Platform.cpp

Comment 8

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages @ Platform.cpp

Backout link: https://hg-edge.mozilla.org/integration/autoland/rev/8b0d004a4d3f03394d1e7072f95d95a65415a0dc

Push with failures

Failure log -> ERROR - /builds/worker/checkouts/gecko/accessible/windows/msaa/Platform.cpp(30,26): error: expected namespace name

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/13d40b1cb891
https://hg.mozilla.org/integration/autoland/rev/0b6a16b23cdd
part 1: Unify Windows AccessibleWrap::UpdateSystemCaretFor. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/af4dba5315ad
https://hg.mozilla.org/integration/autoland/rev/569c9a52612c
part 2: Move UpdateSystemCaretFor to Platform.cpp. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/75b13390f66b
https://hg.mozilla.org/integration/autoland/rev/c86d497de229
part 3: Remove aCaretRect argument from PlatformCaretMoveEvent and PlatformFocusEvent. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/b2bab0d15249
https://hg.mozilla.org/integration/autoland/rev/ab5d9681e738
part 4: Remove DocAccessibleChild overrides of SendCaretMoveEvent and SendFocusEvent. r=eeejay
https://github.com/mozilla-firefox/firefox/commit/7a96815c3ece
https://hg.mozilla.org/integration/autoland/rev/e70925dcdcd7
apply code formatting via Lando

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0b6a16b23cdd
https://hg.mozilla.org/mozilla-central/rev/569c9a52612c
https://hg.mozilla.org/mozilla-central/rev/c86d497de229
https://hg.mozilla.org/mozilla-central/rev/ab5d9681e738
https://hg.mozilla.org/mozilla-central/rev/e70925dcdcd7

Comment 11

[James Teh [:Jamie]]

Attachment: Bug 1976137 part 1: Unify Windows AccessibleWrap::UpdateSystemCaretFor.

Previously, we had separate versions for LocalAccessible and RemoteAccessible.
Now that GetCaretRect is unified (bug 1966812), we can unify these into a single function.

Original Revision: https://phabricator.services.mozilla.com/D256563

Comment 12

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined: Breaks text entry with Windows Magnifier (bug 1984045).
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not applicable
• Risk associated with taking this patch: low
• Explanation of risk level: Baked for almost a full Nightly cycle. Changes Windows specific code to use a cross-platform function which is well covered by automated tests.
• String changes made/needed: none
• Is Android affected?: no

Comment 13

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/d5a2bfd397bf
https://hg.mozilla.org/releases/mozilla-release/rev/e8c65ddc210e