other values between background-clip and background-origin should allowed in background shorthand

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: Gérard Talbot, Assigned: xidorn)

Tracking

(Blocks: 1 bug, {css3, testcase})

Trunk
mozilla55
css3, testcase
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Test
----

http://www.gtalbot.org/BrowserBugsSection/CSS3Backgrounds/Unbound-background-clip-from-shorthand.html

original author is Dirk Schulze


Actual results
--------------

These 9 background shorthand tests fail:

Testing: "background: content-box none repeat scroll 0% 0% / auto padding-box"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.

Testing: "background: none content-box repeat scroll 0% 0% / auto padding-box"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.

Testing: "background: none repeat scroll content-box 0% 0% / auto padding-box"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.

Testing: "background: content-box none padding-box repeat scroll 0% 0% / auto"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.

Testing: "background: content-box none repeat padding-box scroll 0% 0% / auto"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.

Testing: "background: content-box none repeat scroll padding-box 0% 0% / auto"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.

Testing: "background: none content-box repeat padding-box scroll 0% 0% / auto"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.

Testing: "background: none content-box repeat scroll padding-box 0% 0% / auto"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.

Testing: "background: none repeat content-box scroll padding-box 0% 0% / auto"
Failed expected background-origin: 'content-box'; actual background-origin: 'padding-box'.
Failed expected background-clip: 'padding-box'; actual background-clip: 'border-box'.


Explanation
-----------

<bg-layer> = <bg-image> || <position> [ / <bg-size> ]? || <repeat-style> || <attachment> || <box> || <box>

CSS3 Backgrounds and Borders, section 3.10. Backgrounds Shorthand: the ‘background’ property
http://www.w3.org/TR/css3-background/#the-background

"
A double bar (||) separates two or more options: one or more of them must occur, in any order.
"
http://www.w3.org/TR/CSS21/about.html#value-defs

"
If one <box> value is present then it sets both ‘background-origin’ and ‘background-clip’ to that value. If two values are present, then the first sets ‘background-origin’ and the second ‘background-clip’.
"
3.10. Backgrounds Shorthand: the ‘background’ property
http://www.w3.org/TR/css3-background/#the-background

The provided test is testing situation when/where there are 2 <box> values and when their respective order is different in the background shorthand.


More info
---------

- The provided test could be split into 9 separate, distinct tests with an expected rendered layout
- Firefox 39 and Firefox 42.0a1 buildID=20150718201542 fail this test
- IE11, Chrome 44.0.2403.107 and Opera 12.16 pass this test
- Re: [css3-background] unbound background-clip from -origin in background shorthand
http://lists.w3.org/Archives/Public/www-style/2012Nov/0542.html
- I've searched for duplicates and could not find one... although bug 570896 must be related
(Reporter)

Updated

3 years ago
Keywords: css3, testcase
Summary: Independent respective order of background-origin and of background-clip in background shorthand → other values between background-clip and background-origin should allowed in background shorthand
Gecko needs to fix this to have same behavior as stylo.
Blocks: 1321197
I have a feeling that how we fix this may affect other things, specifically, how "text" is allowed in background shorthand.

Currently, Gecko allows "text" to present in background shorthand if it immediately follows another <box> value. The spec isn't clear on this, because background-clip: text is added in css-backgrounds-4 as an independent value of background-clip (not a value of <box>), while in css-backgrounds-3, the <bg-layer> just use "<box> || <box>" for this.

I don't think it makes sense to add "text" to <box>, so if the spec wants to add "text" to background shorthand, it would need to do something like "<box> || [ <box> | text ]", which also means the order of clip and origin can be reversed if one use text for clip. I don't know whether that would be a problem.

FWIW, Blink doesn't support "text" in background shorthand, and Edge supports it but requires the order of origin and clip even if clip is "text".

I suggest we remove the support of "text" in background shorthand when we fix this bug, and we can always add it back when the spec becomes clear.
Assignee: nobody → xidorn+moz
Ok, so I'm now looking at the code, and it seems css-masking does use "<geometry-box> || [ <geometry-box> | no-clip ]" so I suppose css-backgrounds is going to do the same thing. Then it is probably easier to just support "text" and allow it to come before background-origin.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

