Bug 7987 – Inspector displays incorrect summary for padding information

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Sven-S. Porst]

When having CSS that sets something like

        body {padding:20px; padding-top:13px;}

the web inspector will display 'padding:13px' for the element and only reveal the correct information once the disclosure triangle is used to reveal the individual sub-properties.

If, however you use the CSS

        body {padding:20px; padding-left:13px;}

20px will be displayed for padding. 

This kind of inconsistency is bad for the trust you have in the inspector and makes it less useful for 'debugging' style sheets.

Improvements on this behaviour might be to just display '--' when there is no consistent padding property or to display the full set of four distances.

This problem may be related to #7703.

I will add a simple test case and a screenshot from a more complex situation.

[Sven-S. Porst]

Created attachment 7296 [details]
simple html test file 

The attached HTML file illustrates the problem in the body element.

[Sven-S. Porst]

Created attachment 7297 [details]
Screenshot of the problem

A screenshot to illustrate the problem in a more complex context. 

Showing the CSS at the left and the Web Inspector at the right.

[Adam Roben (:aroben)]

<rdar://problem/5712923>

[Jessie Berlin]

Created attachment 40204 [details]
Test case with separate cases of specifying padding top and padding left.

In r48794, the results are slightly different than described previously. I am adding this test case so that both cases of specifying padding top and specifying padding left are shown.

In the case of specifying the padding and the padding top (padding: 20px; padding-top: 35px;), the following is displayed in the Web Inspector:

-> padding: 35px;
      padding-top: 35px;

and when expanded:

padding: 35px;
    padding-right: 20px;
    padding-bottom: 20px;
    padding-left: 20px;
padding-top: 35px;

In the case of specifying the padding and the padding left (padding: 20px; padding-left: 5px;), the following is displayed in the Web Inspector:

-> padding: 20px 5px;
      padding-left: 5px;

and when expanded:

padding: 20px 5px;
    padding-top: 20px;
    padding-right: 20px;
    padding-bottom: 20px;
padding-left: 5px;

I don't believe this is less confusing though. Thoughts on a better display?

[Timothy Hatcher]

(In reply to comment #4)
> Created an attachment (id=40204) [details]
> Test case with separate cases of specifying padding top and padding left.
> 
> In r48794, the results are slightly different than described previously. I am
> adding this test case so that both cases of specifying padding top and
> specifying padding left are shown.
> 
> In the case of specifying the padding and the padding top (padding: 20px;
> padding-top: 35px;), the following is displayed in the Web Inspector:
> 
> -> padding: 35px;
>       padding-top: 35px;
> 
> and when expanded:
> 
> padding: 35px;
>     padding-right: 20px;
>     padding-bottom: 20px;
>     padding-left: 20px;
> padding-top: 35px;
> 
> In the case of specifying the padding and the padding left (padding: 20px;
> padding-left: 5px;), the following is displayed in the Web Inspector:
> 
> -> padding: 20px 5px;
>       padding-left: 5px;
> 
> and when expanded:
> 
> padding: 20px 5px;
>     padding-top: 20px;
>     padding-right: 20px;
>     padding-bottom: 20px;
> padding-left: 5px;
> 
> I don't believe this is less confusing though. Thoughts on a better display?

I think what is expected is:

padding: --20px-- 20px 20px 20px;
   --padding-top: 20px;--
        padding-right: 20px;
        padding-bottom: 20px;
        padding-left: 20px;
padding-top: 35px;

Where "--" is striked out.

[Joseph Pecoraro]

Here is more information on what is currently output and where that output is coming from.  Note that "x" is the "style" object on the padding property being displayed.  The images have plenty of detail.

> padding: 35px;
>     padding-right: 20px;
>     padding-bottom: 20px;
>     padding-left: 20px;
> padding-top: 35px;

#outer_top
http://grab.by/6WF
- fading is consistent
- getShorthandValue is only "35px" (not correct, misleading)
- getLonghandProperties returns 3 elements (indicates 1 missing)


> padding: 20px 5px;
>     padding-top: 20px;
>     padding-right: 20px;
>     padding-bottom: 20px;
> padding-left: 5px;

#outer_left
http://grab.by/6WB
- fading is not consistent
- getShortHandValue is "20px 5px" (not correct, misleading)
- getLonghandProperties returns 3 elements (indicates 1 missing)


> padding: 10px;
>     padding-top: 10px;
>     padding-right: 10px;
>     padding-bottom: 10px;
>     padding-left: 10px;

A <div> I created with div.style.padding = '10px';
http://grab.by/6WG
- fading is not consistent
- getShortHandValue is "10px" (correct)
- getLonghandProperties returns 4 elements

- - - 

> I think what is expected is:
> 
> padding: --20px-- 20px 20px 20px;
>    --padding-top: 20px;--
>         padding-right: 20px;
>         padding-bottom: 20px;
>         padding-left: 20px;
> padding-top: 35px;
> 
> Where "--" is striked out.

Although this is possible from the JavaScript by fiddling with the Shorthands and Longhands I'd be worried about the maintainability of a JavaScript solution.  Any tips on where these values are calculated on the C++ side?

I'm really interested in how it came up with "20px 5px" for #outer_left which indicates top and bottom both have 20px and left and right both have 5px. Thats clearly not right.

[Jessie Berlin]

This behavior is also exhibited by border-color and border-width (and presumably border-style and margin) since they all use

String CSSMutableStyleDeclaration::get4Values(const int* properties) const
{
    String res;
    for (int i = 0; i < 4; ++i) {
        if (!isPropertyImplicit(properties[i])) {
            RefPtr<CSSValue> value = getPropertyCSSValue(properties[i]);

            // apparently all 4 properties must be specified.
            if (!value)
                return String();

            if (!res.isNull())
                res += " ";
            res += value->cssText();
        }
    }
    return res;
}

It seems that the "top" (e.g. padding-top) property is always set to be not implicit, regardless of whether it was explicitly set, but I am not sure why just yet.


(In reply to comment #6)
> Here is more information on what is currently output and where that output is
> coming from.  Note that "x" is the "style" object on the padding property being
> displayed.  The images have plenty of detail.
> 
> > padding: 35px;
> >     padding-right: 20px;
> >     padding-bottom: 20px;
> >     padding-left: 20px;
> > padding-top: 35px;
> 
> #outer_top
> http://grab.by/6WF
> - fading is consistent
> - getShorthandValue is only "35px" (not correct, misleading)
> - getLonghandProperties returns 3 elements (indicates 1 missing)
> 
> 
> > padding: 20px 5px;
> >     padding-top: 20px;
> >     padding-right: 20px;
> >     padding-bottom: 20px;
> > padding-left: 5px;
> 
> #outer_left
> http://grab.by/6WB
> - fading is not consistent
> - getShortHandValue is "20px 5px" (not correct, misleading)
> - getLonghandProperties returns 3 elements (indicates 1 missing)
> 
> 
> > padding: 10px;
> >     padding-top: 10px;
> >     padding-right: 10px;
> >     padding-bottom: 10px;
> >     padding-left: 10px;
> 
> A <div> I created with div.style.padding = '10px';
> http://grab.by/6WG
> - fading is not consistent
> - getShortHandValue is "10px" (correct)
> - getLonghandProperties returns 4 elements
> 
> - - - 
> 
> > I think what is expected is:
> > 
> > padding: --20px-- 20px 20px 20px;
> >    --padding-top: 20px;--
> >         padding-right: 20px;
> >         padding-bottom: 20px;
> >         padding-left: 20px;
> > padding-top: 35px;
> > 
> > Where "--" is striked out.
> 
> Although this is possible from the JavaScript by fiddling with the Shorthands
> and Longhands I'd be worried about the maintainability of a JavaScript
> solution.  Any tips on where these values are calculated on the C++ side?
> 
> I'm really interested in how it came up with "20px 5px" for #outer_left which
> indicates top and bottom both have 20px and left and right both have 5px. Thats
> clearly not right.

[Timothy Hatcher]

(In reply to comment #7)
> This behavior is also exhibited by border-color and border-width (and
> presumably border-style and margin) since they all use
> 
> String CSSMutableStyleDeclaration::get4Values(const int* properties) const

Yes this is the function used by all shorthands.

> It seems that the "top" (e.g. padding-top) property is always set to be not
> implicit, regardless of whether it was explicitly set, but I am not sure why
> just yet.

The top (or first property in the shorthand) is always explicit because it is first.

So margin: 0px really means margin-top: 0px and copy 0px to the other properties implicitly.

[Jessie Berlin]

Created attachment 42834 [details]
Makes the summary information for a padding shorthand reflect the specification for the shorthand.

I could not find a way from the CSSMutableStyleDeclaration class to get at the original style rule for the shorthand was. This instead reconstructs what the shorthand would have to be in order for the 4 properties to have the values they do.

Something similar probably could be done in the Javascript using getLonghandProperties and getShorthandValue, but I think it would end up looking much the same.

This also fixes the bug where, if you had {padding: 20px; padding-top:35px;} and in the web-inspector disabled and then re-enabled the padding shorthand, 35px would be applied to all the sides instead of just the top. This happened because the shorthand disabled and then re-enabled would have been "padding: 35px". With this patch, that shorthand would be "padding: 35px 20px 20px;", which, if disabled and enabled again, would make sure the right padding (20px) would get applied to the right and left and bottom padding properties.

Comment on attachment 42834 [details]
Makes the summary information for a padding shorthand reflect the specification for the shorthand.

Can you upload a patch with a layout test?

[Jessie Berlin]

(In reply to comment #10)
> (From update of attachment 42834 [details])
> Can you upload a patch with a layout test?

I wasn't sure exactly how to test this. I wasn't able to find a way to get at the longhands and shorthands except through the InjectedScript inside the WebInspector.

Any suggestions would be welcome :)

[Timothy Hatcher]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 42834 [details] [details])
> > Can you upload a patch with a layout test?
> 
> I wasn't sure exactly how to test this. I wasn't able to find a way to get at
> the longhands and shorthands except through the InjectedScript inside the
> WebInspector.
> 
> Any suggestions would be welcome :)

You can do style.getPropertyShorthand() and style.isPropertyImplicit() from JS.

[Jessie Berlin]

Created attachment 42954 [details]
Make the summary information for a padding, margin, border-color, border-style, or border-width shorthand reflect the specification of the shorthand

Added layout tests, simplified the logic in CSSMutableStyleDeclaration::get4Values.

[Jessie Berlin]

Comment on attachment 42954 [details]
Make the summary information for a padding, margin, border-color, border-style, or border-width shorthand reflect the specification of the shorthand

Committed in http://trac.webkit.org/changeset/50834