Closed Bug 1212527 Opened 9 years ago Closed 8 years ago

[10.11] QuickLook three-finger tap broken

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Version:
41 Branch
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: bram.speeckaert, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

MozReview Request: Bug 1212527 - Set background color to NSColor before calling showDefinitionForAttributedString on 10.11. r?mstange
(deleted), text/x-review-board-request
mstange
: review+
Details
minimal standalone testcase
(deleted), text/plain
Details 
The three-finger tap QuickLook feature no longer works for Firefox on OSX 10.11. It worked fine using OSX 10.10. The following entries appear in the System Log when trying to use this feature:

07/10/15 21:31:43,810 LookupViewService[422]: <NSXPCConnection: 0x7f8d7050b1c0> connection from pid 343: Warning: Exception caught during decoding of received message, dropping incoming message.
Exception: Exception while decoding argument 0 (#2 of invocation):
<NSInvocation: 0x7f8d70615d00>
return value: {v} void
target: {@} 0x0
selector: {:} bootstrap:withReply:
argument 2: {{?=@@@@{CGRect={CGPoint=dd}{CGSize=dd}}Icc@@@}} <00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000>
argument 3: {@?} 0x0 (block)

Exception: decodeObjectForKey: class "TitlebarAndBackgroundColor" not loaded or does not exist
07/10/15 21:31:43,810 firefox[343]: Lookup: Unhandled exception 'NSObjectNotAvailableException' caught in +[LULookupDefinitionModule _showDefinitionForTerm:atLocation:options:]]
Blocks: el-capitan
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: Cocoa
Ever confirmed: true
Product: Firefox → Core
Hardware: Unspecified → All
Xidorn, Masayuki, hasn't one of you discovered a new, async, API for this? Maybe that's the one we need to use starting with 10.11.
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(masayuki)
It seems the chromium issue 121917 [1] may provide some useful information. There is a proof-of-concept demo on #34. It seems to me that using that API could fix our focus issue as well. The down side is that it seems we may need to do segmentation ourselves.

[1] https://code.google.com/p/chromium/issues/detail?id=121917
Flags: needinfo?(quanxunzhen)
Nothing additional information, information in comment 2 is all.
Flags: needinfo?(masayuki)
Please raise priority for this bug, considering that Dictionary popover keyboard shortcut (ο£Ώ-^-D) is also broken, which is rather frequently used feature in native OS X apps.
Wait... we probably don't need to do segmentation ourselves. It seems Chrome has a different behavior for double click selecting and three-finger tap, which may indicate that the word picked for quicklook is not chosen by their internal word segmentation engine.

I also investigated a bit on the WebKit's implementation, but it seems they use some private function which I can find neither the code nor any document. :(
I looked a bit deeper into WebKit's implementation, and finally got things around their implementation we may need.

In WebKit's implementation, the range of the target word is chosen in DictionaryLookup::rangeAtHitTestResult [1], which picks 250 before and 250 after as a context, and passes that into luLookupDefinitionModule to extract the range of the word.

luLookupDefinitionModule is the trickiest thing here. It is a module from a private framework, which is imported in LookupSPI.h [2].

With this module, we would be able to choose the right word to show definition for. The only question is whether we want to use this private framework. If we do, I think this issue is solvable then. mstange?


[1] https://trac.webkit.org/browser/trunk/Source/WebCore/editing/mac/DictionaryLookup.mm?rev=192090#L96
[2] https://trac.webkit.org/browser/trunk/Source/WebCore/platform/spi/mac/LookupSPI.h?rev=188477
Flags: needinfo?(mstange)
I probably do not have time to do the implementation currently (still have a lot of things to do with text-emphasis and fullscreen). But if anyone is interested in fixing this, I can provide more help if needed.
Also it could be confirmed that Chrome does not use that private framework, because their result is significantly different from Safari's in some cases. Safari's behavior is more sensible as far as I can see.
Thanks for investigating!

