Bug 81737

Summary:

cssText should use shorthand notations

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

CSS

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dbates, dglazkov, enrica, krit, macpherson, max.hong.shen, menard, mitz, ojan, sullivan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

82012, 82028, 82040, 185953

Bug Blocks:

12319, 73967, 82364

Attachments:

Description	Flags
work in progress	none
work in progress 2	none
work in progress 3	none
work in progress 4	webkit.review.bot: commit-queue-
work in progress 5	none
Patch	none
Added the missing BitVector change	none
Another build fix	none
Fixed win build	enrica: review+, webkit.review.bot: commit-queue-

Ryosuke Niwa

Reported 2012-03-20 21:33:11 PDT

Takes an example from 12319. Copying some text from TextEdit in the rich format mode results in verbose inline styles such as: <p style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font-family: Helvetica; font-size: 12px; ">this is line one</p> It could be <p style="margin: 0px; font-family: Helvetica; font-size: 12px; ">this is line one</p> instead.

Attachments
work in progress (11.42 KB, patch) 2012-03-21 01:14 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
work in progress 2 (16.11 KB, patch) 2012-03-21 19:25 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
work in progress 3 (18.90 KB, patch) 2012-03-22 01:32 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
work in progress 4 (9.67 KB, patch) 2012-03-22 13:41 PDT, Ryosuke Niwa	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
work in progress 5 (44.14 KB, patch) 2012-03-23 03:49 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch (32.02 KB, patch) 2012-03-23 17:17 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Added the missing BitVector change (33.20 KB, patch) 2012-03-23 17:36 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Another build fix (33.21 KB, patch) 2012-03-23 17:58 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixed win build (34.29 KB, patch) 2012-03-26 15:41 PDT, Ryosuke Niwa	enrica: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2012-03-21 01:14:10 PDT

Created attachment 132987 [details] work in progress

Ryosuke Niwa

Comment 2 2012-03-21 19:25:23 PDT

Created attachment 133169 [details] work in progress 2 There appear to be some regressions in copy & paste.

Ryosuke Niwa

Comment 3 2012-03-22 01:32:04 PDT

Created attachment 133205 [details] work in progress 3 I just need a test now.

WebKit Review Bot

Comment 4 2012-03-22 01:34:47 PDT

Attachment 133205 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/editing/pasteboard/paste-and-s..." exit_code: 1 Source/WebCore/css/StylePropertySet.h:103: Missing spaces around = [whitespace/operators] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ryosuke Niwa

Comment 5 2012-03-22 11:49:12 PDT

It appears that we can do this for all cases. e.g. Firefox uses shorthand notations for cssText as well.

Ryosuke Niwa

Comment 6 2012-03-22 13:41:23 PDT

Created attachment 133337 [details] work in progress 4 Enable shorthands for all cases.

WebKit Review Bot

Comment 7 2012-03-22 15:24:27 PDT

Comment on attachment 133337 [details] work in progress 4 Attachment 133337 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12116275 New failing tests: editing/pasteboard/paste-and-sanitize.html inspector/styles/styles-update-from-js.html fast/css/remove-shorthand.html editing/style/non-inheritable-styles.html fast/css/uri-token-parsing.html printing/page-rule-css-text.html

Ryosuke Niwa

Comment 8 2012-03-23 03:49:54 PDT

Created attachment 133453 [details] work in progress 5

Ryosuke Niwa

Comment 9 2012-03-23 17:17:20 PDT

Created attachment 133590 [details] Patch

Early Warning System Bot

Comment 10 2012-03-23 17:20:13 PDT

Comment on attachment 133590 [details] Patch Attachment 133590 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12117885

Early Warning System Bot

Comment 11 2012-03-23 17:21:22 PDT

Comment on attachment 133590 [details] Patch Attachment 133590 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12117887

Gyuyoung Kim

Comment 12 2012-03-23 17:22:14 PDT

Comment on attachment 133590 [details] Patch Attachment 133590 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12122879

Build Bot

Comment 13 2012-03-23 17:24:08 PDT

Comment on attachment 133590 [details] Patch Attachment 133590 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12123847

Build Bot

Comment 14 2012-03-23 17:30:58 PDT

Comment on attachment 133590 [details] Patch Attachment 133590 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12116920

Ryosuke Niwa

Comment 15 2012-03-23 17:36:25 PDT

Created attachment 133596 [details] Added the missing BitVector change

WebKit Review Bot

Comment 16 2012-03-23 17:43:50 PDT

Comment on attachment 133596 [details] Added the missing BitVector change Attachment 133596 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12117902

Early Warning System Bot

Comment 17 2012-03-23 17:51:28 PDT

Comment on attachment 133596 [details] Added the missing BitVector change Attachment 133596 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12119007

Early Warning System Bot

Comment 18 2012-03-23 17:56:15 PDT

Comment on attachment 133596 [details] Added the missing BitVector change Attachment 133596 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12097058

Ryosuke Niwa

Comment 19 2012-03-23 17:58:07 PDT

Created attachment 133601 [details] Another build fix

WebKit Review Bot

Comment 20 2012-03-23 18:13:26 PDT

Comment on attachment 133601 [details] Another build fix Attachment 133601 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12122905

Build Bot

Comment 21 2012-03-23 18:30:10 PDT

Comment on attachment 133601 [details] Another build fix Attachment 133601 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12122912

Eric Seidel (no email)

Comment 22 2012-03-26 15:00:57 PDT

Comment on attachment 133601 [details] Another build fix View in context: https://bugs.webkit.org/attachment.cgi?id=133601&action=review Do we know what the perf effect of this change is? > Source/WebCore/css/StylePropertySet.cpp:730 > + // Don't use CSSPropertyFont because old UAs can't recognize them but are important for editing. Is this really still the case? :)

Ryosuke Niwa

Comment 23 2012-03-26 15:03:44 PDT

(In reply to comment #22) > (From update of attachment 133601 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133601&action=review > > Do we know what the perf effect of this change is? I don't think there's any perf. implications because we didn't even use StringBuilder before my patch for https://bugs.webkit.org/show_bug.cgi?id=82028 was landed. > > Source/WebCore/css/StylePropertySet.cpp:730 > > + // Don't use CSSPropertyFont because old UAs can't recognize them but are important for editing. > > Is this really still the case? :) Yes. Unfortunately, some old versions of mail clients support as little CSS as MSIE4 did :( We even used to generate font elements for a very old mail client that didn't support CSS at all.

Enrica Casucci

Comment 24 2012-03-26 15:29:53 PDT

Comment on attachment 133601 [details] Another build fix View in context: https://bugs.webkit.org/attachment.cgi?id=133601&action=review The patch looks good to me. Thanks for doing this :-) > Source/WTF/wtf/BitVector.h:170 > + Unnecessary empty line.

Enrica Casucci

Comment 25 2012-03-26 15:33:28 PDT

Comment on attachment 133601 [details] Another build fix Please fix the build breaks. The rest of the patch looks good.

Ryosuke Niwa

Comment 26 2012-03-26 15:41:35 PDT

Created attachment 133909 [details] Fixed win build

Enrica Casucci

Comment 27 2012-03-26 15:53:13 PDT

Comment on attachment 133909 [details] Fixed win build View in context: https://bugs.webkit.org/attachment.cgi?id=133909&action=review r+ with the Linux build fix and the ChangeLog entry. > Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def:302 > ?retrieveCallerFromVMCode@Interpreter@JSC@@QBE?AVJSValue@2@PAVExecState@2@PAVJSFunction@2@@Z You need a ChangeLog entry for this change.

WebKit Review Bot

Comment 28 2012-03-26 16:38:35 PDT

Comment on attachment 133909 [details] Fixed win build Attachment 133909 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12145156

Ryosuke Niwa

Comment 29 2012-03-26 16:50:42 PDT

Comment on attachment 133909 [details] Fixed win build View in context: https://bugs.webkit.org/attachment.cgi?id=133909&action=review > Source/WebCore/css/StylePropertySet.cpp:777 > + case CSSPropertyWebkitMaskPositionX: > + case CSSPropertyWebkitMaskPositionY: > + shorthandPropertyID = CSSPropertyWebkitMaskPosition; > + break; > + case CSSPropertyWebkitMaskRepeatX: > + case CSSPropertyWebkitMaskRepeatY: > + shorthandPropertyID = CSSPropertyWebkitMaskRepeat; Huh, these properties need to be merged with the following properties for CSSPropertyWebkitMask. It's causing LayoutTests/fast/css/remove-shorthand.html to fail now. It seems like something has changed since last week :\

Ryosuke Niwa

Comment 30 2012-03-26 16:59:37 PDT

Committed r112177: <http://trac.webkit.org/changeset/112177>

mitz

Comment 31 2012-03-26 18:22:48 PDT

(In reply to comment #30) > Committed r112177: <http://trac.webkit.org/changeset/112177> Is this what caused fast/dom/css-dom-read-2.html to fail?

Ryosuke Niwa

Comment 32 2012-03-26 18:27:41 PDT

(In reply to comment #31) > (In reply to comment #30) > > Committed r112177: <http://trac.webkit.org/changeset/112177> > > Is this what caused fast/dom/css-dom-read-2.html to fail? So it seems. Will rebaseline. It's odd that I didn't see this failure locally last week :\

Ryosuke Niwa

Comment 33 2012-03-26 18:32:46 PDT

Chromium build fix landed in http://trac.webkit.org/changeset/112185, and rebaselined the test in http://trac.webkit.org/changeset/112188.

Note You need to log in before you can comment on or make changes to this bug.