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)
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)
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
41 bytes,
text/x-github-pull-request
|
Details | Review |
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.
Updated•7 years ago
|
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
status-firefox56:
--- → unaffected
status-firefox57:
--- → affected
status-firefox58:
--- → affected
Ever confirmed: true
Comment 2•7 years ago
|
||
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
Comment 3•7 years ago
|
||
This does look like a bug. Xidorn, could you take a look at this?
Flags: needinfo?(cam) → needinfo?(xidorn+moz)
Assignee | ||
Comment 4•7 years ago
|
||
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
Comment 5•7 years ago
|
||
(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)
Assignee | ||
Comment 6•7 years ago
|
||
Hmm, Gecko does include logical props, it just includes it _before_ the non-logical version... And that matches Blink's behavior I think... Sigh
Assignee | ||
Comment 7•7 years ago
|
||
(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.
Assignee | ||
Comment 8•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Updated•7 years ago
|
See Also: → https://github.com/w3c/csswg-drafts/issues/1898
Assignee | ||
Comment 11•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 14•7 years ago
|
||
Comment 15•7 years ago
|
||
Emilio, will we want to uplift this fix to Beta 57?
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(emilio)
Assignee | ||
Comment 16•7 years ago
|
||
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?
Comment 17•7 years ago
|
||
[Tracking Requested - why for this release]: Emilio says this Stylo bug is a potential web-compat issue.
tracking-firefox57:
--- → ?
Comment 18•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0896f475d15 Add reftest. r=me
Assignee | ||
Comment 19•7 years ago
|
||
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+
Updated•7 years ago
|
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d0896f475d15
Comment 22•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/213357b2f20d https://hg.mozilla.org/releases/mozilla-beta/rev/c11525bd0ab2
Comment 23•7 years ago
|
||
(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-
You need to log in
before you can comment on or make changes to this bug.
Description
•