We need to file bugs with Apple for any APIs that we need and that are not public. Can you summarize in what way Safari's behavior is more sensible than Chrome's?
Flags: needinfo?(mstange) → needinfo?(quanxunzhen)
(In reply to Markus Stange [:mstange] from comment #11)
> Thanks for investigating!
> 
> We need to file bugs with Apple for any APIs that we need and that are not
> public.

I'm not familiar with filing bug for Apple (other than WebKit and LLVM). Could you file that? Or alternatively tell me how can I do that?

> Can you summarize in what way Safari's behavior is more sensible
> than Chrome's?

Safari grabs more context so that it could extract a better word boundary.

This doesn't have much effect for English, because English uses space to separate words, but there are still some examples, like, if we want to lookup what does Big Bang mean, Chrome just highlights "Big" or "Bang" depending on which word you tap on, while Safari hightlights the whole "Big Bang", which also matches the behavior elsewhere in textbox on OS X.

It is more important for languages like Chinese and Japanese. For example, in Chinese, "δΈ­εŽδΊΊζ°‘ε…±ε’Œε›½" (People's Republic of China) is a single word which consists of three fine-granularity words "中华" (China), "δΊΊζ°‘" (People), and "ε…±ε’Œε›½" (Republic). Chrome can only mark every single fine-granularity word, but not the whole word, however Safari works as expected.
Flags: needinfo?(quanxunzhen)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #7)
> Wait... we probably don't need to do segmentation ourselves. It seems Chrome
> has a different behavior for double click selecting and three-finger tap,
> which may indicate that the word picked for quicklook is not chosen by their
> internal word segmentation engine.
> 
> I also investigated a bit on the WebKit's implementation, but it seems they
> use some private function which I can find neither the code nor any
> document. :(

Actually, there is no need in selecting the word and, hence, double-clicking. All you need is just to put your cursor over the word and hit a Command-Control-D.
(In reply to Yuriy Zakalyuzhnyy from comment #13)
> Actually, there is no need in selecting the word and, hence,
> double-clicking. All you need is just to put your cursor over the word and
> hit a Command-Control-D.

We were talking about how it can be implemented, not the behavior users see...
Oh, pardon me, the user ;)
Even a context menu "Look Up" would be immensely beneficial, similar to Search Google for "Word"
(In reply to Soheil from comment #16)
> Even a context menu "Look Up" would be immensely beneficial, similar to
> Search Google for "Word"

You can have the LookUp in a two finger tap context menu by using the add-on "askDictionary".
@platsch for reference, you are likely referring to this Firefox addon:
https://addons.mozilla.org/de/firefox/addon/ask-your-dictionary/

Ok workaround. Having a solution inside of firefox with 3 finger tap is of course faster, less window clutter, less clicks and much better integrated. but thanks for posting that workaround. seems to work on 10.11.1.
(In reply to platsch from comment #17) 
> You can have the LookUp in a two finger tap context menu by using the add-on
> "askDictionary".

This is an awful user experience. Opening the Dictionary app is not the same as an elegant pop up, I suggest you use Chrome or Safari's three finger look up if you haven't already.

Please don't suggest ideas that are clearly not the solution, this avoids confusion for other users.

If anything I find Search Google for "word" in a new tab a much less intrusive way than 1. Installing an add-on 2. Permitting it to open the Dictionary app 3. Opening the Dictionary app for every single look up.
To push this a bit, one proposal here.

I guess we can probably split this task into two steps: 
1. implement the new system API with a naive word segmentation algorithm, so that this function at least works;
2. ask Apple to publish the private framework and use that framework to replace our immature algorithm.

The first step does not depend on anything other than developer's time. Doing that would cause a regression to some extent for 10.7~10.10 where we currently have the correct behavior. But that would at least allow 10.11 users to lookup words for simple cases (like English single word split by whitespace), and to lookup the content user selects (which could be a workaround before we replace our naive algorithm with the system one). So I believe it'd be better to do this rather than do nothing until everything is ready.

Also, I expect the implementation with the new system API also fixes some of our existing bugs including bug 569331 and bug 1145224, and probably also bug 1177943 if we design the flow properly from the beginning.
I asked a friend in Apple, and he suggested we use CFStringTokenizer [1] or NSLinguisticTagger [2] to do text segmentation, so that we don't need to access the private framework.

It seems this is doable now...


[1] https://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFStringTokenizerRef/index.html#//apple_ref/doc/uid/TP40005095
[2] https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSLinguisticTagger_Class/
Any chances to see this fix in the nearest major release?
Hey Xidorn, if you explained to me how to solve it I could give it a try. 

As much information as possible would be welcome (since this is my first contribution to Mozilla).

Thanks!
despite xidorn's note about being less responsive, setting to NeedInfo from him, since that was requested in Comment 23 by marin.
Flags: needinfo?(quanxunzhen)
Well, it could be pretty challenging as a first contribution. But if you want to try, welcome! It hasn't been completely clear to me how to implement this, though.

So basically the entry of this is a "quickLookWithEvent:" handler [1] in either ChildView [2] or some certain subclass of NSWindow, probably BaseWindow [3]. And you need to call "showDefinitionForAttributedString:" [4] asynchronously to show the popup. See the demo in Chromium's issue page. [5]

This feature on 10.7~10.10 relies on several IME-related handlers including "characterIndexForPoint:", "attributedSubstringForProposedRange:", and "firstRectForCharacterRange:". All of them are currently in ChildView [2], and calls into corresponding methods inside TextInputHandler [6]. (See bug 308471, bug 569334 for somehow more details about the old implementation) You can follow those entries down to explore the current impl.

Since 10.8 or something, the "quickLookWithEvent:" and "showDefinitionForAttributedString:" are added for this. And the old path is no longer called since 10.11, and thus it is broken on 10.11.

In the old path, the system queries a large piece of text via attributedSubstringForProposedRange, and decide the word to be highlighted according to this. However, with the new path, system is no longer responsible to this, and we have to choose word to display definition for. And how to do this, is what I was exploring in comment 7, comment8, ... and the final answer is in comment 21.

The old impl which shares the IME callbacks suffers from bug 569331, which makes me want to look for another code path for this. In addition, the current impl of querying text content suffers from bug 1145224, which could potentially be fixed with reimplementing this in a different way here.

I think this is what I can tell at this point.

[1] https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSResponder_Class/index.html#//apple_ref/occ/instm/NSResponder/quickLookWithEvent:
[2] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.h#166
[3] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaWindow.h#66
[4] https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSView_Class/#//apple_ref/occ/instm/NSView/showDefinitionForAttributedString:atPoint:
[5] https://code.google.com/p/chromium/issues/detail?id=121917#c34
[6] https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/TextInputHandler.h#739-741,778-779,790
Flags: needinfo?(quanxunzhen)
I tried using showDefinitionForAttributedString with a fixed string and point as showcased in the Chrome issue page demo. Both synchronously and asynchronously, from ChildView and BaseWindow, in all cases I got the following exception:

2016-03-13 15:49:08.198 firefox[97571:148561] Lookup: Unhandled exception 'NSObjectNotAvailableException' caught in -[LULookupDefinitionModule showDefinitionForString:range:options:originProvider:inView:]
:m_kato is fixing bug 1177943, which would likely fix this as well at the same time, since we need to use the new API there.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1177943
Assignee: nobody → m_kato
Even if landing Bug 1177943, this doesn't work on 10.11.  Because, since nsCocoaWindow::initWithContentRect calls setBackgroundColor with our custom class TitlebarAndBackgroundColor, LookupViewService will throw exception.

So after landing e10s support, I will handle this.
s/nsCocoaWindow/ToolbarWindow/
TitlebarAndBackgroundColor is added by bug 303110...
Depends on: 1177943
When calling showDefinitionforAttributedString on OSX 10.11, it always return error due to the following exception.

Exception: decodeObjectForKey: class "TitlebarAndBackgroundColor" not loaded or does not exist

So before calling it, we set temporary color value, then restore color after calling it.

Review commit: https://reviewboard.mozilla.org/r/54990/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54990/
Attachment #8756155 - Flags: review?(mstange)
Attached file minimal standalone testcase (deleted) β€” 
Wow, that's a pretty amazing bug. We should definitely submit this to the Apple bug tracker. I'll do that now.
Attachment #8756491 - Attachment mime type: text/x-troff-mm → text/plain
rdar://26476091
Comment on attachment 8756155 [details]
MozReview Request: Bug 1212527 - Set background color to NSColor before calling showDefinitionForAttributedString on 10.11. r?mstange

https://reviewboard.mozilla.org/r/54990/#review51910

::: widget/cocoa/nsChildView.mm:2873
(Diff revision 1)
>  
> +// When using 10.11, calling showDefinitionForAttributedString causes the
> +// following exception on LookupViewService.
> +//
> +// Exception: decodeObjectForKey: class "TitlebarAndBackgroundColor" not
> +// loaded or does not exist

Please add the link "rdar://26476091" to this comment.

::: widget/cocoa/nsCocoaWindow.mm:3406
(Diff revision 1)
>    return mBackgroundColor;
>  }
>  
> +- (void)setTemporaryBackgroundColor
> +{
> +  [super setBackgroundColor:[NSColor clearColor]];

Let's use [NSColor whiteColor] here so that nothing else gets confused about the opaqueness of the window.
Attachment #8756155 - Flags: review?(mstange) → review+
Since Apple's radar bug tracker is non-public, for reference here's the openradar link:
https://openradar.appspot.com/26476091
https://hg.mozilla.org/integration/mozilla-inbound/rev/d89f93a40f78
Backed out 
http://hg.mozilla.org/integration/mozilla-inbound/rev/1bb60897b5e1

I fotget explicit keyword.  I will land it wit this fix.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3098ba832e42
https://hg.mozilla.org/mozilla-central/rev/3098ba832e42
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I've checked the 49.0b6 and it works great, thank you very much. But are there any chances for Force Touch support on Mac? Like in Safari, where hard press on the track pad invokes the same Dictionary pop-out as the three-finger tap.
(In reply to Yuriy Zakalyuzhnyy from comment #42)
> I've checked the 49.0b6 and it works great, thank you very much. But are
> there any chances for Force Touch support on Mac? Like in Safari, where hard
> press on the track pad invokes the same Dictionary pop-out as the
> three-finger tap.

You'd probably be interested in following bug 1277095.
(In reply to Stephen A Pohl [:spohl] from comment #43)
> (In reply to Yuriy Zakalyuzhnyy from comment #42)
> > I've checked the 49.0b6 and it works great, thank you very much. But are
> > there any chances for Force Touch support on Mac? Like in Safari, where hard
> > press on the track pad invokes the same Dictionary pop-out as the
> > three-finger tap.
> 
> You'd probably be interested in following bug 1277095.
Thanks, Stephen. Probably I should have better searching for it.
Thankfully this bug has been fixed, but what about the right-click menu entry? I can't understand why it doesn't work in Firefox.
(In reply to Hamam from comment #45)
> Thankfully this bug has been fixed, but what about the right-click menu
> entry? I can't understand why it doesn't work in Firefox.

It is bug 1116391.
Depends on: 1354715
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: