Bug 81737 – cssText should use shorthand notations

• Component: CSS
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 132987 [details]
work in progress

[Ryosuke Niwa]

Created attachment 133169 [details]
work in progress 2

There appear to be some regressions in copy & paste.

[Ryosuke Niwa]

Created attachment 133205 [details]
work in progress 3

I just need a test now.

[WebKit Review Bot]

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]

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

[Ryosuke Niwa]

Created attachment 133337 [details]
work in progress 4

Enable shorthands for all cases.

[WebKit Review Bot]

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]

Created attachment 133453 [details]
work in progress 5

[Ryosuke Niwa]

Created attachment 133590 [details]
Patch

[Early Warning System Bot]

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 on attachment 133590 [details]
Patch

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

[Gyuyoung Kim]

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 on attachment 133590 [details]
Patch

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

[Build Bot]

Comment on attachment 133590 [details]
Patch

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

[Ryosuke Niwa]

Created attachment 133596 [details]
Added the missing BitVector change

[WebKit Review Bot]

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 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 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]

Created attachment 133601 [details]
Another build fix

[WebKit Review Bot]

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 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 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]

(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 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 on attachment 133601 [details]
Another build fix

Please fix the build breaks. The rest of the patch looks good.

[Ryosuke Niwa]

Created attachment 133909 [details]
Fixed win build

[Enrica Casucci]

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 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 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]

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

(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]

(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]

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