Bug 94923 - Web Inspector: Incorrect property override computation when !important is involved
Summary: Web Inspector: Incorrect property override computation when !important is inv...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-24 04:50 PDT by Alexander Pavlov (apavlov)
Modified: 2012-08-27 02:35 PDT (History)
11 users (show)

See Also:


Attachments
Patch (9.01 KB, patch)
2012-08-24 07:35 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (10.38 KB, patch)
2012-08-24 08:57 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (9.58 KB, patch)
2012-08-27 01:50 PDT, Alexander Pavlov (apavlov)
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2012-08-24 04:50:26 PDT
What steps will reproduce the problem?
1. Have an element with an !important css rule and a conflicting inline style
2. Inspect element

What is the expected result?

I'm expecting the element style to be crossed out as it is not the active style

What happens instead?

The element style is not crossed out. This can make it very hard to figure out what is going on, as the !important rule can be much farther down in the list of styles.

Test case:

<html>
<head><style type="text/css">
p.foo { display: block !important }
</style><body>
<p class="foo bar" style="display:none">Hello, world!</p>
</body></html>

Upstreaming http://code.google.com/p/chromium/issues/detail?id=144209
Comment 1 Alexander Pavlov (apavlov) 2012-08-24 07:35:18 PDT
Created attachment 160421 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2012-08-24 08:57:07 PDT
Created attachment 160429 [details]
Patch
Comment 3 Vsevolod Vlasov 2012-08-24 09:59:08 PDT
Comment on attachment 160429 [details]
Patch

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

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:521
> +            styleRule.importantProperties = {};

This could be a local variable, see below.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:535
> +                if (!isImportant && usedProperties.hasOwnProperty(canonicalName))

I would add    || foundImportantProperties.hasOwnProperty(canonicalName) to the condition so that we don't need to store styleRule.importantProperties for the next loop iterations.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:549
> +                for (var j = 0; j < i; ++j) {

As discussed we can avoid inner loop by using a map to remember the only rule from which this particular property is used.
Comment 4 Vsevolod Vlasov 2012-08-24 09:59:57 PDT
Please update bug title with a more generic description of this bug.
Comment 5 Alexander Pavlov (apavlov) 2012-08-24 10:11:20 PDT
Comment on attachment 160429 [details]
Patch

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

>> Source/WebCore/inspector/front-end/StylesSidebarPane.js:535
>> +                if (!isImportant && usedProperties.hasOwnProperty(canonicalName))
> 
> I would add    || foundImportantProperties.hasOwnProperty(canonicalName) to the condition so that we don't need to store styleRule.importantProperties for the next loop iterations.

This check is done a lot earlier, on the line 529 above. We still need to store importantProperties unless we get a map in, as discussed.

>> Source/WebCore/inspector/front-end/StylesSidebarPane.js:549
>> +                for (var j = 0; j < i; ++j) {
> 
> As discussed we can avoid inner loop by using a map to remember the only rule from which this particular property is used.

Yes, will fix.
Comment 6 Alexander Pavlov (apavlov) 2012-08-27 01:50:46 PDT
Created attachment 160670 [details]
Patch
Comment 7 Vsevolod Vlasov 2012-08-27 02:32:49 PDT
Comment on attachment 160670 [details]
Patch

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

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:528
> +                // No need to check any further if we know an !important value has already been seen.

I don't think this comment is needed.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:534
> +                // Property is not important in this rule and has encountered earlier - do nothing.

I don't think this comment is needed.
Comment 8 Alexander Pavlov (apavlov) 2012-08-27 02:35:51 PDT
Committed r126737: <http://trac.webkit.org/changeset/126737>