Bug 182471 - Web Inspector: Styles: Control-Space should show completion popover
Summary: Web Inspector: Styles: Control-Space should show completion popover
Status: RESOLVED DUPLICATE of bug 194796
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-04 20:08 PST by Nikita Vasilyev
Modified: 2022-03-01 02:28 PST (History)
3 users (show)

See Also:

Attachments
Patch (8.35 KB, patch)
2018-02-04 20:27 PST, Nikita Vasilyev 		bburg: review-
Details | Formatted Diff | Diff
[Animated GIF] With patch applied (254.56 KB, image/gif)
2018-02-04 20:32 PST, Nikita Vasilyev 		no flags Details
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2018-02-04 20:08:20 PST 
Control-Space shows completion in just about every modern code editor, including Sublime Text, Chrome DevTools, and Xcode.

The new styles sidebar doesn't have a way to show all available completion values for an empty prefix.
Comment 1 Radar WebKit Bug Importer 2018-02-04 20:08:58 PST 
<rdar://problem/37222435>
Comment 2 Nikita Vasilyev 2018-02-04 20:27:04 PST 
Created attachment 333059 [details]
Patch
Comment 3 Nikita Vasilyev 2018-02-04 20:32:01 PST 
Created attachment 333060 [details]
[Animated GIF] With patch applied
Comment 4 BJ Burg 2018-02-05 11:54:07 PST 
Comment on attachment 333059 [details]
Patch

I don't like your changes. Why thread acceptEmptyPrefix everywhere? It would be better to make a different factory method on WI.CSSKeywordCompletions that returns an object with all completions, and use it.

This is not possible right now because completionProvider is a function, not a delegate/object. I don't understand the completionProvider design. It seems like this should just be a different delegate that provides completion objects to various widgets. As it's written now, it's just a callback so it can't have different methods (ie., completionsForProperty(), completionsForValue()). Not only does this make the code hard to read (i.e., what does this._completionProvider() actually do?) but it scatters the responsibility for figuring out what type completions to provide. It should be obvious at the calcite.
Comment 5 BJ Burg 2018-02-05 11:54:47 PST 
s/calcite/call site/
Comment 6 Nikita Vasilyev 2018-02-05 12:49:42 PST 
(In reply to Brian Burg from comment #4)
> Comment on attachment 333059 [details]
> Patch
> 
> I don't like your changes. Why thread acceptEmptyPrefix everywhere? It would
> be better to make a different factory method on WI.CSSKeywordCompletions
> that returns an object with all completions, and use it.

How would the new factory method on WI.CSSKeywordCompletions be different from the existing ones?
Comment 7 BJ Burg 2018-02-05 14:20:53 PST 
(In reply to Nikita Vasilyev from comment #6)
> (In reply to Brian Burg from comment #4)
> > Comment on attachment 333059 [details]
> > Patch
> > 
> > I don't like your changes. Why thread acceptEmptyPrefix everywhere? It would
> > be better to make a different factory method on WI.CSSKeywordCompletions
> > that returns an object with all completions, and use it.
> 
> How would the new factory method on WI.CSSKeywordCompletions be different
> from the existing ones?

Perhaps not a different factory, but a different map chain function that doesn't filter stuff out.
Comment 8 Nikita Vasilyev 2019-03-14 13:56:43 PDT 

*** This bug has been marked as a duplicate of bug 194796 ***