Closed Bug 1419355 Opened 8 years ago Closed 8 years ago

Add a test for non-content-accessible properties.

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1419355: Add a test for non-content-accessible properties, and make Stylo and Gecko consistent.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Note that this test written as is also uncovers a bunch of bugs on stylo. I'm working on a fix for those.
https://github.com/servo/servo/pull/19317 is the servo fix that allows this test to pass.
Assignee: nobody → emilio
Comment on attachment 8930459 [details] Bug 1419355: Add a test for non-content-accessible properties, and make Stylo and Gecko consistent. https://reviewboard.mozilla.org/r/201614/#review206900 ::: layout/style/nsCSSPropList.h:4516 (Diff revision 1) > CSS_PROP_UIRESET( > -moz-window-transform, > _moz_window_transform, > CSS_PROP_DOMPROP_PREFIXED(WindowTransform), > CSS_PROPERTY_INTERNAL | > - CSS_PROPERTY_PARSE_FUNCTION | > + CSS_PROPERTY_PARSE_VALUE | This seems to be a mistake?f`-moz-window-transform` is parsed in the same way as `transform`, which uses a parsing function in `nsCSSParser`. Before we actually remove that code, I think we should still keep it working. (It's especially important given that we haven't enabled stylo-chrome yet...)
Attachment #8930459 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8930459 [details] Bug 1419355: Add a test for non-content-accessible properties, and make Stylo and Gecko consistent. https://reviewboard.mozilla.org/r/201614/#review206900 > This seems to be a mistake?f`-moz-window-transform` is parsed in the same way as `transform`, which uses a parsing function in `nsCSSParser`. Before we actually remove that code, I think we should still keep it working. (It's especially important given that we haven't enabled stylo-chrome yet...) Yup, that was a typo, which was happily caught by the own test I wrote for bug 1418963 :). Fixed, and devtools DB updated too.
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/39a68df1359c Add a test for non-content-accessible properties, and make Stylo and Gecko consistent. r=xidorn
Backout by aiakab@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e35aa16d84a6 Backed out changeset 39a68df1359c for failing chrome's toolkit/content/tests/chrome/test_arrowpanel.xul on OS X on a CLOSED TREE
This was backed out for failing chrome's toolkit/content/tests/chrome/test_arrowpanel.xul on OS X in: https://hg.mozilla.org/integration/autoland/rev/e35aa16d84a6 https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=06f11b61118d3f661e7a6e872bc7af9721bf9bd3&filter-searchStr=10.10%20m(c3)&selectedJob=146540420 I have no clue why or how (there was a failing intermittent for it, bug 1351040), so so far I've pushed three try runs, one with the patch as-is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1804060b521531bb6bd47138265689a92cfd55b5 One with the test removed (in case it is due to rebucketing): https://treeherder.mozilla.org/#/jobs?repo=try&revision=970acca26e4dee877918608cbe83138a5c424a9a One with the props hidden, but the test there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76721a3754727f96da23681fec961f08460ff173 If (1) and (2) are orange, but not (3), I'll reland. Otherwise I guess I get to investigate the menupopup code, yay... Though given[1], I guess it may even be a legit failure... [1]: https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/toolkit/content/xul.css#452
So I bet that's it, and that hiding -moz-window-opacity and -moz-window-transform from content pages disables the transition, at least on Gecko (bug 1265611). Birtles, is there any chance to relax this requirement? It's unfortunate that we need to expose those properties to every webpage because otherwise our popups don't have transitions...
Flags: needinfo?(bbirtles)
So, it seems we only parse properties in transition-property for those enabled in content at the omment: https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/layout/style/nsRuleNode.cpp#5723-5731
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > Birtles, is there any chance to relax this requirement? It's unfortunate > that we need to expose those properties to every webpage because otherwise > our popups don't have transitions... I'm sure it's possible but I don't expect its trivial. Throughout animation/transition code we only deal with properties that are enabled for all content (or at least that was the case when I wrote the comment in [1]) so I imagine it's a bit of work to make all that code correctly distinguish between chrome and content documents. I suppose eventually we'll need to change that. [1] https://hg.mozilla.org/mozilla-central/rev/fe27ee21b66b
Flags: needinfo?(bbirtles)
See Also: → 1419695
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d119d5f35935 Add a test for non-content-accessible properties. r=xidorn
I pushed the test with these properties commented out for now. Filed bug 1419695 on the transitions issue.
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/d119d5f35935
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: