Bug 177684

Summary: Cannot unset transition with important
Product: WebKit Reporter: Justin Ridgewell <jridgewell>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, ews-watchlist, fred.wang, graouts, jonlee, koivisto, rbuis, rwlbuis, simon.fraser, webkit-bug-importer, youennf, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: Unspecified   
URL: https://w3c-test.org/css/cssom/cssom-setProperty-shorthand.html
See Also: https://github.com/w3c/web-platform-tests/pull/11047
Bug Depends on: 185727    
Bug Blocks:    
Attachments:
Description Flags
unset transition important breaks
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch none

Description Justin Ridgewell 2017-09-29 14:25:45 PDT
Created attachment 322231 [details]
unset transition important breaks

Setting `transition: none !important` (or any valid transition value) using inline styles (CSSStyleDeclaration#setProperty) prevents directly unsetting the style (`el.style.transition = ''`).

A workaround is to set a valid non-important style first, then unset (`el.style.transition = 'none', el.style.transition = ''`)
Comment 1 Radar WebKit Bug Importer 2017-10-03 01:02:34 PDT
<rdar://problem/34785714>
Comment 2 Justin Ridgewell 2018-05-08 13:31:22 PDT
Also noticed that you can't use CSSStyleDeclaration.removeProperty to unset it, either.
It also returns "", instead of the old value.
Comment 3 Rob Buis 2018-05-17 02:00:06 PDT
Created attachment 340565 [details]
Patch
Comment 4 EWS Watchlist 2018-05-17 02:01:14 PDT
Attachment 340565 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Rob Buis 2018-05-17 05:10:42 PDT
Created attachment 340575 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2018-05-17 05:32:35 PDT
Comment on attachment 340575 [details]
Patch

Thanks, LGTM!

I was checking WPT and Chromium repositories and it seems they have the same "cssom-remove-shorthand-property.html" as in WebKit: 

https://github.com/w3c/web-platform-tests/blob/master/css/cssom/cssom-setProperty-shorthand.html 
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css/cssom-remove-shorthand-property.html

So I would instead propose to check the !important case in the same WPT file and remove the duplicate from the WebKit/Chromium repository.
Comment 7 Frédéric Wang (:fredw) 2018-05-17 05:33:41 PDT
Comment on attachment 340575 [details]
Patch

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

> LayoutTests/fast/css/cssom-remove-important-shorthand-property.html:45
> +description("Tests that shorthand properties can be removed via CSSOM.");

Mmh, my comment was lost in the previous review... I wanted to say that you should mention !important in the desc.
Comment 8 Rob Buis 2018-05-17 11:26:31 PDT
Created attachment 340609 [details]
Patch
Comment 9 Chris Dumez 2018-05-17 11:31:59 PDT
Comment on attachment 340609 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:1
> +2018-05-17  Rob Buis  <rbuis@igalia.com>

Rob with an Igalia email. Can believe I missed that :)
Comment 10 Chris Dumez 2018-05-17 11:32:37 PDT
(In reply to Chris Dumez from comment #9)
> Comment on attachment 340609 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340609&action=review
> 
> r=me
> 
> > Source/WebCore/ChangeLog:1
> > +2018-05-17  Rob Buis  <rbuis@igalia.com>
> 
> Rob with an Igalia email. Can believe I missed that :)

*Cannot* believe..
Comment 11 Rob Buis 2018-05-17 11:45:09 PDT
(In reply to Chris Dumez from comment #10)
> (In reply to Chris Dumez from comment #9)
> > Comment on attachment 340609 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=340609&action=review
> > 
> > r=me

Thanks!

> > > Source/WebCore/ChangeLog:1
> > > +2018-05-17  Rob Buis  <rbuis@igalia.com>
> > 
> > Rob with an Igalia email. Can believe I missed that :)
> 
> *Cannot* believe..

You better believe it! :) Since the start of the month and I am slowly getting back into the swing of things...
Comment 12 EWS Watchlist 2018-05-17 14:14:08 PDT
Comment on attachment 340609 [details]
Patch

Attachment 340609 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7713981

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 13 EWS Watchlist 2018-05-17 14:14:19 PDT
Created attachment 340641 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 14 Rob Buis 2018-05-17 23:21:55 PDT
Created attachment 340681 [details]
Patch
Comment 15 EWS Watchlist 2018-05-18 11:59:47 PDT
Comment on attachment 340681 [details]
Patch

Rejecting attachment 340681 [details] from review queue.

rbuis@igalia.com does not have reviewer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 16 EWS Watchlist 2018-05-18 12:00:20 PDT
Comment on attachment 340681 [details]
Patch

Rejecting attachment 340681 [details] from commit-queue.

rbuis@igalia.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 17 WebKit Commit Bot 2018-05-18 18:51:03 PDT
Comment on attachment 340681 [details]
Patch

Rejecting attachment 340681 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 340681, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/7728288
Comment 18 WebKit Commit Bot 2018-05-18 21:05:11 PDT
Comment on attachment 340681 [details]
Patch

Clearing flags on attachment: 340681

Committed r231995: <https://trac.webkit.org/changeset/231995>
Comment 19 WebKit Commit Bot 2018-05-18 21:05:13 PDT
All reviewed patches have been landed.  Closing bug.