Bug 28972 – REGRESSION: Styles showing as implicit when they shouldn't

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

[Timothy Hatcher]

Styles are showing as implicit (dimmed) when they shouldn't.

Steps:
1) Visit webkit.org
2) Inspect #icon
3) Look at the styles.

Results:
Multiple things are dimmed.

Expected:
Nothing should be dimmed here.

[Timothy Hatcher]

Created attachment 39066 [details]
Bug

[Timothy Hatcher]

Created attachment 39067 [details]
Expected

[Yury Semikhatsky]

This regression was introduced with the fix for
https://bugs.webkit.org/show_bug.cgi?id=28635.

[Yury Semikhatsky]

Created attachment 39914 [details]
patch and test

[Timothy Hatcher]

Comment on attachment 39914 [details]
patch and test

Looks OK to me, but Hyatt, Beth or Mitz should review.

[Timothy Hatcher]

I still wonder why the Inspector shows them as implicit (dim), when they should show as normal properties. Fixing the shorthand is correct. But they should be black despite fixing the shorthand.

[Yury Semikhatsky]

(In reply to comment #6)
> I still wonder why the Inspector shows them as implicit (dim), when they should
> show as normal properties. Fixing the shorthand is correct. But they should be
> black despite fixing the shorthand.
With the patch applied  only background-repeat-x and background-repeat-y are dimmed for the icon at webkit.org while background-repeat is shown as explicit style. It seems to be correct representation since http://webkit.org/css/main.css declares  background-repeat: no-repeat; (shorthand property).

[Timothy Hatcher]

(In reply to comment #7)
> (In reply to comment #6)
> > I still wonder why the Inspector shows them as implicit (dim), when they should
> > show as normal properties. Fixing the shorthand is correct. But they should be
> > black despite fixing the shorthand.
>
> With the patch applied  only background-repeat-x and background-repeat-y are
> dimmed for the icon at webkit.org while background-repeat is shown as explicit
> style. It seems to be correct representation since
> http://webkit.org/css/main.css declares  background-repeat: no-repeat;
> (shorthand property).

OK, I see.

Actually background-repeat-x should be black, since it is the first in the shorthand. Like what you see with margin: 0, margin-top: 0 will be black and the rest are implicit (dim).

So I think there is more that needs fixed here, but I don't know what to suggest.

[Adam Barth]

Timothy, what's the status of this patch?  Should we land this one and then fix the remaining cases in another bug?  Discussion seems to have stagnated.

[Dave Hyatt]

Comment on attachment 39914 [details]
patch and test

Fix the ChangeLog typo... "correct" not "cirrect".

[Yury Semikhatsky]

(In reply to comment #9)
> Timothy, what's the status of this patch?  Should we land this one and then fix
> the remaining cases in another bug?  Discussion seems to have stagnated.

Sorry, I was distracted by other issues and didn't have time to work on this one.

(In reply to comment #8)
> Actually background-repeat-x should be black, since it is the first in the
> shorthand. Like what you see with margin: 0, margin-top: 0 will be black and
> the rest are implicit (dim).
> 
> So I think there is more that needs fixed here, but I don't know what to
> suggest.
I don't know whether the difference in handling background-repeat and other properties affected by this patch and those whose values are parsed by CSSParser::parse4Values (margin, padding, color, border-style, border-width) is intentional or not but it is parse4Values that makes first longhand property explicit in some cases. I could support similar behavior for background-repeat and others but it's going to be a bit trickier since they are handled in several places in CSSParser.

[Adam Barth]

Comment on attachment 39914 [details]
patch and test

Hyatt asked for some minor changes to this patch, so we can't land it automatically.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/backgrounds/repeat/background-repeat-shorthand-expected.txt
	A	LayoutTests/fast/backgrounds/repeat/background-repeat-shorthand.html
	A	LayoutTests/fast/backgrounds/repeat/resources/background-repeat-shorthand.css
	A	LayoutTests/fast/backgrounds/repeat/resources/background-repeat-shorthand.js
	M	WebCore/ChangeLog
	M	WebCore/css/CSSParser.cpp
Committed r49616