Closed
Bug 1380259
Opened 7 years ago
Closed 7 years ago
stylo: radial gradients are not serialized using modern unprefixed style
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | wontfix |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
People
(Reporter: ferjm, Assigned: ferjm)
References
Details
Attachments
(1 file, 2 obsolete files)
After bug 1367274 fix this is the only remaining failure in test_computed_style.html: 165 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_computed_style.html | computed value of prefixed gradient expression (-webkit-radial-gradient with legacy 'contain' keyword) - got "-webkit-radial-gradient(closest-side, rgb(255, 0, 0), rgb(0, 0, 255))", expected "radial-gradient(closest-side, rgb(255, 0, 0), rgb(0, 0, 255))"
Assignee | ||
Updated•7 years ago
|
Blocks: stylo, stylo-style-mochitest
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ferjmoreno
Comment 2•7 years ago
|
||
The one failure in test_specified_value_serialization.html is also the same reason. It's not clear to me what is the right way to serialize it... I guess either way is fine as far as we roundtrip.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8889826 [details] Bug 1380259 - update test expectations for test_computed_style.html. https://reviewboard.mozilla.org/r/160924/#review166198 ::: layout/style/test/stylo-failures.md:59 (Diff revision 1) > > ## Assertions > > ## Need Gecko change > > * test_specified_value_serialization.html `-webkit-radial-gradient`: bug 1380259 [1] Shouldn't this be fixed as well?
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8889825 [details] Bug 1380259 - stylo: update test expectations. https://reviewboard.mozilla.org/r/160922/#review166200 It doesn't seem to exactly match how Gecko handles it... but if it works, it works. The code there is too complicated, and doesn't seem to be fully standard-conforming... so I'm fine as far as tests pass.
Attachment #8889825 -
Flags: review?(xidorn+moz) → review+
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8889826 [details] Bug 1380259 - update test expectations for test_computed_style.html. https://reviewboard.mozilla.org/r/160924/#review166204 Please ensure that the other failure referring to this bug passes.
Attachment #8889826 -
Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8889825 [details] Bug 1380259 - stylo: update test expectations. I forgot about the failure from test_specified_value_serialization.html It required a few more changes.
Attachment #8889825 -
Flags: review+ → review?(xidorn+moz)
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8889825 [details] Bug 1380259 - stylo: update test expectations. https://reviewboard.mozilla.org/r/160922/#review166566 ::: servo/components/style/values/generics/image.rs:298 (Diff revision 2) > + if let Some(ref a) = *angle { > + dest.write_str(" ")?; > + a.to_css(dest)?; > + } Is it possible that webkit mode radial gradient has angle? IIUC it doesn't? If it could have, wouldn't it be a problem that there is no separator between angle and shape?
Attachment #8889825 -
Flags: review?(xidorn+moz) → review+
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12) > Comment on attachment 8889825 [details] > Bug 1380259 - stylo: serialize radial gradients using modern unprefixed > style. > > https://reviewboard.mozilla.org/r/160922/#review166566 > > ::: servo/components/style/values/generics/image.rs:298 > (Diff revision 2) > > + if let Some(ref a) = *angle { > > + dest.write_str(" ")?; > > + a.to_css(dest)?; > > + } > > Is it possible that webkit mode radial gradient has angle? IIUC it doesn't? > You are right, it seems it doesn't. I removed that piece. Thank you!
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8889826 -
Attachment is obsolete: true
Comment 15•7 years ago
|
||
Pushed by ferjmoreno@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c25880692555 stylo: update test expectations. r=xidorn
Comment 16•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0886a662636d Backed out changeset c25880692555 on developers request
Comment 17•7 years ago
|
||
It seems this breaks some other tests. Please have a try run before landing.
Flags: needinfo?(ferjmoreno)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8889825 [details] Bug 1380259 - stylo: update test expectations. I am afraid I am only able to fix the serialization of the computed value and make test_computed_style.html. All the changes I tried to fix test_specified_value_serialization.html break other tests.
Flags: needinfo?(ferjmoreno)
Attachment #8889825 -
Flags: review+ → review?(xidorn+moz)
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8889825 [details] Bug 1380259 - stylo: update test expectations. https://reviewboard.mozilla.org/r/160922/#review169172
Attachment #8889825 -
Flags: review?(xidorn+moz) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8892837 [details] Bug 1380259 - stylo: update test expectations. https://reviewboard.mozilla.org/r/163840/#review169174
Attachment #8892837 -
Flags: review?(xidorn+moz) → review+
Updated•7 years ago
|
Priority: -- → P2
Comment 23•7 years ago
|
||
Could you land this patch and file another bug for the remaining issue? I'll try to figure out a solution.
Flags: needinfo?(ferjmoreno)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8892837 -
Attachment is obsolete: true
Assignee | ||
Comment 25•7 years ago
|
||
The servo piece is in homu's queue https://reviewboard.mozilla.org/r/160920 And I've reopened bug 1367299 to track the remaining issue.
Flags: needinfo?(ferjmoreno)
Assignee | ||
Comment 26•7 years ago
|
||
Wrong link, it should be https://github.com/servo/servo/pull/17957
Comment 27•7 years ago
|
||
Pushed by ferjmoreno@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4c3434561b7f stylo: update test expectations. r=xidorn
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c3434561b7f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•