Closed Bug 1344149 Opened 7 years ago Closed 7 years ago

EditorEventListener should stop accepting odd focus/blur events

Categories

(Core :: DOM: Editor, enhancement)

Product:
Core
Component:
DOM: Editor
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file)

Bug 1344149 EditorEventListener shouldn't handle odd focus/blur events which may be created in chrome script
59 bytes, text/x-review-board-request
smaug
: review+
Details 
EditorEventListener currently listen odd focus/blur events which may be synthesized by JS. Then, editor will setup selection limitation. This may cause odd behavior. Let's stop accepting such event.
FYI: If chrome script synthesizes "focus" or "blur" event with |FocusEvent|, it still causes editor handling them for initializing/finalizing the selection limit. I'll file another bug for it because WidgetEvent needs new flag if it's initialized with script.
Comment on attachment 8844434 [details]
Bug 1344149 EditorEventListener shouldn't handle odd focus/blur events which may be created in chrome script

https://reviewboard.mozilla.org/r/117538/#review119646

::: editor/libeditor/EditorEventListener.cpp:193
(Diff revision 1)
>                                 TrustedEventsAtCapture());
>    elmP->AddEventListenerByType(this,
>                                 NS_LITERAL_STRING("click"),
>                                 TrustedEventsAtCapture());
> -// Focus event doesn't bubble so adding the listener to capturing phase.
> +  // Focus event doesn't bubble so adding the listener to capturing phase.
> -// Make sure this works after bug 235441 gets fixed.
> +  // XXX Should we listen focus/blur events of system group too? Or should

yes we should, I think, and IIRC I tried to make the change but it broke at least some tests

::: editor/libeditor/EditorEventListener.cpp:196
(Diff revision 1)
>                                 TrustedEventsAtCapture());
> -// Focus event doesn't bubble so adding the listener to capturing phase.
> +  // Focus event doesn't bubble so adding the listener to capturing phase.
> -// Make sure this works after bug 235441 gets fixed.
> +  // XXX Should we listen focus/blur events of system group too? Or should
> +  //     editor notified focus/blur of the element from nsFocusManager
> +  //     directly?  Because if the event propagation is stopped by JS,
> +  //     editor cannot initialize selection as expected.

Hmm, I don't know how to easily notify the editor. But stopPropagation in default group doesn't affect system group

::: editor/libeditor/EditorEventListener.cpp:1079
(Diff revision 1)
>  }
>  
>  nsresult
> -EditorEventListener::Focus(WidgetEvent* aFocusEvent)
> +EditorEventListener::Focus(InternalFocusEvent* aFocusEvent)
>  {
> -  if (NS_WARN_IF(!aFocusEvent) || DetachedFromEditor()) {
> +  if (DetachedFromEditorOrDefaultPrevented(aFocusEvent)) {

focus/blur shouldn't care about defaultPrevented(), so DetachedFromEditor() should be enough

::: editor/libeditor/EditorEventListener.cpp:1162
(Diff revision 1)
>  }
>  
>  nsresult
> -EditorEventListener::Blur(WidgetEvent* aBlurEvent)
> +EditorEventListener::Blur(InternalFocusEvent* aBlurEvent)
>  {
> -  if (NS_WARN_IF(!aBlurEvent) || DetachedFromEditor()) {
> +  if (DetachedFromEditorOrDefaultPrevented(aBlurEvent)) {

same here.
Attachment #8844434 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load, queue closed for couple of days) from comment #5)
> Comment on attachment 8844434 [details]
> Bug 1344149 EditorEventListener shouldn't handle odd focus/blur events which
> may be created in chrome script
> 
> https://reviewboard.mozilla.org/r/117538/#review119646
> 
> ::: editor/libeditor/EditorEventListener.cpp:193
> (Diff revision 1)
> >                                 TrustedEventsAtCapture());
> >    elmP->AddEventListenerByType(this,
> >                                 NS_LITERAL_STRING("click"),
> >                                 TrustedEventsAtCapture());
> > -// Focus event doesn't bubble so adding the listener to capturing phase.
> > +  // Focus event doesn't bubble so adding the listener to capturing phase.
> > -// Make sure this works after bug 235441 gets fixed.
> > +  // XXX Should we listen focus/blur events of system group too? Or should
> 
> yes we should, I think, and IIRC I tried to make the change but it broke at
> least some tests
> 
> ::: editor/libeditor/EditorEventListener.cpp:196
> (Diff revision 1)
> >                                 TrustedEventsAtCapture());
> > -// Focus event doesn't bubble so adding the listener to capturing phase.
> > +  // Focus event doesn't bubble so adding the listener to capturing phase.
> > -// Make sure this works after bug 235441 gets fixed.
> > +  // XXX Should we listen focus/blur events of system group too? Or should
> > +  //     editor notified focus/blur of the element from nsFocusManager
> > +  //     directly?  Because if the event propagation is stopped by JS,
> > +  //     editor cannot initialize selection as expected.
> 
> Hmm, I don't know how to easily notify the editor. But stopPropagation in
> default group doesn't affect system group

Yeah, but perhaps, if we'd listen to only system group events, we might break the web. For example, if web apps checks selection range in "focus" event handler, EditorEventListener hasn't listen to the coming "focus" event in system group. So, perhaps, it's the best way to stop listening "focus"/"blur" event and nsFocusManager should call something of editor directly. But I have no idea to decide focused editor before the editor getting "focus" event, sigh...

> ::: editor/libeditor/EditorEventListener.cpp:1079
> (Diff revision 1)
> >  }
> >  
> >  nsresult
> > -EditorEventListener::Focus(WidgetEvent* aFocusEvent)
> > +EditorEventListener::Focus(InternalFocusEvent* aFocusEvent)
> >  {
> > -  if (NS_WARN_IF(!aFocusEvent) || DetachedFromEditor()) {
> > +  if (DetachedFromEditorOrDefaultPrevented(aFocusEvent)) {
> 
> focus/blur shouldn't care about defaultPrevented(), so DetachedFromEditor()
> should be enough

Yeah, but DetachedFromEditorOrDefaultPrevented() is also checks if aFocusEvent is nullptr. Okay, I'll revert it as the current code.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/fd66e733b075
EditorEventListener shouldn't handle odd focus/blur events which may be created in chrome script r=smaug
https://hg.mozilla.org/mozilla-central/rev/fd66e733b075
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: