Closed Bug 1410028 Opened 7 years ago Closed 7 years ago

stylo: all property with `initial` issues when style properties set within JS

Categories

(Core :: CSS Parsing and Computation, defect, P2)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: adomas.ven, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

Inject an element with JS into a page and set the style on the element using element.style object, where the first property was `all: initial`.

See fiddle (https://jsfiddle.net/8w9ud1f4/4/), compare results on FX 56 (correct) and FX 57 (wrong).

I suspect this affects anyone using front-end frameworks if they use `all: initial`.


Actual results:

Logical css properties like `padding-inline-start: initial` overrode `padding: 0`.


Expected results:

See fiddle.
Component: Untriaged → Layout
Keywords: regression
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Regressed by Stylo enabling in bug 1330412.
heycam, do you see any insights here?
Blocks: stylo
Component: Layout → CSS Parsing and Computation
Flags: needinfo?(cam)
Priority: -- → P2
Summary: all: initial issues when style properties set within JS → stylo: all property with `initial` issues when style properties set within JS
This does look like a bug.  Xidorn, could you take a look at this?
Flags: needinfo?(cam) → needinfo?(xidorn+moz)
This is not really a bug, but an incompatibility in which declarations we generate for the `all` longhand. I can take, should be trivial, but we should probably also get this specced.

For example in Blink if you change the 'padding': '0px 8px' declaration for 'padding-inline-start': '10px', they don't honor it, while FF does...

I think we just shouldn't include logical props in the 'all' longhand.
Assignee: nobody → emilio
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> This is not really a bug, but an incompatibility in which declarations we
> generate for the `all` longhand. I can take, should be trivial, but we
> should probably also get this specced.
> 
> For example in Blink if you change the 'padding': '0px 8px' declaration for
> 'padding-inline-start': '10px', they don't honor it, while FF does...
> 
> I think we just shouldn't include logical props in the 'all' longhand.

Thanks for taking it.

But I don't understand. Do you mean we are generating logical props from all shorthand? In that case, shouldn't the declaration added later be placed later in the declaration block and thus override the logical props generated by all shorthand? Or maybe we don't append but rather we replace existing declarations in the block, which means different order by all shorthand would produce different result?
Flags: needinfo?(xidorn+moz)
Hmm, Gecko does include logical props, it just includes it _before_ the non-logical version... And that matches Blink's behavior I think... Sigh
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #5)
> But I don't understand. Do you mean we are generating logical props from all
> shorthand? In that case, shouldn't the declaration added later be placed
> later in the declaration block and thus override the logical props generated
> by all shorthand? Or maybe we don't append but rather we replace existing
> declarations in the block, which means different order by all shorthand
> would produce different result?

Not for CSSOM. In CSSOM the slot is reused, and this matches the spec, so it depends on the order we output those declarations which ones are used... I still think logical props shouldn't be generated by `all`, because that makes behavior inconsistent.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> (In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16)
> from comment #5)
> > But I don't understand. Do you mean we are generating logical props from all
> > shorthand? In that case, shouldn't the declaration added later be placed
> > later in the declaration block and thus override the logical props generated
> > by all shorthand? Or maybe we don't append but rather we replace existing
> > declarations in the block, which means different order by all shorthand
> > would produce different result?
> 
> Not for CSSOM. In CSSOM the slot is reused, and this matches the spec, so it
> depends on the order we output those declarations which ones are used... I
> still think logical props shouldn't be generated by `all`, because that
> makes behavior inconsistent.

i.e., you're right this is because of the order of the properties.
Comment on attachment 8920953 [details]
Bug 1410028: Ensure logical longhands appear before their physical counter-part.

https://reviewboard.mozilla.org/r/191918/#review197094

I don't think this is correct. It still seems to be a behavior change, e.g. if someone has

```javascript
e.style.paddingLeft = "10px";
e.style.paddingInlineStart = "10px";
e.style.all = "initial";
```

It seems that the padding all be `initial` now, but it's actually not, because `all` doesn't reset `padding-inline-start`.

If there is some order in the current impl, can we use the same order in Stylo somehow?
Attachment #8920953 - Flags: review?(xidorn+moz) → review-
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive Nov 5 ~ Dec 16) from comment #10)
> Comment on attachment 8920953 [details]
> Bug 1410028: Don't make the all property set logical properties.
> 
> https://reviewboard.mozilla.org/r/191918/#review197094
> 
> I don't think this is correct. It still seems to be a behavior change, e.g.
> if someone has

I'm well aware this is a change in behavior, technically. This seemed like preferrable to me, but you're right I didn't account for something like that... I guess you're right, and we just need to get the order properly defined.
Comment on attachment 8920953 [details]
Bug 1410028: Ensure logical longhands appear before their physical counter-part.

https://reviewboard.mozilla.org/r/191918/#review197104

OK I guess this is fine.

Could you add a mochitest in the Gecko side (for now) to check the behavior?

We may want to add a new web platform test once CSSWG decides an expected behavior on this.
Attachment #8920953 - Flags: review?(xidorn+moz) → review+
Attached file Servo bits
Emilio, will we want to uplift this fix to Beta 57?
Flags: needinfo?(emilio)
Comment on attachment 8920953 [details]
Bug 1410028: Ensure logical longhands appear before their physical counter-part.

This hasn't landed on trunk yet because of bug 1410775, but yes.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: Potential web-compat issue.
[Is this code covered by automated tests?]: Yes (have one ready to land as soon as bug 1410775 is resolved).
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very risky
[Why is the change risky/not risky?]: very isolated in the handling of the `all` property which aligns us to other browsers.
[String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8920953 - Flags: approval-mozilla-beta?
[Tracking Requested - why for this release]:

Emilio says this Stylo bug is a potential web-compat issue.
Whoops, this landed and test-case works on current nightly.

Just pushed a reftest so it's tested.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 8920953 [details]
Bug 1410028: Ensure logical longhands appear before their physical counter-part.

Stylo web-compat issue, beta57+
Attachment #8920953 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+
Target Milestone: --- → mozilla58
(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> [Is this code covered by automated tests?]: Yes (have one ready to land as
> soon as bug 1410775 is resolved).
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
See Also: → 1415330
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: