Move FocusRemovalEventsMode into FocusOptions
Created attachment 425984 [details] Patch
Created attachment 426041 [details] Patch
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 });
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.
Created attachment 426079 [details] Patch for landing
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).
Created attachment 426081 [details] Patch for landing
(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.
Created attachment 426089 [details] Patch for landing
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].
<rdar://problem/76695885>
(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.
(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.
Reopening to attach new patch.
Created attachment 426128 [details] Patch
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].