1134171 - Improve seralization of 'background' shorthand

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P5)

Description

[Sebastian Zartner [:sebo]]

Attachment: test.html

Serializing the background shorthand is too verbose. It should just return the authored longhands.

For the attached test case the expected output is:

#test1 { background: yellow; }
#test2 { width: 16px; height: 16px; background: url(http://mozilla.org/favicon.ico) no-repeat; }

What you currently get is this:

#test1 { background: yellow none repeat scroll 0% 0%; }
#test2 { width: 16px; height: 16px; background: transparent url("http://mozilla.org/favicon.ico") no-repeat scroll 0% 0%; }

Note that Webkit, Blink and Trident already do a better job here.

Sebastian

Comment 1

[Sebastian Zartner [:sebo]]

I've just tested this again in Firefox 62.0 and the issue persists.

Sebastian

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Should be very easy to fix, the hard part is figuring out what do other browsers do when multiple backgrounds are specified and see if it makes sense.

Relevant code is https://searchfox.org/mozilla-central/rev/6201a9e0067cf6af118c6a99ae9314b8ceb2c4d5/servo/components/style/properties/shorthands/background.mako.rs#136

Comment 3

[Ya'ar Hever]

I would like to pick up this bug. 

I'm guessing that the approach here would be to compare the longhand properties against the default values of the matching shorthand property and omit the ones that have their default values. Does that make sense?

How does it go with regards to the Servo repo in github vs. the Servo in mozilla-central? Am I right that the one in mozilla-central is only a part of the full Servo? Should patches be sent as pull-requests in github or via phabricator (or both)?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Phabricator is fine, I sync them back and forth periodically.

Comment 6

[Shobhit Chittora]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Phabricator is fine, I sync them back and forth periodically.

Hey Emilio!
If this is still an open issue, I'd like to pick this up and close this. Do let me know on this.

Comment 7

[wangchao719]

I can pick up this one as "first bug"

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Sure, please ask any concrete questions you may have.

What we need to do is to:

• Check whether other browsers behave sanely when you have multiple backgrounds.
• Check for initial values like we do here for most of the longhands.

Comment 9

[wangchao719]

Hi Emilio,
Thanks for reply.

I have Tested this page

Firefox(default):

#test1 { background: yellow none repeat scroll 0% 0%; }
#test2 { width: 16px; height: 16px; background: rgba(0, 0, 0, 0) url("http://mozilla.org/favicon.ico") no-repeat scroll 0% 0%; }

Chrome:

#test1 { background: yellow; }
#test2 { width: 16px; height: 16px; background: url("http://mozilla.org/favicon.ico") no-repeat; }

Safari:

#test1 { background-color: yellow; background-position: initial initial; background-repeat: initial initial; }
#test2 { width: 16px; height: 16px; background-image: url("http://mozilla.org/favicon.ico"); background-position: initial initial; background-repeat: no-repeat no-repeat; }

Here is some question:

• Q1: Seems Chrome's result is OK, should we switch to that?
• Q2: Should we change both servo and Firefox code?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Q1: Seems Chrome's result is OK, should we switch to that?

That doesn't test multiple backgrounds, but yeah, Chrome has a better serialization in this case. Safari is just wrong, it shouldn't use initial.

Q2: Should we change both servo and Firefox code?

Just change Firefox (though the code is under servo/). I take care of syncing the changes to Servo from time to time (see this PR for the last one). I preserve committer attribution so it should be the same, I just do the legwork of keeping both builds because it's easier for me to do it once that for everyone that submits a patch to do the same every time :)

Comment 11

[wangchao719]

Attachment: 1134171-multiple.html

Comment 12

[wangchao719]

Firefox

#test1 { background: yellow none repeat scroll 0% 0%; }
#test2 { width: 16px; height: 16px; background: url("http://mozilla.org/favicon.ico") no-repeat scroll 0% 0%, rgba(0, 0, 0, 0) url("http://mozilla.org/favicon.ico") no-repeat scroll 0% 0%; }

Chrome

#test1 { background: yellow; }
#test2 { width: 16px; height: 16px; background: url("http://mozilla.org/favicon.ico") no-repeat, url("http://mozilla.org/favicon.ico") no-repeat; }

Seems not change much from single background,
We should just match up Chrome result right?

Comment 13

[shnmorimoto]

Hi, wangchao719.
Are you still working on this?
If not, Can I pick up this?

Comment 14

[wangchao719]

(In reply to shnmorimoto from comment #13)

Hi, wangchao719.
Are you still working on this?
If not, Can I pick up this?

not having progress for a while,
please feel free to go ahead

Comment 15

[shnmorimoto]

(In reply to wangchao719 from comment #14)

(In reply to shnmorimoto from comment #13)

Hi, wangchao719.
Are you still working on this?
If not, Can I pick up this?

not having progress for a while,
please feel free to go ahead

Thanks for your reply.
Got it. I will proceed this.

Comment 16

[Connor Pearson]

If nobody is actively working on this, I'd like to take a crack at it.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Please go ahead! Feel free to ask / request more information as needed.

Comment 18

[Connor Pearson]

So far I’ve gotten the example to behave as expected, but still need to add some tests.

Could you point me in the right direction? Is this something that should go in the web platform tests?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Yeah, the tests here should be in WPT. Feel free to submit the patch and I can run it through automation to see if any existing test needs updating. Otherwise https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-backgrounds/parsing/ seems like a reasonable place for it.

Comment 20

[Connor Pearson]

Attachment: Bug 1134171 - When serializing background shorthand skip initial values and order values according to grammar. r=emilio

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2ca38513bdf
When serializing background shorthand skip initial values and order values according to grammar. r=emilio

Comment 22

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35547 for changes under testing/web-platform/tests

Comment 23

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/d2ca38513bdf

Comment 24

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot