Open
Bug 569331
Opened 15 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)
Tracking
()
NEW
People
(Reporter: mstange, Unassigned)
References
(Depends on 2 open bugs)
Details
Attachments
(2 files)
10.91 KB,
patch
|
Details | Diff | Splinter Review | |
24.57 KB,
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?
Comment 1•15 years ago
|
||
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.
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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. :-)
Reporter | ||
Comment 4•10 years ago
|
||
(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)
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
(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)
Comment 7•10 years ago
|
||
(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.
Comment 8•10 years ago
|
||
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)
Comment 9•10 years ago
|
||
Line 775 of the above as of this time is:
shell->HandleEvent(frame, aEvent, false, aStatus)
Comment 10•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
I wonder what Safari and Chrome do for Dictionary and IME...
Comment 12•10 years ago
|
||
(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.
Comment 13•10 years ago
|
||
(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).
Comment 14•10 years ago
|
||
(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.
Comment 15•10 years ago
|
||
(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.
Comment 16•10 years ago
|
||
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.
Comment 17•10 years ago
|
||
(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.
Comment 18•10 years ago
|
||
(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?
Comment 19•10 years ago
|
||
(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.
Comment 20•10 years ago
|
||
(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.
Comment 21•10 years ago
|
||
(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.
Comment 22•10 years ago
|
||
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.
Comment 23•10 years ago
|
||
(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.
Updated•10 years ago
|
Assignee: nobody → quanxunzhen
Comment 24•10 years ago
|
||
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
Comment 25•10 years ago
|
||
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
Comment 26•9 years ago
|
||
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.
Comment 27•9 years ago
|
||
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.
Comment 28•9 years ago
|
||
And the issue is here: https://code.google.com/p/chromium/issues/detail?id=17951
Comment 29•9 years ago
|
||
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.
Comment 30•9 years ago
|
||
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.
Updated•9 years ago
|
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
Comment 31•9 years ago
|
||
Comment 32•9 years ago
|
||
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.
Comment 33•8 years ago
|
||
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
Comment 34•8 years ago
|
||
(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)
Comment 35•8 years ago
|
||
(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)
Comment 36•4 years ago
|
||
Resetting assignee which I don't work on in this several months.
Assignee: masayuki → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•