Bug 224549 – Move FocusRemovalEventsMode into FocusOptions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Manuel Rego Casasnovas]

Move FocusRemovalEventsMode into FocusOptions

[Manuel Rego Casasnovas]

Created attachment 425984 [details]
Patch

[Manuel Rego Casasnovas]

Created attachment 426041 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 426041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426041&action=review

> Source/WebCore/dom/Document.cpp:4363
> +        FocusOptions focusOptions = { };
> +        focusOptions.removalEventsMode = FocusRemovalEventsMode::DoNotDispatch;
> +        setFocusedElement(nullptr, focusOptions);

I've initially used the following line here, but it doesn't build on Windows port due to the C++ version:
      setFocusedElement(nullptr, { .removalEventsMode = FocusRemovalEventsMode::DoNotDispatch });

[Darin Adler]

Comment on attachment 426041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426041&action=review

>> Source/WebCore/dom/Document.cpp:4363
>> +        setFocusedElement(nullptr, focusOptions);
> 
> I've initially used the following line here, but it doesn't build on Windows port due to the C++ version:
>       setFocusedElement(nullptr, { .removalEventsMode = FocusRemovalEventsMode::DoNotDispatch });

Yes, I think that might be a C++20 feature and we can’t start using those yet. But soon I hope!

But maybe you could write something like this:

    setFocusedElement(nullptr, { { }, { }, { FocusRemovalEventsMode::DoNotDispatch }, { } });

While it’s not super-beautiful, there’s no chance of the FocusRemovalEventsMode ending up in the wrong structure element, nor of anything else being set to a non-default value.

> Source/WebCore/dom/FocusRemovalEventsMode.h:30
> +enum class FocusRemovalEventsMode { Dispatch, DoNotDispatch };

Do we really need a separate header for this? Can we just put this into FocusOptions.h and include that everywhere this is needed?

I’m willing to have a separate header for a single enumeration if it’s really helpful but it would be good to avoid if possible.

[Manuel Rego Casasnovas]

Created attachment 426079 [details]
Patch for landing

[Manuel Rego Casasnovas]

Thanks for the quick review.

(In reply to Darin Adler from comment #4)
> Comment on attachment 426041 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426041&action=review
> 
> >> Source/WebCore/dom/Document.cpp:4363
> >> +        setFocusedElement(nullptr, focusOptions);
> > 
> > I've initially used the following line here, but it doesn't build on Windows port due to the C++ version:
> >       setFocusedElement(nullptr, { .removalEventsMode = FocusRemovalEventsMode::DoNotDispatch });
> 
> Yes, I think that might be a C++20 feature and we can’t start using those
> yet. But soon I hope!
> 
> But maybe you could write something like this:
> 
>     setFocusedElement(nullptr, { { }, { }, {
> FocusRemovalEventsMode::DoNotDispatch }, { } });
> 
> While it’s not super-beautiful, there’s no chance of the
> FocusRemovalEventsMode ending up in the wrong structure element, nor of
> anything else being set to a non-default value.

Yeah that works, thanks for the suggestion.

> > Source/WebCore/dom/FocusRemovalEventsMode.h:30
> > +enum class FocusRemovalEventsMode { Dispatch, DoNotDispatch };
> 
> Do we really need a separate header for this? Can we just put this into
> FocusOptions.h and include that everywhere this is needed?
> 
> I’m willing to have a separate header for a single enumeration if it’s
> really helpful but it would be good to avoid if possible.

Yeah I don't think it's needed, I've merged it into FocusOptions (I was mimicking what is done for FocusDirection, but maybe that's separated just because it's older than FocusOptions).

[Manuel Rego Casasnovas]

Created attachment 426081 [details]
Patch for landing

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #6)
> Thanks for the quick review.
> 
> (In reply to Darin Adler from comment #4)
> > Comment on attachment 426041 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=426041&action=review
> > 
> > >> Source/WebCore/dom/Document.cpp:4363
> > >> +        setFocusedElement(nullptr, focusOptions);
> > > 
> > > I've initially used the following line here, but it doesn't build on Windows port due to the C++ version:
> > >       setFocusedElement(nullptr, { .removalEventsMode = FocusRemovalEventsMode::DoNotDispatch });
> > 
> > Yes, I think that might be a C++20 feature and we can’t start using those
> > yet. But soon I hope!
> > 
> > But maybe you could write something like this:
> > 
> >     setFocusedElement(nullptr, { { }, { }, {
> > FocusRemovalEventsMode::DoNotDispatch }, { } });
> > 
> > While it’s not super-beautiful, there’s no chance of the
> > FocusRemovalEventsMode ending up in the wrong structure element, nor of
> > anything else being set to a non-default value.
> 
> Yeah that works, thanks for the suggestion.

Actually that only worked on Linux ports, not on Mac. So I'm coming back to the code in the first patch.

[Manuel Rego Casasnovas]

Created attachment 426089 [details]
Patch for landing

[EWS]

Committed r276016 (236568@main): <https://commits.webkit.org/236568@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426089 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76695885>

[Darin Adler]

(In reply to Manuel Rego Casasnovas from comment #8)
> Actually that only worked on Linux ports, not on Mac. So I'm coming back to
> the code in the first patch.

Would eventually like to know what didn’t work on Mac about this. Not important, but curious. I suppose I can try it myself.

[Manuel Rego Casasnovas]

(In reply to Darin Adler from comment #12)
> (In reply to Manuel Rego Casasnovas from comment #8)
> > Actually that only worked on Linux ports, not on Mac. So I'm coming back to
> > the code in the first patch.
> 
> Would eventually like to know what didn’t work on Mac about this. Not
> important, but curious. I suppose I can try it myself.

The error was:
./dom/Document.cpp:4360:48: error: braces around scalar initializer [-Werror,-Wbraced-scalar-init]

I've been now testing on a Mac machine and the problem was that I was using too many braces.
This works fine on Mac:
    setFocusedElement(nullptr, { { }, { }, FocusRemovalEventsMode::DoNotDispatch, { } });

I'll upload a follow-up patch to change this.

[Manuel Rego Casasnovas]

Reopening to attach new patch.

[Manuel Rego Casasnovas]

Created attachment 426128 [details]
Patch

[EWS]

Committed r276077 (236590@main): <https://commits.webkit.org/236590@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426128 [details].