1840295 - [CTW] Get rid of the Windows ifdefs for PDocAccessible CaretMoveEvent and FocusEvent

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[James Teh [:Jamie]]

After bug 1831035, PDocAccessible will be unified across platforms. However, there are still some ifdefs. Two of these are FocusEvent and CaretMoveEvent. This is because on Windows, these events need to push the caret rectangle. The caret rectangle isn't used on any other platform.

There are a few paths forward:

1. Do nothing; wontfix this bug. An ifdef is a bit ugly, but probably not the end of the world.
2. Make the FocusEvent ifdef more like the CaretMoveEvent ifdef; i.e. just ifdef the caret rect parameter. That way, we at least don't have two separate functions handling focus events on different platforms: PDocAccesible::FocusEvent on Windows and PDocAccessible::Event everywhere else.
3. Get rid of the ifdef almost everywhere except where we would pass 0s for the caret rect on non-Windows. This avoids calculating the caret rect where it won't be used, but makes most of the code cleaner.
4. Just push the caret rect on all platforms. This unifies everything across platforms, at the cost of something that won't ever get used on non-Windows.

Comment 1

[James Teh [:Jamie]]

Eitan, I think these ifdefs bother you more than they do me. Do you have any preference as to which option we go with here?

Comment 2

[James Teh [:Jamie]]

Personally, I'd probably prefer option 3 or 4. 3 avoids pointless computation, but that might be premature optimisation.

Comment 3

[Eitan Isaacson [:eeejay]]

Can't we just pass an empty rect to CaretMoveEvent on non-windows instead of having a different protocol? I would like us to have the same protocol everywhere, but maybe utilize it differently on different platforms to avoid expensive things.

Honestly, I don't have strong opinions here and can go whatever way you choose since you are the great unifier. I think we both share the goal of simplifying this stuff and de-fragmenting it, so whatever path you choose is great.

We really just need to pick some balance with regard to simple code base, unified test outcomes across platforms, and not spending extra resources on things that aren't used in other platforms. A thing that comes to mind is the EVENT_SCROLLING event that is not consumed on anything but android and is probably wasteful. The flip side is that it is really nice to have good browser tests that run in desktop platforms for that.

Comment 4

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #3)

Can't we just pass an empty rect to CaretMoveEvent on non-windows instead of having a different protocol? I would like us to have the same protocol everywhere, but maybe utilize it differently on different platforms to avoid expensive things.

Yup, that's what I meant by option 3. Sorry if that wasn't clear. Sounds like we're both on the same page here; just wanted a gut check.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1840295: Remove Windows specific ifdefs for PDocAccessible:: FocusEvent and CaretMoveEvent.

On Windows, focus and caret move events include the caret rectangle.
This isn't used on other platforms.
To simplify the cross-platform interface (including Platform.h), remove the ifdefs from there.
However, we use ifdefs to avoid calculating the rectangle on non-Windows platforms, instead just sending an empty rectangle.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e02bf68fd17
Remove Windows specific ifdefs for PDocAccessible:: FocusEvent and CaretMoveEvent. r=eeejay

Comment 7

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/7e02bf68fd17