8 months ago
mozreview-review
Comment on attachment 8871144 [details]
Bug 1188074 part 1 - Refactor ParseImageLayersItem a bit to check whether a slot is filled before checking whether the keyword matches.

https://reviewboard.mozilla.org/r/142660/#review146334

Yeah, seems better for it to be a bit slower when the value has a syntax error due to repeated values, than to keep looking up the keywords for already filled slots.
Attachment #8871144 - Flags: review?(cam) → review+

Comment 10

8 months ago
mozreview-review
Comment on attachment 8871145 [details]
Bug 1188074 part 2 - Allow origin and clip values to appear anywhere in an image layer shorthand property.

https://reviewboard.mozilla.org/r/142662/#review146338

::: commit-message-b31d4:1
(Diff revision 2)
> +Bug 1188074 part 2 - Implement the combine any syntax for image layer values. r?heycam

I'm having trouble parsing this.  Maybe "Allow origin and clip values to appear anywhere in an image layer shorthand property."?

::: layout/style/nsCSSParser.cpp:12399
(Diff revision 2)
>  #ifdef DEBUG
> -        const KTableEntry* clipTable = nsCSSProps::kKeywordTableTable[aTable[nsStyleImageLayers::clip]];
> -        for (size_t i = 0; originTable[i].mValue != -1; i++) {
> +          for (size_t i = 0; originTable[i].mValue != -1; i++) {
> -          // For each keyword & value in kOriginKTable, ensure that
> +            // For each keyword & value in kOriginKTable, ensure that
> -          // kBackgroundKTable has a matching entry at the same position.
> +            // kBackgroundKTable has a matching entry at the same position.
> -          MOZ_ASSERT(originTable[i].mKeyword == clipTable[i].mKeyword);
> +            MOZ_ASSERT(originTable[i].mKeyword == clipTable[i].mKeyword);
> -          MOZ_ASSERT(originTable[i].mValue == clipTable[i].mValue);
> +            MOZ_ASSERT(originTable[i].mValue == clipTable[i].mValue);
> -        }
> +          }
>  #endif

Maybe we should move these assertions to the top of the function, just after we declare originTable and clipTable, so that it's not so distracting here.  And just comment that we've asserted above the values are compatible.
Attachment #8871145 - Flags: review?(cam) → review+
(Assignee)

Comment 11

8 months ago
mozreview-review-reply
Comment on attachment 8871145 [details]
Bug 1188074 part 2 - Allow origin and clip values to appear anywhere in an image layer shorthand property.

https://reviewboard.mozilla.org/r/142662/#review146338

> I'm having trouble parsing this.  Maybe "Allow origin and clip values to appear anywhere in an image layer shorthand property."?

I actually had trouble parsing this myself... but I didn't figure out a much better commit message so I just left it as is. Sorry about that.

> Maybe we should move these assertions to the top of the function, just after we declare originTable and clipTable, so that it's not so distracting here.  And just comment that we've asserted above the values are compatible.

I tried moving it to the top of the function, but later I moved it back because I think this should be put near where we actually want the assertion, so that it is clear that the assertion is related to this code, and when someone wants to refactor it, they know it should or should not be removed as well.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Servo side: servo/servo#17035

They don't really need coordinated landing... that would just cause some increasing of mochitest failures...

Comment 15

8 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/934aae5e56b2
part 1 - Refactor ParseImageLayersItem a bit to check whether a slot is filled before checking whether the keyword matches. r=heycam
https://hg.mozilla.org/integration/autoland/rev/abcf45c9ad66
part 2 - Allow origin and clip values to appear anywhere in an image layer shorthand property. r=heycam

Comment 16

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/934aae5e56b2
https://hg.mozilla.org/mozilla-central/rev/abcf45c9ad66
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1373750
No longer depends on: 1373750
You need to log in before you can comment on or make changes to this bug.