Open Bug 569331 Opened 14 years ago Updated 2 years ago

WidgetQueryContentEvent should be able to query whole contents of the focused document if it's requested by the dispatcher

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Platform:
All
macOS
Type:
defect

Tracking

()

People

(Reporter: mstange, Unassigned)

References

(Depends on 2 open bugs)

Details

Attachments

(2 files)

part.1 Implement a way to query whole contents of the focused document
(deleted), patch
Details | Diff | Splinter Review
part.2 Request to query whole contents when it seems that the dictionary service of OS X is working with us
(deleted), patch
Details | Diff | Splinter Review 
Looking up words in the dictionary by pressing Cmd+Ctrl+D (see bug 301451) calls NSTextInput methods that send content query events into Gecko. However, those events are constrained to focused elements, which breaks dictionary lookup in non-focused elements.

Masayuki, I'm afraid of breaking IME when trying to fix this. Can you look into this?
Probably, the change can break IME behavior.  E.g., IME cannot know that where is a boundary of a editor.

The main issue is that our native widget has one or more contexts. If ChildView can create a dummy NSView/NSTextInput class for focused editor and set focus to it, we can fix all similar issues completely.

But I'm not sure whether it's possible and realistic.
I am willing to work on this bug in order to unblock Bug 301451. Does anyone know what files may possibly be relevant to this bug?
Flags: needinfo?(mstange)
Thanks for your help, Nathan! Once you've pulled the current source, I'd start by applying attachment 8463189 [details] [diff] [review] from bug 308471. You can then observe that a Cmd-Ctrl-D lookup on text in a non-focused widget will fail with "charAt event returns not found" in the console. Stepping through the code after breaking at the mWidget->DispatchWindowEvent(charAt); call in attachment 8463189 [details] [diff] [review] should tell you what code is involved. I may have better advice for you after I had the time to look into it myself tomorrow, or Markus might beat me to it. :-)
(In reply to Nathan Yee from comment #2)
> Does anyone know what files may possibly be relevant to this bug?

Other than "everything that touches WidgetQueryContentEvent", I don't know, sorry. It's been a long time since I've looked at it and much of the code has changed.
Flags: needinfo?(mstange)
I am testing attachment 8463189 [details] [diff] [review] from bug 308471. Strangely, cmd-ctrl-D/three-finger-tap-gesture works without needing to highlight text once text on a page is clicked on. This behavior goes away when changing pages though. As described in bug 569334, the Dictionary overlay text shows the wrong font superimposed on the desired text (possibly Apple’s default behavior when the font attributes of a block of text cannot be determined?). I will begin debugging soon. Stephen does https://developer.mozilla.org/en-US/docs/Debugging_on_Mac_OS_X accurately describe how to set up debugging in OS X?
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to Nathan Yee from comment #5)
> I am testing attachment 8463189 [details] [diff] [review] from bug 308471.
> Strangely, cmd-ctrl-D/three-finger-tap-gesture works without needing to
> highlight text once text on a page is clicked on. This process has to be
> repeated again after changing pages though.

Right, this is the expected behavior until bug 569334 is fixed. The goal of bug 569334 is to make it so that you don't need to click on the text or page first. The dictionary lookup doesn't work after changing pages because the page (or its content) doesn't have focus. You should be able to observe the same behavior between text on a page and text in a text field.

Highlighting text isn't necessary for dictionary lookups to work.

> As described in bug 569334, the
> Dictionary overlay text shows the wrong font superimposed on the desired
> text (possibly Apple’s default behavior when the font attributes of a block
> of text cannot be determined?).

Right, that's the third step described in bug 301451 comment 75, i.e. the focus issue and text styling are separate problems and can be fixed individually. Feel free to pick either one first.

> I will begin debugging soon. Stephen does
> https://developer.mozilla.org/en-US/docs/Debugging_on_Mac_OS_X accurately
> describe how to set up debugging in OS X?

Yes, a quick look seems to indicate that this page is quite up to date.
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to Stephen Pohl [:spohl] from comment #6)
> (In reply to Nathan Yee from comment #5)
> > I am testing attachment 8463189 [details] [diff] [review] from bug 308471.
> > Strangely, cmd-ctrl-D/three-finger-tap-gesture works without needing to
> > highlight text once text on a page is clicked on. This process has to be
> > repeated again after changing pages though.
> 
> Right, this is the expected behavior until bug 569334 is fixed.

Ugh, I meant bug 569331 (this one here), not bug 569334. I actually missed the fact that bug 569334 existed, so there is no need to track proper font styling in bug 301451 like I suggested in bug 301451 comment 75.
I've been having difficulty trying to debug this issue. However, I have noticed that nsViewManager::DispatchEvent may be related to the problem, as line 775 appears to return either nsEventStatus_eConsumeNoDefault or nsEventStatus_eConsumeDoDefault depending on some unknown factor. nsChildView, nsView and nsViewManager are all possibly related to this bug. Stephen do you happen to have any more advice on this?
Flags: needinfo?(spohl.mozilla.bugs)
Line 775 of the above as of this time is:

shell->HandleEvent(frame, aEvent, false, aStatus)
I don't have much advice at this point, other than to try and compare a successful lookup with an unsuccessful one and to keep in mind what Masayuki said in comment 1 and bug 308471 comment 35.

An unsuccessful lookup has the characteristic that the |charAt.mReply.mOffset| in IMEInputHandler::CharacterIndexForPoint will have a value that's indicative of 'not found'. By setting a watchpoint on |charAt.mReply.mOffset| before calling |mWidget->DispatchWindowEvent(charAt)|, it's clear that the point of failure is in ContentEventHandler::OnQueryCharacterAtPoint[1] because |nsContentUtils::ContentIsDescendantOf(targetFrame->GetContent(), mRootContent)| returns false. Unfortunately, I don't have any suggestion how to fix this at the moment. I'll try to spend some more time on this sometime this week and will report back should I come up with anything.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/events/ContentEventHandler.cpp#970
Flags: needinfo?(spohl.mozilla.bugs)
I wonder what Safari and Chrome do for Dictionary and IME...
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11)
> I wonder what Safari and Chrome do for Dictionary and IME...

I think they blur any focus when dictionary lookup happens.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #12)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11)
> > I wonder what Safari and Chrome do for Dictionary and IME...
> 
> I think they blur any focus when dictionary lookup happens.

Sounds reasonable. Could you confirm it with window.addEventListener("blur", .., true) and window.addEventListener("focus", .., true)? If it's so, the events must be fired (at least blur).
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> 
> Sounds reasonable. Could you confirm it with window.addEventListener("blur",
> .., true) and window.addEventListener("focus", .., true)? If it's so, the
> events must be fired (at least blur).

There is at least one blur event every time dictionary lookup happens.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #14)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> > 
> > Sounds reasonable. Could you confirm it with window.addEventListener("blur",
> > .., true) and window.addEventListener("focus", .., true)? If it's so, the
> > events must be fired (at least blur).
> 
> There is at least one blur event every time dictionary lookup happens.

Hmmm, nope. If the document is not focused before, no blur will be triggered during lookup.
I see. How about with iframe? If an iframe has focus, dictionary look up targets its frame? or its root document?

Anyway, I think that we should create new event message such as NS_SELECTION_BLUR. ESM should handle it in proper process and call blur method of the focused content when an edit has focus. But I'm not sure *when* we should dispatch the new event from ChildView or TextInputHandler, though.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> I see. How about with iframe? If an iframe has focus, dictionary look up
> targets its frame? or its root document?

It seems that Safari simply blurs whatever has focus before, and doesn't return the focus back.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #17)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> > I see. How about with iframe? If an iframe has focus, dictionary look up
> > targets its frame? or its root document?
> 
> It seems that Safari simply blurs whatever has focus before, and doesn't
> return the focus back.

I meant that dictionary look up target is previously focused frame or its root document on Safari. Do you meant the latter behavior?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #17)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> > > I see. How about with iframe? If an iframe has focus, dictionary look up
> > > targets its frame? or its root document?
> > 
> > It seems that Safari simply blurs whatever has focus before, and doesn't
> > return the focus back.
> 
> I meant that dictionary look up target is previously focused frame or its
> root document on Safari. Do you meant the latter behavior?

I have no idea about that. Is there any difference in behavior? According to my test, the blur event is dispatched just like you switch to another window.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #19)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #17)
> > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> > > > I see. How about with iframe? If an iframe has focus, dictionary look up
> > > > targets its frame? or its root document?
> > > 
> > > It seems that Safari simply blurs whatever has focus before, and doesn't
> > > return the focus back.
> > 
> > I meant that dictionary look up target is previously focused frame or its
> > root document on Safari. Do you meant the latter behavior?
> 
> I have no idea about that. Is there any difference in behavior? According to
> my test, the blur event is dispatched just like you switch to another window.

Hmm, IIRC, our NS_QUERY_CONTENT_* events don't collect any sub document contents. So, if its target becomes its root frame, we need to hack ContentEventHandler.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #19)
> > I have no idea about that. Is there any difference in behavior? According to
> > my test, the blur event is dispatched just like you switch to another window.
> 
> Hmm, IIRC, our NS_QUERY_CONTENT_* events don't collect any sub document
> contents. So, if its target becomes its root frame, we need to hack
> ContentEventHandler.

I guess it is not a TextHandler event, but a window event before any query dispatched. If you try any native application, you can find any focus in it is lost at that time. We may not handle that window event properly like a normal application, which cause this problem.
Well, nope. The losing of focus happens after the highlight box appears.

They seem to use a page-wide querying at very beginning, which you can see from the implementation of WebKit here: https://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/mac/WKView.mm#L2102

Hope it is helpful for us.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> Hmm, IIRC, our NS_QUERY_CONTENT_* events don't collect any sub document
> contents. So, if its target becomes its root frame, we need to hack
> ContentEventHandler.

OK I finally understand what did you say here. I think we need to hack not only ContentEventHandler, but also nsContentIterator, which doesn't take sub document into account at all.
Assignee: nobody → quanxunzhen
Don't have time to work on this recently...

Some note about this bug:

I think for this bug, the prior task is allow looking up text in a document even when a text box has the focus.

To achieve this, we need to change the nsContentIterator. When we find a node without any children, check if it implements nsITextControlElement and has editor node. If it does, then treat the editor node as its only child and traverse into it. We may add a flag to constructor of nsContentIterator to control whether we should do that.

In addition to this bug, we should file another bug for not constraining content query events to only focused document. I think that is a different problem, and has a relatively lower priority than this bug, although that is still very important.
Assignee: quanxunzhen → nobody
nsFind [1] contains a useful nsContentIterator implementation which is able to go through text fields. It might be helpful for solving this.

In addition, nsTypeAheadFind [2] shows a way to go through all documents, which might also be helpful for this.

[1]: http://dxr.mozilla.org/mozilla-central/source/embedding/components/find/nsFind.cpp#53
[2]: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp#397
If we don't care e10s, CharacterIndexForPoint will be able to access content easily.  But for e10s and this issue, we need re-consider the implementation of this method.

FWIW, Chromium uses synchronized access (timeout: 1.5s) to content process for this implementation.
I found interesting document at trying to support dictionary in Chromium:
http://www.chromium.org/developers/design-documents/system-dictionary-pop-up-architecture

> Dictionary Pop-Up Internals
> This section will detail how Mac OS X brings up the dictionary pop-up. This examination was done on Mac OS X
> 10.6.5.
> 
> All typical Cocoa apps get the dictionary pop-up behavior for free because NSTextField and NSTextView conform
> to NSTextInput (or NSTextInputClient). These protocols define the three methods used to implement the
> dictionary functionality:
> 
>     - (NSAttributedString*)attributedSubstringFromRange:(NSRange)theRange
>     - (NSUInteger)characterIndexForPoint:(NSPoint)thePoint
>     - (NSRect)firstRectForCharacterRange:(NSRange)theRange
> 
> The system calls these methods in that order, with this rough pattern:
> 
>     -attributedStringForRange:{0, 10}
>     -characterIndexForPoint:{mouseX, mouseY} → returns index C 
>     -attributedStringForRange:{C, 50} → returns string S
>     -firstRectForCharacterRange:{C ± 50, S*.length}
> 
> It's unclear why each dictionary popup request always starts with (1), but it's likely some sort of warmup
> test. The rest is fairly forward: it receives the index of the character in the text stream over which the
> mouse points. From there, it gets the attributed string at that point and the 50 characters after it. The
> string it returns is attributed because the highlighted word effect is done by drawing the attributed string
> with the gray background over the word that is being looked up. Finally, it gets the drawing rectangle for
> the word so it knows where to position the popup. The range it passes is determined by the Dictionary
> framework, which breaks up the 50 character string and finds the individual word or phrase that is being
> looked up.

So, we should allow to access whole contents of the focused document in this cases. I guess that any IME doesn't use |characterIndexForPoint| when there is no composition. So, perhaps we can do that safely.
I created two experimental patches. However, I see some problems:

* ContentEventHandler doesn't dig out of anonymous subtrees, i.e., doesn't include text in <input type="text"> etc.

I'm investigating this issue. I'm still not sure if we should do that strictly. I mean, it might be better to use faster approach to include contents for performance.

* ContentEventHandler perhaps should insert \n at end of blocks.

The most strict approach is to check frame type of each content. But probably this is expensive. The second idea is to use nsParserService::IsBlock(). However, this is virtual method and needs to retrieve element ID from tag name. I guess that this is more expensive than the most strict approach. The most hacky idea is, we should add insert \n for most contents but just ignoring some inline elements.

I think that we should try the first approach first. But unfortunately, if the damage to the performance is too bad, we should use the last approach.

Anyway, perhaps, we should fix this issue first in other bug.
And also I have no idea to query contents which doesn't have focus.

For example, when the search bar has focus and move mouse cursor into the content a tab, then, WidgetQueryEvent target is the focused document. I.e., ties to query XUL elements of the browser window. I think that this should be out of scope of this bug for now. It might be possible, but more risky than just to fix this bug.

And also I don't want to support to query sub documents of focused document because it may cause much more expensive than current runtime cost... We should investigate that later.
Assignee: nobody → masayuki
Summary: Content query events from dictionary shouldn't be constrained to focused elements → WidgetQueryContentEvent should be able to query whole contents of the focused document if it's requested by the dispatcher
This works in most cases on OS X. But we need to fix a lot of issues of ContentEventHandler first because current ContentEventHandler has a lot of problem for daily use of Dictionary Service.
Blocks: 1177943
No longer blocks: 1177943
Now, dictionary service doesn't use IME's query event.  It is implemented using new API and nsLookUpDictionaryCommand, so I remove dependency.
No longer blocks: 301451
(In reply to Makoto Kato [:m_kato] from comment #33)
> Now, dictionary service doesn't use IME's query event.  It is implemented
> using new API and nsLookUpDictionaryCommand, so I remove dependency.

So, does it mean Gecko for OS X supports to use any text in the page with dictionary service even when an editor has focus? If so, we don't need to fix this bug anymore.
Flags: needinfo?(m_kato)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) from comment #34)
> (In reply to Makoto Kato [:m_kato] from comment #33)
> > Now, dictionary service doesn't use IME's query event.  It is implemented
> > using new API and nsLookUpDictionaryCommand, so I remove dependency.
> 
> So, does it mean Gecko for OS X supports to use any text in the page with
> dictionary service even when an editor has focus? If so, we don't need to
> fix this bug anymore.

I think that there are still another issues. (I will file this as new bug).  eQueryCharacterAtPoint cannot find frame if focus is non-editable and we tries to look up on editable.  If focus is chrome, command isn't dispatch to content well.
Flags: needinfo?(m_kato)

Resetting assignee which I don't work on in this several months.

Assignee: masayuki → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: