Bug 151235 - Web Inspector: Type popover should shrink to its content size
Summary: Web Inspector: Type popover should shrink to its content size
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-11-12 16:47 PST by Nikita Vasilyev
Modified: 2016-12-13 15:38 PST (History)
4 users (show)

See Also:


Attachments
[Image] Bug (101.13 KB, image/png)
2015-11-12 16:47 PST, Nikita Vasilyev
no flags Details
[Image] Debugger popovers (59.75 KB, image/png)
2015-11-12 16:49 PST, Nikita Vasilyev
no flags Details
Patch (1.45 KB, patch)
2016-03-13 10:54 PDT, Daniel Strokis
bburg: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2015-11-12 16:47:24 PST
Created attachment 265451 [details]
[Image] Bug

See the attached screenshot. There is no reason for the type popover to be that tall.
Comment 1 Radar WebKit Bug Importer 2015-11-12 16:47:50 PST
<rdar://problem/23527020>
Comment 2 Nikita Vasilyev 2015-11-12 16:49:32 PST
Created attachment 265453 [details]
[Image] Debugger popovers

For debugger, popovers are nice and small (see the attached image). Type popovers should be the same.r
Comment 3 Daniel Strokis 2016-03-13 10:54:21 PDT
Created attachment 273896 [details]
Patch
Comment 4 Joseph Pecoraro 2016-03-13 15:44:21 PDT
Comment on attachment 273896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273896&action=review

> Source/WebInspectorUI/ChangeLog:8
> +        Popovers in the source code editor that were cuased by hovering over a symbol were unnecessarily large due to min-width and min-height rules. Removed those style rules.

I think this was added to have better basic support for Object/Class popovers which dynamically fill in content. What happens when you hover an Object/Element/Node type bubble that has a list of properties?
Comment 5 Joseph Pecoraro 2016-03-13 15:47:10 PDT
(In reply to comment #4)
> Comment on attachment 273896 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273896&action=review
> 
> > Source/WebInspectorUI/ChangeLog:8
> > +        Popovers in the source code editor that were cuased by hovering over a symbol were unnecessarily large due to min-width and min-height rules. Removed those style rules.
> 
> I think this was added to have better basic support for Object/Class
> popovers which dynamically fill in content. What happens when you hover an
> Object/Element/Node type bubble that has a list of properties?

If this is the case, perhaps a better solution would be to set some default min size for "Object" popovers and resize the popover to better fit when content changes dynamically.
Comment 6 Daniel Strokis 2016-03-13 16:17:13 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 273896 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=273896&action=review
> > 
> > > Source/WebInspectorUI/ChangeLog:8
> > > +        Popovers in the source code editor that were cuased by hovering over a symbol were unnecessarily large due to min-width and min-height rules. Removed those style rules.
> > 
> > I think this was added to have better basic support for Object/Class
> > popovers which dynamically fill in content. What happens when you hover an
> > Object/Element/Node type bubble that has a list of properties?
> 
> If this is the case, perhaps a better solution would be to set some default
> min size for "Object" popovers and resize the popover to better fit when
> content changes dynamically.

AFAICT from testing, popovers dynamically resize for Objects, but not for Nodes. I think having a class for Nodes that retains the min-width and min-height rules could be a viable solution.
Comment 7 Daniel Strokis 2016-03-13 16:28:58 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Comment on attachment 273896 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=273896&action=review
> > > 
> > > > Source/WebInspectorUI/ChangeLog:8
> > > > +        Popovers in the source code editor that were cuased by hovering over a symbol were unnecessarily large due to min-width and min-height rules. Removed those style rules.
> > > 
> > > I think this was added to have better basic support for Object/Class
> > > popovers which dynamically fill in content. What happens when you hover an
> > > Object/Element/Node type bubble that has a list of properties?
> > 
> > If this is the case, perhaps a better solution would be to set some default
> > min size for "Object" popovers and resize the popover to better fit when
> > content changes dynamically.
> 
> AFAICT from testing, popovers dynamically resize for Objects, but not for
> Nodes. I think having a class for Nodes that retains the min-width and
> min-height rules could be a viable solution.

The popover for a Node does show all of the data, but it feels cramped.
Comment 8 BJ Burg 2016-03-18 10:24:36 PDT
Comment on attachment 273896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273896&action=review

Thanks for submitting a patch. Please include screenshots of the various cases Joe mentioned with this patch applied so we can move forward.

>>>>> Source/WebInspectorUI/ChangeLog:8
>>>>> +        Popovers in the source code editor that were cuased by hovering over a symbol were unnecessarily large due to min-width and min-height rules. Removed those style rules.
>>>> 
>>>> I think this was added to have better basic support for Object/Class popovers which dynamically fill in content. What happens when you hover an Object/Element/Node type bubble that has a list of properties?
>>> 
>>> If this is the case, perhaps a better solution would be to set some default min size for "Object" popovers and resize the popover to better fit when content changes dynamically.
>> 
>> AFAICT from testing, popovers dynamically resize for Objects, but not for Nodes. I think having a class for Nodes that retains the min-width and min-height rules could be a viable solution.
> 
> The popover for a Node does show all of the data, but it feels cramped.

Typo: 'caused'
Comment 9 BJ Burg 2016-04-01 07:41:39 PDT
Comment on attachment 273896 [details]
Patch

Clearing r? since it seems that you were going to improve handling of the Node popover case. (FYI, in the future, adding screenshots of relevant cases to the bug can make reviewing go a lot faster.)