Closed Bug 1840295 Opened 1 year ago Closed 1 year ago

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

Categories

(Core :: Disability Access APIs, task)

task

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
firefox117 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ctw-postship])

Attachments

(1 file)

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.

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?

Depends on: 1831035
Flags: needinfo?(eitan)

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

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.

Flags: needinfo?(eitan)

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

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.

Assignee: nobody → jteh
Status: NEW → ASSIGNED
Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7e02bf68fd17 Remove Windows specific ifdefs for PDocAccessible:: FocusEvent and CaretMoveEvent. r=eeejay
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: