Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(391)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1094003002: CSSStyleDeclaration::setProperty should override important style (Closed)

Created:
5 years, 8 months ago by sashab
Modified:
5 years, 8 months ago
Reviewers:
Timothy Loh
CC:
blink-reviews, blink-reviews-style_chromium.org, blink-reviews-css, ed+blinkwatch_opera.com, dglazkov+blink, apavlov+blink_chromium.org, darktears, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

CSSStyleDeclaration::setProperty should override important style This patch changes CSSStyleDeclaration::setProperty to override any styles set with !important, and clear the !important flag if not set for that property in the inline style. BUG=331236 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194469

Patch Set 1 #

Patch Set 2 : Removed important declaration and added test #

Patch Set 3 : Rebase #

Patch Set 4 : Rebase and added doctype to test #

Total comments: 1

Patch Set 5 : Rebase again #

Total comments: 2

Patch Set 6 : Removed a bunch of unnecessary stuff #

Total comments: 1

Patch Set 7 : Renamed addParsedProperty to addRespectingCascade #

Total comments: 4

Patch Set 8 : Review feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+83 lines, -14 lines) Patch
M LayoutTests/fast/css/important-js-override.html View 1 2 3 4 5 6 7 1 chunk +76 lines, -6 lines 0 comments Download
D LayoutTests/fast/css/important-js-override-expected.txt View 1 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/css/StylePropertySet.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/StylePropertySet.cpp View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/css/parser/CSSParser.cpp View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/resolver/ViewportStyleResolver.cpp View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/editing/EditingStyle.cpp View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 12 (2 generated)
sashab
https://codereview.chromium.org/1094003002/diff/60001/Source/core/css/StylePropertySet.cpp File Source/core/css/StylePropertySet.cpp (right): https://codereview.chromium.org/1094003002/diff/60001/Source/core/css/StylePropertySet.cpp#newcode272 Source/core/css/StylePropertySet.cpp:272: unsigned getIndexInShorthandVectorForPrefixingVariant(const CSSProperty& property, CSSPropertyID prefixingVariant) This function existed ...
5 years, 8 months ago (2015-04-23 05:04:32 UTC) #2
Timothy Loh
https://codereview.chromium.org/1094003002/diff/80001/Source/core/css/StylePropertySet.cpp File Source/core/css/StylePropertySet.cpp (right): https://codereview.chromium.org/1094003002/diff/80001/Source/core/css/StylePropertySet.cpp#newcode291 Source/core/css/StylePropertySet.cpp:291: *toReplace = CSSProperty(property.id(), property.value(), false, property.isSetFromShorthand(), getIndexInShorthandVector(property), property.metadata().m_implicit); Isn't ...
5 years, 8 months ago (2015-04-23 05:42:03 UTC) #3
sashab
Oops; thanks. That makes this patch much smaller. :) https://codereview.chromium.org/1094003002/diff/80001/Source/core/css/StylePropertySet.cpp File Source/core/css/StylePropertySet.cpp (right): https://codereview.chromium.org/1094003002/diff/80001/Source/core/css/StylePropertySet.cpp#newcode291 Source/core/css/StylePropertySet.cpp:291: ...
5 years, 8 months ago (2015-04-23 05:59:05 UTC) #4
Timothy Loh
https://codereview.chromium.org/1094003002/diff/100001/Source/core/css/StylePropertySet.cpp File Source/core/css/StylePropertySet.cpp (right): https://codereview.chromium.org/1094003002/diff/100001/Source/core/css/StylePropertySet.cpp#newcode315 Source/core/css/StylePropertySet.cpp:315: bool MutableStylePropertySet::addParsedProperty(const CSSProperty& property) Other functions call this and ...
5 years, 8 months ago (2015-04-24 00:35:05 UTC) #5
sashab
Good find. Take a look at what I've done and let me know what you ...
5 years, 8 months ago (2015-04-24 05:50:45 UTC) #6
Timothy Loh
You can probably be a bit clearer with the patch title/description, e.g. "CSSStyleDeclaration::setProperty should override ...
5 years, 8 months ago (2015-04-24 09:21:27 UTC) #7
sashab
https://codereview.chromium.org/1094003002/diff/120001/LayoutTests/fast/css/important-js-override.html File LayoutTests/fast/css/important-js-override.html (right): https://codereview.chromium.org/1094003002/diff/120001/LayoutTests/fast/css/important-js-override.html#newcode6 LayoutTests/fast/css/important-js-override.html:6: <script> On 2015/04/24 09:21:27, Timothy Loh wrote: > Should ...
5 years, 8 months ago (2015-04-26 23:23:41 UTC) #8
Timothy Loh
lgtm
5 years, 8 months ago (2015-04-27 00:27:02 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1094003002/140001
5 years, 8 months ago (2015-04-27 00:31:27 UTC) #11
commit-bot: I haz the power
5 years, 8 months ago (2015-04-27 00:35:17 UTC) #12
Message was sent while issue was closed. 
Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194469

Powered by Google App Engine
This is Rietveld 408576698